-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Definition::removeMethodCall should remove all matching calls #40167
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hey! Excellent, keep up the good work. I think @atailouloute has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
fabpot
approved these changes
Feb 14, 2021
It would only remove the first match, leaving the other method call(s) there to exist. This leads to unexpected situations.
a2ae93d
to
944ba23
Compare
Thank you @ruudk. |
m-vo
added a commit
to m-vo/contao
that referenced
this pull request
May 14, 2021
see symfony/symfony#40167 (Definition::removeMethodCall won't remove all calls)
m-vo
added a commit
to m-vo/contao
that referenced
this pull request
May 14, 2021
see symfony/symfony#40167 (Definition::removeMethodCall won't remove all calls)
m-vo
added a commit
to m-vo/contao
that referenced
this pull request
Jul 2, 2021
see symfony/symfony#40167 (Definition::removeMethodCall won't remove all calls)
m-vo
added a commit
to m-vo/contao
that referenced
this pull request
Jul 2, 2021
see symfony/symfony#40167 (Definition::removeMethodCall won't remove all calls)
m-vo
added a commit
to m-vo/contao
that referenced
this pull request
Jul 9, 2021
see symfony/symfony#40167 (Definition::removeMethodCall won't remove all calls)
leofeyer
pushed a commit
to contao/contao
that referenced
this pull request
Jul 14, 2021
Description ----------- | Q | A | -----------------| --- | Fixed issues | Fixes #2303 and potentially #2638 | Docs PR or issue | todo supersedes #2612 This PR aims at implementing first steps of native Twig support for Contao. # What's inside ### Encoding You now can write Twig templates like so… ```twig <h1>{{ headline }}</h1> <p class="rte">{{ content|raw }}</p> ``` … and provide input encoded data for the variables. They won't be encoded twice thanks to setting `double_encode: false` (idea © @ausi). This means you do not have to use `|raw` for normal values or, put differently, only where you indeed intend to output the raw data - for example when dealing with rte data. This way templates can be written like they would with output encoding and we get a nice upgrade path. This logic is implemented in a `NodeVisitor` that rewrites the escaping strategy to our own one called `contao_html`. It is only applied to template names that follow a certain rule set (`'%^@contao(_[a-zA-Z0-9_-]*)?/%'`) but you can add your own rules as well. ### Loader Twig is configured with a `ChainLoader` that supports multiple loaders by default. We're using this fact and are now providing two additional loaders with a higher priority (default has priority 0): 1. `FailTolerantFilesystemLoader` (priority: 1) - this is just a simple wrapper around the original implementation that doesn't care about adding missing paths. Paths that Symfony adds at compile (i.e. all the Twig bundle paths) do get rewired to this loader in a compiler pass. We kind of used this mechanism already (only with decoration instead of a new loader). 2. `ContaoFilesystemLoader` (priority: 2) - this is where all the magic happens. This loader will provide all Contao templates from bundles, the app, global templates, theme folders… It is also designed to be altered at runtime and uses a cache pool to store its state. Here are some features that differ from Twig's default filesystem loader: - If you request a template but the current page context is 'a theme' and there is a theme variant of the template, you'll get this instead. In fact that's the case for every operation (`getCacheKey`, `getSourceContext`, `exists`, `isFresh`). - When adding template paths (namespace + directory) there is a switch to enable 'tracking templates'. If enabled, the content of the path will be scanned and found templates will be put in a 'hierarchy' that represents a possible inheritance chain (see below). Tracking templates also allows invalidation (see `isFresh`) for associated variants. Our filesystem loader gets primed by a mandatory cache warmer (`ContaoFilesystemLoaderWarmer`) that'll add all the paths and namespaces. Speaking of namespaces, these are the ones getting registered: 1. `Contao_Theme_<themeSlug>` for each theme folder 2. `Contao_Global` for the global templates folder 3. `Contao_<bundleName>` for each bundle path (could be multiple per bundle); this includes `Contao_App` for the app resources (i.e. in a `AppBundle` or `/contao/templates`). 4. additionally `Contao` for all resources Also see the `TemplateLocator` helper service for reference. Btw.: The existing `AddResourcesPass` also searches these paths but behaves differently. Maybe we could merge those two things in the future. But I'm not planning to do this in this PR. ### Inheritance Twig allows inheriting multiple times (`A extends B extends C extends …`) which is a very helpful concept when building complex components. However this doesn't play nicely with an extension system like ours where the code does not know how it will be orchestrated. Imagine you've got an original resource and you are adjusting it in the app: ```twig {# the original thing.html.twig #} {% block content %}{{ foo }}{% endblock %} <footer>{% block footer %}Footer{% endblock %}</footer> ``` ```twig {# App #} {% extends "@Contao/thing.html.twig" %} {% block content %}<section>{{ parent() }}</section>{% endblock %} {% block footer %}My own footer{% footer %} ``` Now you install an extension (think of an extension to a bigger extension) that brings its own modifications: ```twig {# BarExtension #} {% extends "@Contao/thing.html.twig" %} {% block content %}{{ parent() }}? Bar!{% endblock %} ``` You would probably expect this output: ```twig <section>{{ foo }}? Bar!</section> <footer>My own footer</footer> ``` And in fact, this is what you would get with this PR. 😁 You may have noticed the namespace every template is extending from is `@Contao`. In regular Twig land there can only ever win a single template (with the same name) in each namespace, in fact this is why Symfony registers each bundle namespace twice (`@<bundle>` and `@!<bundle>`), so that you can overwrite a template and still extend from it (using the `!` one). We're working around this limitation using two things: we know about templates not only template paths (see tracking in previous section) and we're providing our own version of the `ExtendsTokenParser` that - whenever a `@Contao` template is targeted - will rewrite it to a specific namespace according to the inheritance hierarchy. So `@Contao/foo.html.twig` would for instance become `@Contao_App/foo.html.twig` in the theme template, `@Contao_FooBundle/foo.html.twig` in the app template and `@Contao_CoreBundle/foo.html.twig` in the FooBundle template. ### PHP Template Interop Twig and our current templates are similar in a way that at the end PHP code is evaluated and both support template inheritance. But that's kind of it. 😄 To be able to make them extend from each other seamlessly we IMHO have to transpile PHP to Twig and process them like regular Twig templates. But that's something for the future. What **is** possible with this PR is extending a `.html5` template in Twig (like `{% extends "@contao\nav_default.html5" %}`) but bear in mind that this won't allow certain dynamic things like introducing new blocks and extending again. This part is probably the thing that involves most "magic". Here is the gist how it works: We're also registering `.html5` templates as if they were Twig templates in our loader. The loader, however, strips the PHP code when returning a source context to not confuse Twig's lexer/parser chain but simply keeps the identified block names in there. Later on - in a node visitor - we're then dressing the `ModuleNode` of this template to include a proxy call and inject `BlockNode` s that return `[[TL_PARENT]]` and that are rendered when running the proxy code. If a block was overwritten by a child template it will win, otherwise the placeholder will be output. Contao's template engine then gets run with the blocks prepopulated and will output the result. Commits ------- 0f3c623 add ContaoEscaper that does not double encode input 9cbb7bb add NodeVisitor that can alter escape filter strategies e28bb7c add InteropExtension that allows registering templates that should get processed by the ContaoEscaperNodeVisitor d594574 add our own FilesystemLoader instead of decorating 5eda502 add ContaoTwigTemplateLocator + rewire/register locations in TwigPathsPass compiler pass 5e9b81d delegate rendering to twig if a template surrogate exists 15ebc62 register template for input encoding when rendering via TemplateInheritance c7406f9 fix tests 1cd3c1d add todo note for theme mapping f0f90e6 check for non-existing directories in ContaoTwigTemplateLocator 784af57 cleanup 3d10b53 add and configure TemplateHierarchy + DynamicExtendsTokenParser 0b3e6b3 handle uppercase entities 3188917 rework and simplify the concept f71b882 reimplement contao resources location 193cf15 fix service declaration 80ee873 make signature compatible to legacy interface 93016d7 fix tests 2fdd8e4 raise symfony/dependency-injection dependency see symfony/symfony#40167 (Definition::removeMethodCall won't remove all calls) 018f7be throw if templates are ambiguous (multiple variants in subfolders) b93d098 CS 8db05ee try to find out what's wrong on windows 9ca8e27 fix comments 8f0bd98 canonicalize paths fc2af8a fix locator paths b2f0c62 add shortcut to refresh the loader 87d1374 add automatic cache busting 4c2c0fc add command to list + refresh the current template hierarchy 12780cc fix tests c8474d4 split loader responsibilities c2f1f76 refactor ContaoFilesystemLoader + make it only apply to Contao namespaces cedf138 make warmer auto refresh in dev mode ec092b6 add and improve tests 91476a7 improve regular expressions 181c099 add more tests 3fed107 add more tests fd33065 add more tests 1ee756b hash each individual hierarchy chain 36a9cfb also load html5 templates + allow extending them e7f0720 refactor legacy block rendering 9e72dcd rework cache busting f65800e add + adjust tests 30d68e6 CS + path normalizing babf65f fix call 3647786 fix version constraints d3cf90e improve block regex Co-authored-by: Martin Auswöger <martin@auswoeger.com> b9bbcf2 improve + harden block parsing ee73cf2 revert changes to unaffected code e4243c3 improve naming + comments c970703 remove static keyword from callback function d32aa44 always use the FrontendTemplate class to render legacy templates 6e55565 WIP 7612606 fix filemtime check 8f854fa token parsers: improve support for extends/embed + support include 2d26645 add context transformer to support closures c690031 improve debug command 1e57321 add todo 3b5253f add tests for DebugContaoTwigCommand 1711029 add + fix tests for contao twig extension 7cef52f fix tests 90c4be1 mark classes as final/internal/experimental 23de8dd fix tests 3262d9b add todo placeholders 39d8b4f add tests for ContextHelper 77430e9 fix phpstan errors 9ae391a fix psalm + php7.3 tests b2ea883 simplify + harden twig_include function call 0609427 use named arguments 3625509 ask Twig's CoreExtension for the twig_include callable 39c4761 add and improve tests bf862f5 simplify call de4f44c improve coverage b84cae3 simplify expression 25a3f18 improve coverage 12a7fc1 fix tests 27ca43d ignore coverage for PHP7.3 code 47012da improve coverage 79c0534 improve coverage cdc553a fix test dependency c414250 fix tests on Windows ec65108 add contao_sections + contao_section methods 544a95b small improvement for generated PhpTemplateProxyNode code 9188b03 refactor/unify Twig extensions + runtimes bef9bab fix tests 1c9f019 check for closures instead of callables when transforming the context 9edace8 remove redundant variable bcad333 remove redundant return statement Co-authored-by: Martin Auswöger <martin@auswoeger.com> 80dcb4f handle filemtime only generates warnings, no errors 3b4c877 CS 2c6346f CS f65506b remove redundant return value
leofeyer
pushed a commit
to contao/core-bundle
that referenced
this pull request
Jul 14, 2021
Description ----------- | Q | A | -----------------| --- | Fixed issues | Fixes #2303 and potentially #2638 | Docs PR or issue | todo supersedes #2612 This PR aims at implementing first steps of native Twig support for Contao. # What's inside ### Encoding You now can write Twig templates like so… ```twig <h1>{{ headline }}</h1> <p class="rte">{{ content|raw }}</p> ``` … and provide input encoded data for the variables. They won't be encoded twice thanks to setting `double_encode: false` (idea © @ausi). This means you do not have to use `|raw` for normal values or, put differently, only where you indeed intend to output the raw data - for example when dealing with rte data. This way templates can be written like they would with output encoding and we get a nice upgrade path. This logic is implemented in a `NodeVisitor` that rewrites the escaping strategy to our own one called `contao_html`. It is only applied to template names that follow a certain rule set (`'%^@contao(_[a-zA-Z0-9_-]*)?/%'`) but you can add your own rules as well. ### Loader Twig is configured with a `ChainLoader` that supports multiple loaders by default. We're using this fact and are now providing two additional loaders with a higher priority (default has priority 0): 1. `FailTolerantFilesystemLoader` (priority: 1) - this is just a simple wrapper around the original implementation that doesn't care about adding missing paths. Paths that Symfony adds at compile (i.e. all the Twig bundle paths) do get rewired to this loader in a compiler pass. We kind of used this mechanism already (only with decoration instead of a new loader). 2. `ContaoFilesystemLoader` (priority: 2) - this is where all the magic happens. This loader will provide all Contao templates from bundles, the app, global templates, theme folders… It is also designed to be altered at runtime and uses a cache pool to store its state. Here are some features that differ from Twig's default filesystem loader: - If you request a template but the current page context is 'a theme' and there is a theme variant of the template, you'll get this instead. In fact that's the case for every operation (`getCacheKey`, `getSourceContext`, `exists`, `isFresh`). - When adding template paths (namespace + directory) there is a switch to enable 'tracking templates'. If enabled, the content of the path will be scanned and found templates will be put in a 'hierarchy' that represents a possible inheritance chain (see below). Tracking templates also allows invalidation (see `isFresh`) for associated variants. Our filesystem loader gets primed by a mandatory cache warmer (`ContaoFilesystemLoaderWarmer`) that'll add all the paths and namespaces. Speaking of namespaces, these are the ones getting registered: 1. `Contao_Theme_<themeSlug>` for each theme folder 2. `Contao_Global` for the global templates folder 3. `Contao_<bundleName>` for each bundle path (could be multiple per bundle); this includes `Contao_App` for the app resources (i.e. in a `AppBundle` or `/contao/templates`). 4. additionally `Contao` for all resources Also see the `TemplateLocator` helper service for reference. Btw.: The existing `AddResourcesPass` also searches these paths but behaves differently. Maybe we could merge those two things in the future. But I'm not planning to do this in this PR. ### Inheritance Twig allows inheriting multiple times (`A extends B extends C extends …`) which is a very helpful concept when building complex components. However this doesn't play nicely with an extension system like ours where the code does not know how it will be orchestrated. Imagine you've got an original resource and you are adjusting it in the app: ```twig {# the original thing.html.twig #} {% block content %}{{ foo }}{% endblock %} <footer>{% block footer %}Footer{% endblock %}</footer> ``` ```twig {# App #} {% extends "@Contao/thing.html.twig" %} {% block content %}<section>{{ parent() }}</section>{% endblock %} {% block footer %}My own footer{% footer %} ``` Now you install an extension (think of an extension to a bigger extension) that brings its own modifications: ```twig {# BarExtension #} {% extends "@Contao/thing.html.twig" %} {% block content %}{{ parent() }}? Bar!{% endblock %} ``` You would probably expect this output: ```twig <section>{{ foo }}? Bar!</section> <footer>My own footer</footer> ``` And in fact, this is what you would get with this PR. 😁 You may have noticed the namespace every template is extending from is `@Contao`. In regular Twig land there can only ever win a single template (with the same name) in each namespace, in fact this is why Symfony registers each bundle namespace twice (`@<bundle>` and `@!<bundle>`), so that you can overwrite a template and still extend from it (using the `!` one). We're working around this limitation using two things: we know about templates not only template paths (see tracking in previous section) and we're providing our own version of the `ExtendsTokenParser` that - whenever a `@Contao` template is targeted - will rewrite it to a specific namespace according to the inheritance hierarchy. So `@Contao/foo.html.twig` would for instance become `@Contao_App/foo.html.twig` in the theme template, `@Contao_FooBundle/foo.html.twig` in the app template and `@Contao_CoreBundle/foo.html.twig` in the FooBundle template. ### PHP Template Interop Twig and our current templates are similar in a way that at the end PHP code is evaluated and both support template inheritance. But that's kind of it. 😄 To be able to make them extend from each other seamlessly we IMHO have to transpile PHP to Twig and process them like regular Twig templates. But that's something for the future. What **is** possible with this PR is extending a `.html5` template in Twig (like `{% extends "@contao\nav_default.html5" %}`) but bear in mind that this won't allow certain dynamic things like introducing new blocks and extending again. This part is probably the thing that involves most "magic". Here is the gist how it works: We're also registering `.html5` templates as if they were Twig templates in our loader. The loader, however, strips the PHP code when returning a source context to not confuse Twig's lexer/parser chain but simply keeps the identified block names in there. Later on - in a node visitor - we're then dressing the `ModuleNode` of this template to include a proxy call and inject `BlockNode` s that return `[[TL_PARENT]]` and that are rendered when running the proxy code. If a block was overwritten by a child template it will win, otherwise the placeholder will be output. Contao's template engine then gets run with the blocks prepopulated and will output the result. Commits ------- 0f3c623c add ContaoEscaper that does not double encode input 9cbb7bbc add NodeVisitor that can alter escape filter strategies e28bb7c4 add InteropExtension that allows registering templates that should get processed by the ContaoEscaperNodeVisitor d594574d add our own FilesystemLoader instead of decorating 5eda502a add ContaoTwigTemplateLocator + rewire/register locations in TwigPathsPass compiler pass 5e9b81de delegate rendering to twig if a template surrogate exists 15ebc62b register template for input encoding when rendering via TemplateInheritance c7406f94 fix tests 1cd3c1dc add todo note for theme mapping f0f90e6c check for non-existing directories in ContaoTwigTemplateLocator 784af57d cleanup 3d10b538 add and configure TemplateHierarchy + DynamicExtendsTokenParser 0b3e6b35 handle uppercase entities 31889173 rework and simplify the concept f71b882a reimplement contao resources location 193cf152 fix service declaration 80ee873d make signature compatible to legacy interface 93016d7b fix tests 2fdd8e4f raise symfony/dependency-injection dependency see symfony/symfony#40167 (Definition::removeMethodCall won't remove all calls) 018f7be3 throw if templates are ambiguous (multiple variants in subfolders) b93d098f CS 8db05eea try to find out what's wrong on windows 9ca8e273 fix comments 8f0bd98f canonicalize paths fc2af8a3 fix locator paths b2f0c62f add shortcut to refresh the loader 87d13747 add automatic cache busting 4c2c0fc6 add command to list + refresh the current template hierarchy 12780cc8 fix tests c8474d4f split loader responsibilities c2f1f760 refactor ContaoFilesystemLoader + make it only apply to Contao namespaces cedf1381 make warmer auto refresh in dev mode ec092b61 add and improve tests 91476a7d improve regular expressions 181c099e add more tests 3fed107a add more tests fd330655 add more tests 1ee756b6 hash each individual hierarchy chain 36a9cfb2 also load html5 templates + allow extending them e7f0720c refactor legacy block rendering 9e72dcd5 rework cache busting f65800eb add + adjust tests 30d68e65 CS + path normalizing babf65f3 fix call 36477867 fix version constraints d3cf90eb improve block regex Co-authored-by: Martin Auswöger <martin@auswoeger.com> b9bbcf29 improve + harden block parsing ee73cf23 revert changes to unaffected code e4243c3c improve naming + comments c9707033 remove static keyword from callback function d32aa44e always use the FrontendTemplate class to render legacy templates 6e55565b WIP 7612606c fix filemtime check 8f854fa1 token parsers: improve support for extends/embed + support include 2d26645b add context transformer to support closures c6900312 improve debug command 1e573214 add todo 3b5253fb add tests for DebugContaoTwigCommand 17110299 add + fix tests for contao twig extension 7cef52f4 fix tests 90c4be11 mark classes as final/internal/experimental 23de8dd1 fix tests 3262d9b7 add todo placeholders 39d8b4f5 add tests for ContextHelper 77430e96 fix phpstan errors 9ae391af fix psalm + php7.3 tests b2ea883f simplify + harden twig_include function call 06094279 use named arguments 36255096 ask Twig's CoreExtension for the twig_include callable 39c47615 add and improve tests bf862f51 simplify call de4f44cd improve coverage b84cae35 simplify expression 25a3f187 improve coverage 12a7fc1a fix tests 27ca43d0 ignore coverage for PHP7.3 code 47012daf improve coverage 79c0534d improve coverage cdc553a6 fix test dependency c4142502 fix tests on Windows ec65108a add contao_sections + contao_section methods 544a95b2 small improvement for generated PhpTemplateProxyNode code 9188b03c refactor/unify Twig extensions + runtimes bef9bab9 fix tests 1c9f0199 check for closures instead of callables when transforming the context 9edace83 remove redundant variable bcad3331 remove redundant return statement Co-authored-by: Martin Auswöger <martin@auswoeger.com> 80dcb4fb handle filemtime only generates warnings, no errors 3b4c8771 CS 2c6346f4 CS f65506b0 remove redundant return value
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It would only remove the first match, leaving the other method call(s) there to exist. This leads to unexpected situations.
I found out that
Definition::removeMethodCall
only removes the first method call and stops. It should remove all method calls named$method
.