[TwigComponent] Fix LiveComponent namespace mapping #1772
Conversation
smnandre
force-pushed
the
twig/fix-namespace-template-mapping
branch
from
4269528 to
0514f59
Compare
| @@ -11,21 +11,24 @@ | |||
|
|
|||
| namespace Symfony\UX\TwigComponent\Twig; | |||
|
|
|||
| /** | |||
| * @internal | |||
| */ | |||
| final class TemplateNameParser | |||
There was a problem hiding this comment.
Is this class still necessary?
It's not parsing a template name anymore, but just validating the template name, which already happens in Twig\Loader\FilesystemLoader::parseName (in all scenarios in this class, it's just returning the name as-is, so it's just duplicating the validation logic from FilesystemLoader)
There was a problem hiding this comment.
But we cannot remove it because BC i guess.. @kbond ?
There was a problem hiding this comment.
I consider this class internal so let's remove if not required anymore.
|
Ready to merge (confirmed to fix the original issue #1754 (comment) ) when you want guys |
kbond
force-pushed
the
twig/fix-namespace-template-mapping
branch
from
d68cc3c to
bb76055
Compare
|
Thanks Simon. |
|
Thank you, everyone! |
|
Hi Simon (@smnandre), It seems it does introduce a BC break, because my templates are no longer found now :\ I'll investigate why this is happening, but since the namespace is no longer stripped (https://github.com/symfony/ux/pull/1772/files#diff-13fd1de1f292494959041e77eee8b739859160fb2f7da4290d278c9bdcdcb907L28). It cannot be found with the template name including the namespace. However... perhaps it should be in the template map WITH a namespace.... |
|
I went down to the TemplateIterator code, and it seems that my "fix" in #1070 was not correct. Shopware, in which I'm using Live components, is stripping the namespace in its decorated version of the template iterator. So to conclude: the TemplateNameParser is indeed obsolete now. I'll create a new PR to remove it! |
…yvv) This PR was merged into the 2.x branch. Discussion ---------- [TwigComponent] Remove obsolete TemplateNameParser It no longer parses (and should not have been stripping namespaces as it did before) | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Issues | | License | MIT As discussed in #1772 (comment), the code added in #1070 (& expanded in #1082) was not needed (even incorrect, based upon a decorator TemplateIterator of Shopware). Commits ------- 99cb909 Remove obsolete TemplateNameParser
…yvv) This PR was merged into the 2.x branch. Discussion ---------- [TwigComponent] Remove obsolete TemplateNameParser It no longer parses (and should not have been stripping namespaces as it did before) | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Issues | | License | MIT As discussed in symfony/ux#1772 (comment), the code added in #1070 (& expanded in #1082) was not needed (even incorrect, based upon a decorator TemplateIterator of Shopware). Commits ------- 99cb909ad Remove obsolete TemplateNameParser
I believe this modification fix #1754
Original problem
If you render a template @Acme/dashboard.html.twig, embedded Components (live or twig) referenced wrong template (dashboard.html.twig here), making LiveComponent impossible to rerender (this file beeing not referenced in the TemplateMap).
This behaviour was introduced in #1082 (and worked then). But i'm convinced all the work/changes made by Ryan to fix the embed bug (#1247 and related) made this obselete (and in this particular situation even originates a bug)
All tests are green, and i'd like to test it more heavily... but before that : do you think this is a bug fix or is there some kind of BC break here?