Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Renamed the argument resolver tag#18529
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
stof commentedApr 13, 2016
if this does not require any change in the testsuite, it looks like we are missing some tests... |
linaori commentedApr 13, 2016
@stof this is functionally tested only (implicitly), I can add a compiler pass unit-test though. |
javiereguiluz commentedApr 13, 2016
👍 |
linaori commentedApr 13, 2016
Added a test and named the compiler pass properly, it was still having "Controller" which is not part of the tag anymore. |
nicolas-grekas commentedApr 13, 2016
👍 |
| * @author Iltar van der Berg <kjarli@gmail.com> | ||
| */ | ||
| classControllerArgumentValueResolverPassimplements CompilerPassInterface | ||
| classArgumentValueResolverPassimplements 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.
This rename looks wrong to me. Compiler passes is in theSymfony\Bundle\FrameworkBundle\DependencyInjection\Compiler namespace, which gives no hints as to what Argument refers to.
fabpot commentedApr 13, 2016
👎 I'm all for having shorter names, but they need to be self-descriptive as well. Here, we're not talking about any argument, but only controller arguments. |
javiereguiluz commentedApr 13, 2016
Other alternatives considered:
|
xabbuh commentedApr 13, 2016
If we were to change this, I would vote for |
linaori commentedApr 13, 2016
@xabbuh That's actually the same length as before:
|
xabbuh commentedApr 13, 2016
@iltar That's true. But it still conveys the proper meaning while making the namespace more clear imo. To me all other suggestions lose too much information. |
linaori commentedApr 13, 2016
That's true, in that case I will also name the class the same as before again. |
linaori commentedApr 14, 2016
Rebased to a clean commit, if everyone agrees on the name, I will update my other PRs |
fabpot commentedApr 14, 2016
👍 |
xabbuh commentedApr 14, 2016
Why do you mock the DI related classes in the test? Imo you should rather use the real classes and check that the definitions are modified as expected. |
linaori commentedApr 14, 2016
@xabbuh I can do that, this was simply copied from the other tests |
| ); | ||
| $definition = new Definition(ArgumentResolver::class, array(null, array())); | ||
| $container = $this->getMock(ContainerBuilder::class, array('findTaggedServiceIds')); |
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.
You can just create aContainerBuilder instance here (like, for example, done in theFormPassTest andCachePoolPassTest).
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.
and the same for the other test methods as well
linaori commentedApr 15, 2016
@xabbuh, no more mocking ;) |
fabpot commentedApr 15, 2016
Thank you@iltar. |
This PR was merged into the 3.1-dev branch.Discussion----------[HttpKernel] Renamed the argument resolver tag| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | not if merged before 3.1| Deprecations? | no| Tests pass? | yes| Fixed tickets | ~| License | MIT| Doc PR | ~Changed as discussed several times:#18510 (comment),symfony/symfony-docs#6422 (comment).Commits-------cd10057 Renamed argument resolver tag, added test
Changed as discussed several times:#18510 (comment),symfony/symfony-docs#6422 (comment).