Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle][Workflow] better errors when security deps are missing#23638
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| /** | ||
| * @author Christian Flothmann <christian.flothmann@sensiolabs.de> | ||
| */ | ||
| class GuardListenerPass implements CompilerPassInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The name should probably containWorkflow as this is really just the for workflow configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
renamed toWorkflowGuardListenerPass
9bb2c28 tof54c374Compare| public function testListenersAreNotRemovedIfAllDependenciesArePresent() | ||
| { | ||
| $this->container->setParameter('workflow.guard_listeners', ['foo.listener.guard', 'bar.listener.guard']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should use long array syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
thx
f54c374 to8ec0db2Compare| */ | ||
| public function process(ContainerBuilder $container) | ||
| { | ||
| if (!$container->hasParameter('workflow.guard_listeners')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't see the place setting this parameter in FrameworkBundle. Are you sure your code works fine ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
good catch, fixed
8ec0db2 to3b884b0Compare
lyrixx left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 except one little change.
| return; | ||
| } | ||
| if (!$container->has('security.token_storage')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you remove the parameterworkflow.has_guard_listeners? It's not need at runtime.
(I added the comment here because I guess you will do it a this line ;) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed :)
| * file that was distributed with this source code. | ||
| */ | ||
| namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure it's need but now we add all Compiler pass in the associated component.
It's a forgetfulness or it's done on purpose? (I dont this it's necessary here but ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO this is completely specifc to the FrameworkBundle. If you want to register the listener in the container in your own application, you will probably have better ways to check if all needed dependencies are present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
3b884b0 to56ee4aaComparelyrixx commentedAug 4, 2017
👍 |
lyrixx commentedAug 4, 2017
Thank you@xabbuh. |
…ps are missing (xabbuh)This PR was merged into the 3.3 branch.Discussion----------[FrameworkBundle][Workflow] better errors when security deps are missing| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#23499 (comment)| License | MIT| Doc PR |All the referenced services must either be explicitly configured or the SecurityBundle must be installed with some security related config being enabled. Otherwise, you will end up with a not so useful error message.Commits-------56ee4aa better errors when security deps are missing
All the referenced services must either be explicitly configured or the SecurityBundle must be installed with some security related config being enabled. Otherwise, you will end up with a not so useful error message.