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

[SecurityBundle] Prevent auto-registration of UserPasswordEncoderCommand #22858

Merged
merged 1 commit into from May 23, 2017

Conversation

Projects
None yet
5 participants
@chalasr
Member

chalasr commented May 22, 2017

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22855
License MIT
Doc PR n/a

Always registers the command as a service even if no encoders are configured so that it is not auto-registered via reflection.
Prevents an irrelevant deprecation when no encoders are configured and, if a day the per-convention command registration is removed, ensures that the command always exists (leading to an exception saying that you have no configured encoder, better than nothing).
ping @ogizanagi

@ogizanagi

Oh...Now I understand this issue better indeed. Good catch. 👍

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan May 22, 2017

Member

👍 Fixes the issue

However, I am a bit worried about the PR that introduced this - #20677 - specifically that it adds the EncoderFactoryInterface to the constructor. I think we need to be careful to keep commands lazy - I believe this change will cause that service to be instantiated just by running bin/console with no arguments.

Member

weaverryan commented May 22, 2017

👍 Fixes the issue

However, I am a bit worried about the PR that introduced this - #20677 - specifically that it adds the EncoderFactoryInterface to the constructor. I think we need to be careful to keep commands lazy - I believe this change will cause that service to be instantiated just by running bin/console with no arguments.

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone May 22, 2017

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi May 22, 2017

Member

@weaverryan : Which is something that #22734 would solve. (Anyway for this particular case, EncoderFactory is pretty cheap to instantiate)

Member

ogizanagi commented May 22, 2017

@weaverryan : Which is something that #22734 would solve. (Anyway for this particular case, EncoderFactory is pretty cheap to instantiate)

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan May 22, 2017

Member

@ogizanagi Thanks for pointing me to that :). I'd like to see that get done!

👍

Member

weaverryan commented May 22, 2017

@ogizanagi Thanks for pointing me to that :). I'd like to see that get done!

👍

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas
Member

nicolas-grekas commented May 23, 2017

Thank you @chalasr.

@nicolas-grekas nicolas-grekas merged commit b39b35b into symfony:3.3 May 23, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request May 23, 2017

bug #22858 [SecurityBundle] Prevent auto-registration of UserPassword…
…EncoderCommand (chalasr)

This PR was merged into the 3.3 branch.

Discussion
----------

[SecurityBundle] Prevent auto-registration of UserPasswordEncoderCommand

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22855
| License       | MIT
| Doc PR        | n/a

Always registers the command as a service even if no encoders are configured so that it is not auto-registered via reflection.
Prevents an irrelevant deprecation when no encoders are configured and, if a day the per-convention command registration is removed, ensures that the command always exists (leading to an exception saying that you have no configured encoder, better than nothing).
ping @ogizanagi

Commits
-------

b39b35b Prevent auto-registration of UserPasswordEncoderCommand

@chalasr chalasr deleted the chalasr:fix-deprec branch May 23, 2017

@fabpot fabpot referenced this pull request May 29, 2017

Merged

Release v3.3.0 #22949

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment