Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added powered-by-header #2618

Merged
merged 7 commits into from
Mar 5, 2021
Merged

added powered-by-header #2618

merged 7 commits into from
Mar 5, 2021

Conversation

luceos
Copy link
Member

@luceos luceos commented Feb 20, 2021

Adds a X-Powered-By header. It can be disabled using:

Flarum\Http\Middleware\FlarumPromotionHeader::disable() in extend.php.

Copy link
Contributor

@KyrneDev KyrneDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I didn't know we needed but I love it! Everything works locally from testing.

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be disabled using:

Flarum\Http\Middleware\FlarumPromotionHeader::disable() in extend.php.

The Compat extender is going away with stable, so no more callbacks in extend.php. I suppose it could also be done outside a callback, but I would prefer for extend.php to remain just extenders. Instead, let's use the config.php.

src/Http/Middleware/FlarumPromotionHeader.php Outdated Show resolved Hide resolved
@luceos
Copy link
Member Author

luceos commented Mar 3, 2021

I've refactored based on the review. You can now disable the promotion header in the config.php like so:

<?php

return [
    'url' => 'http://toby.local',
    'debug' => true,
    'promote' => false,
    // ..
];

If the promote value is true, null or is not available from the config it will assume true.

{
static::$enabled = false;
$this->enabled = $config['promote'] ?? true;
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->enabled = $config['promote'] ?? true;
$this->enabled = $config['flarumPromotionalHeader'] ?? true;

Copy link
Contributor

@tankerkiller125 tankerkiller125 Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like that suggested name is a little long? I'm perfectly fine with simple "promote" if we did want to explain it more maybe "promotionHeader" or "poweredByHeader"

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

@SychO9
Copy link
Member

SychO9 commented Mar 3, 2021

can we also add the key to StoreConfig.php so that it's there by default ?

@askvortsov1
Copy link
Sponsor Member

can we also add the key to StoreConfig.php so that it's there by default ?

Good point. It'll also be in the release announcement and config.php docs

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "promote" is too broad, "promoteFlarumHeader" or "promoteFlarum" might be better.

@KyrneDev
Copy link
Contributor

KyrneDev commented Mar 5, 2021

I think "promote" is too broad, "promoteFlarumHeader" or "promoteFlarum" might be better.

What about "promoHeader"

@askvortsov1 askvortsov1 added this to the 0.1.0-beta.16 milestone Mar 5, 2021
@askvortsov1 askvortsov1 merged commit 4b0ad69 into master Mar 5, 2021
@askvortsov1 askvortsov1 deleted the dk/powered-by-header branch March 5, 2021 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants