[HttpKernel] Renamed the argument resolver tag #18529
Conversation
|
if this does not require any change in the testsuite, it looks like we are missing some tests... |
|
@stof this is functionally tested only (implicitly), I can add a compiler pass unit-test though. |
|
👍 |
|
Added a test and named the compiler pass properly, it was still having "Controller" which is not part of the tag anymore. |
|
👍 |
| @@ -20,7 +20,7 @@ | |||
| * | |||
| * @author Iltar van der Berg <kjarli@gmail.com> | |||
| */ | |||
| class ControllerArgumentValueResolverPass implements CompilerPassInterface | |||
| class ArgumentValueResolverPass implements CompilerPassInterface | |||
There was a problem hiding this comment.
This rename looks wrong to me. Compiler passes is in the Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler namespace, which gives no hints as to what Argument refers to.
|
👎 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. |
|
Other alternatives considered:
|
|
If we were to change this, I would vote for |
|
@xabbuh That's actually the same length as before:
|
|
@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. |
|
That's true, in that case I will also name the class the same as before again. |
|
Rebased to a clean commit, if everyone agrees on the name, I will update my other PRs |
|
👍 |
|
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. |
|
@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.
You can just create a ContainerBuilder instance here (like, for example, done in the FormPassTest and CachePoolPassTest).
There was a problem hiding this comment.
and the same for the other test methods as well
|
@xabbuh, no more mocking ;) |
|
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).