Skip to content

#21809 [SecurityBundle] bugfix: if security provider's name contains upper cases then container didn't compile #21810

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

Conversation

antanas-arvasevicius
Copy link
Contributor

@antanas-arvasevicius antanas-arvasevicius commented Mar 1, 2017

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #21809
License MIT

then security.yml providers was with upper case, on container compile error was thrown:

[04:39:32][Ant output]      [exec]      [exec] > Sensio\Bundle\DistributionBundle\Composer\ScriptHandler::clearCache
[04:39:32][Ant output]      [exec]      [exec] 
[04:39:32][Ant output]      [exec]      [exec] 
[04:39:32][Ant output]      [exec]      [exec]   [Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException]
[04:39:32][Ant output]      [exec]      [exec]   The service "security.authentication.provider.simple_form.default" has a de
[04:39:32][Ant output]      [exec]      [exec]   pendency on a non-existent service "security.user.provider.concrete.carrier
[04:39:32][Ant output]      [exec]      [exec]   User".

Problem has occurred with this commit line:
fbd9f88#diff-2be909961a57bf75fbb600c1f5fc46e3R320

Issue fixes with this PR.

@nicolas-grekas nicolas-grekas changed the title #21809 [SecurityBundle] bugfix: if security provider's name contains … #21809 [SecurityBundle] bugfix: if security provider's name contains upper cases then container didn't compile Mar 1, 2017
@nicolas-grekas
Copy link
Member

Any test case?

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Mar 1, 2017
@antanas-arvasevicius
Copy link
Contributor Author

still no, in 20min

@xabbuh
Copy link
Member

xabbuh commented Mar 1, 2017

@antanas-arvasevicius Can you confirm that this change is not necessary if #21811 gets merged?

@antanas-arvasevicius antanas-arvasevicius force-pushed the fix-security-provider-case-sensitive branch from 0329b73 to 7625f9e Compare March 1, 2017 13:51
@antanas-arvasevicius
Copy link
Contributor Author

added test cases for this.
Revertion is not needed, as this bug is due inconsistent getUserProviderId() usage, sometimes it was called like: getUserProviderId(strtolower($name)); sometimes like getUserProviderId($name);

I think it's better to accept this PR as this will prevent that kind of bugs in the future.

…ntains upper cases then container didn't compile
@antanas-arvasevicius antanas-arvasevicius force-pushed the fix-security-provider-case-sensitive branch from 7625f9e to e0107d0 Compare March 1, 2017 14:22
@antanas-arvasevicius
Copy link
Contributor Author

test errors on 5.5 7.1 https://travis-ci.org/symfony/symfony/jobs/206617501#L2417
Error: Class 'Doctrine\ORM\Version' not found
?
How to this fix? Or better remove test case completely?

fabpot added a commit that referenced this pull request Mar 1, 2017
… (xabbuh)

This PR was merged into the 3.3-dev branch.

Discussion
----------

Revert "[SecurityBundle] only pass relevant user provider"

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21809, #21810
| License       | MIT
| Doc PR        |

This reverts commit d97e07f (applies #21798 on `master`). There is no merge commit that could be reverted.

Commits
-------

5b016ce Revert "[SecurityBundle] only pass relevant user provider"
@fabpot
Copy link
Member

fabpot commented Mar 23, 2017

@antanas-arvasevicius Indeed, just removing the orm part of the configuration should be enough.

@fabpot
Copy link
Member

fabpot commented Mar 23, 2017

Thank you @antanas-arvasevicius.

fabpot added a commit that referenced this pull request Mar 23, 2017
…e contains upper cases then container didn't compile (Antanas Arvasevicius)

This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #21810).

Discussion
----------

#21809 [SecurityBundle] bugfix: if security provider's name contains upper cases then container didn't compile

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? no
| Tests pass?   | yes
| Fixed tickets | #21809
| License       | MIT

then security.yml  providers was with upper case, on container compile error was thrown:
````
[04:39:32][Ant output]      [exec]      [exec] > Sensio\Bundle\DistributionBundle\Composer\ScriptHandler::clearCache
[04:39:32][Ant output]      [exec]      [exec]
[04:39:32][Ant output]      [exec]      [exec]
[04:39:32][Ant output]      [exec]      [exec]   [Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException]
[04:39:32][Ant output]      [exec]      [exec]   The service "security.authentication.provider.simple_form.default" has a de
[04:39:32][Ant output]      [exec]      [exec]   pendency on a non-existent service "security.user.provider.concrete.carrier
[04:39:32][Ant output]      [exec]      [exec]   User".

`````

Problem has occurred with this commit line:
fbd9f88#diff-2be909961a57bf75fbb600c1f5fc46e3R320

Issue fixes with this PR.

Commits
-------

6d23c8c #21809 [SecurityBundle] bugfix: if security provider's name contains upper cases then container didn't compile
@fabpot fabpot closed this Mar 23, 2017
@antanas-arvasevicius
Copy link
Contributor Author

@fabpot, if orm is removed then createUserDaoProvider is not called

@fabpot
Copy link
Member

fabpot commented Mar 23, 2017

@antanas-arvasevicius I've taken care of it when merging. Thanks.

This was referenced Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants