NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[SecurityBundle] Make enable_authenticator_manager true as there is no other way in Symfony 6#44554

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

wouterj merged 1 commit intosymfony:6.0fromalexander-schranz:patch-10

Dec 12, 2021

Merged

[SecurityBundle] Make enable_authenticator_manager true as there is no other way in Symfony 6#44554

wouterj merged 1 commit intosymfony:6.0fromalexander-schranz:patch-10

Dec 12, 2021

Conversation

Copy link

Contributor

alexander-schranz commentedDec 10, 2021

Q	A
Branch?	6.0 for features / 4.4, 5.3, 5.4 or 6.0 for bug fixes
Bug fix?	no
New feature?	no
Deprecations?	/no
Tickets	Fix #...
License	MIT
Doc PR	symfony/symfony-docs#...

Did read the new docs and I was confused about:

security:    enable_authenticator_manager: true

as I thought thats the default now and there is no other way. And that seems to case as if you set it to false it will error in:

symfony/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

Lines 93 to 95 in6ab662b

	if (!$config['enable_authenticator_manager']) {
	thrownewInvalidConfigurationException('"security.enable_authenticator_manager" must be set to "true".');
	}

So I would remove theenable_authenticator_manager also from the 6.0 docs and make this just the default to avod confusion here.

alexander-schranz requested review fromchalasr andwouterj ascode owners

December 10, 2021 13:56

carsonbot added the Status: Needs Review label

Dec 10, 2021

carsonbot added this to the6.0 milestone

Dec 10, 2021

alexander-schranz mentioned this pull request

Dec 10, 2021

Remove "enable_authenticator_manager" from Symfony 6 docssymfony/symfony-docs#16218

Merged

alexander-schranz commented

Dec 10, 2021

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php OutdatedShow resolvedHide resolved

wouterj requested changes

Dec 10, 2021

View reviewed changes

Copy link

Member

wouterj left a comment

Choose a reason for hiding this comment

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

We're a bit stricter with what is considered a bugfix or a feature, and given there was no bug before (just an oddity) I think we should call this a feature and target 6.1.

That would also allow us to immediately deprecating setting this option (I think there is a build indeprecate() method for this). This also resolves the description question 🙂

What do you think?

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Dec 10, 2021

nicolas-grekas approved these changes

Dec 11, 2021

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(after the message is removed)

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Dec 11, 2021

Copy link

Member

nicolas-grekas commentedDec 11, 2021

I missed your comment@wouterj :)
But since not setting/setting to false this option is deprecated in 5.3, I think this should have been part of the cleanup doe in 6.0.false yields an exception already in 6.0. Defaulting to something that throws looks odd to me.

Copy link

Member

wouterj commentedDec 11, 2021

Alright, I'm OK with having this in 6.0😉

wouterj approved these changes

Dec 12, 2021

View reviewed changes

Make enable_authenticator_manager true as there is no other way in Sy…

b7a2ae6

…mfony 6

wouterj force-pushed thepatch-10 branch from710e2d5 tob7a2ae6Compare

December 12, 2021 12:30

Copy link

Member

wouterj commentedDec 12, 2021

Thanks Alexander!

wouterj merged commit348f252 intosymfony:6.0

Dec 12, 2021

alexander-schranz deleted the patch-10 branch

December 12, 2021 13:35

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request

Dec 13, 2021

minor#16218Remove "enable_authenticator_manager" from Symfony 6 doc…

917a47e

…s (alexander-schranz)This PR was merged into the 6.0 branch.Discussion----------Remove "enable_authenticator_manager" from Symfony 6 docsSeesymfony/symfony#44554 to avoid confusion that there is something else.Commits-------df24637 Remove "enable_authenticator_manager" from Symfony 6 docs

xabbuh added the SecurityBundle label

Dec 13, 2021

carsonbot changed the title~~Make enable_authenticator_manager true as there is no other way in Symfony 6~~[SecurityBundle] Make enable_authenticator_manager true as there is no other way in Symfony 6

Dec 13, 2021