[TwigComponent] Fix loading Twig Components by FQCN #3196
Conversation
This fixes the issue that when relying on the classMap, even components with classes would be loaded as anonymous components, because loading an anonymous template was tried before even checking the classMap. This fixes the issue by resolving the class map right at the beginning, if possible. This also allows us to reuse the code that loads the metadata: As we make sure `$name` holds the real config key right at the beginning, we don't have to care anymore about whether the caller provided the internal config key or a class name. The special case for class names can go. As a side effect, we get a much better error message when attempting to load using an FQCN, because of the code reuse.
carsonbot
added
Bug
|
I couldn't find a PHP-CS-Fixer config, so I just tried to match the coding style I saw in the rest of the code the best I could |
Kocal
added
Status: Waiting feedback
I have no memory of that ... where do we wrote this ? |
The docs mention it for test code, and this is where the bug occured to me – it broke my tests. https://symfony.com/bundles/ux-twig-component/current/index.html#test-helpers It seems like If we come to the conclusion that support for FQCNs for TwigComponents should be dropped, we should remove the population of that property as well as everything in |
WebMamba
left a comment
WebMamba
left a comment
There was a problem hiding this comment.
I need to dig more into this I don't understand how this could work. Looks something is broken somewhere
| if ($mappedName = $this->classMap[$name] ?? null) { | ||
| if ($config = $this->config[$mappedName] ?? null) { | ||
| return new ComponentMetadata($config); | ||
| } | ||
|
|
||
| throw new \InvalidArgumentException(\sprintf('Unknown component "%s".', $name)); | ||
| } |
There was a problem hiding this comment.
I don't understand if you remove all of this this method only search for anonymous component then? It's mean something is wrong with the current architecture and we don't need to separate the finding of the template of the anonymous component and the normal component
There was a problem hiding this comment.
Of course we do! One is compilable once, the other is/may not be :)
There was a problem hiding this comment.
I don't understand if you remove all of this this method only search for anonymous component then?
No! The first thing it does is trying to create a "normal" component. (see https://github.com/symfony/ux/pull/3196/files#r2591624694) Only if this doesn't work, it tries to create an anonymous component.
Opposed to how it worked before:
First, it tried to create a "normal" component. If that didn't work using the identifier, it tried to load an anonymous component. And only if both of those didn't work, it had the same code for loading a "normal" component again, just this time with the resolved class map.
Now we resolve the class map from the beginning on, so we don't need to repeat the code just to do the same thing again with the name from the resolved class map.
| /** | ||
| * @testWith ["bar"] | ||
| * ["Foo\\Bar"] | ||
| */ |
There was a problem hiding this comment.
you can remove the comment here
There was a problem hiding this comment.
I don't understand. Should I replace @testWith with an attribute or a dataprovider?
There was a problem hiding this comment.
The comment must be kept for PHPUnit 9 compatibility, but #[TestWith(...)] must be added for PHPUnit 10+ (UX 3.x)
| return new ComponentMetadata($this->config[$name]); | ||
| } | ||
|
|
||
| if ($mappedName = $this->classMap[$name] ?? null) { |
There was a problem hiding this comment.
That's why this PR proposes to evaluate it before, not after trying to load the component anonymously
|
-- Discussion ongoing in issue #3195 -- |
| if ($config = $this->config[$name] ?? null) { | ||
| return new ComponentMetadata($config); | ||
| } |
There was a problem hiding this comment.
This is the place where "normal" (non-anonymous) components get created. If we resolve the classMap first and save it in $name, there's no need for the duplication I removed.
|
Let's close here, original issue fixed by a configuration update. |
5 participants
When loading Twig Components, code needs a way to identify the component. You can use the identifiers that
ComponentFactoryinternally uses (this will usually be an identifier consisting of the namespaces separated by:). But as the docs still state, past versions of ux-twig-component could also load them by the fully-qualified PHP class name of the component class.da0537d broke this: From this commit on,
ComponentFactory, which is responsible for resolving the identifier the user provided to a Twig template and a class, would only attempt to resolve an FQCN if it couldn't provide metadata for loading the component anonymously (which means without a class, just the Twig template).Because loading the component anonymously always works as long as there is a Twig template,In case the FQCN could also be resolved as a Twig template,ComponentFactorywould not resolve FQCNs to a class component, and instead load only the corresponding Twig template as an anonymous component.The issue affects all versions from
2.20.0to2.31.0.This PR fixes this by trying to resolve the FQCN right at the beginning, makig sure the first attempt to load the component with the class already happens using the right identifier.