Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
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
carsonbot commentedFeb 13, 2021
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 changesFeb 14, 2021
It would only remove the first match, leaving the other method call(s) there to exist. This leads to unexpected situations.
Member
fabpot commentedFeb 14, 2021
Thank you@ruudk. |
m-vo added a commit to m-vo/contao that referenced this pull requestMay 14, 2021
seesymfony/symfony#40167(Definition::removeMethodCall won't remove all calls)
m-vo added a commit to m-vo/contao that referenced this pull requestMay 14, 2021
seesymfony/symfony#40167(Definition::removeMethodCall won't remove all calls)
m-vo added a commit to m-vo/contao that referenced this pull requestJul 2, 2021
seesymfony/symfony#40167(Definition::removeMethodCall won't remove all calls)
m-vo added a commit to m-vo/contao that referenced this pull requestJul 2, 2021
seesymfony/symfony#40167(Definition::removeMethodCall won't remove all calls)
m-vo added a commit to m-vo/contao that referenced this pull requestJul 9, 2021
seesymfony/symfony#40167(Definition::removeMethodCall won't remove all calls)
leofeyer pushed a commit to contao/contao that referenced this pull requestJul 14, 2021
Description-----------| Q | A| -----------------| ---| Fixed issues |Fixes#2303 and potentially#2638| Docs PR or issue | todosupersedes#2612 This PR aims at implementing first steps of native Twig support for Contao.# What's inside### EncodingYou now can write Twig templates like so…```twig<h1>{{ headline }}</h1><p>{{ 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.### LoaderTwig 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 folder2. `Contao_Global` for the global templates folder3. `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 resourcesAlso 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.### InheritanceTwig 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 InteropTwig 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 input9cbb7bb add NodeVisitor that can alter escape filter strategiese28bb7c add InteropExtension that allows registering templates that should get processed by the ContaoEscaperNodeVisitord594574 add our own FilesystemLoader instead of decorating5eda502 add ContaoTwigTemplateLocator + rewire/register locations in TwigPathsPass compiler pass5e9b81d delegate rendering to twig if a template surrogate exists15ebc62 register template for input encoding when rendering via TemplateInheritancec7406f9 fix tests1cd3c1d add todo note for theme mappingf0f90e6 check for non-existing directories in ContaoTwigTemplateLocator784af57 cleanup3d10b53 add and configure TemplateHierarchy + DynamicExtendsTokenParser0b3e6b3 handle uppercase entities3188917 rework and simplify the conceptf71b882 reimplement contao resources location193cf15 fix service declaration80ee873 make signature compatible to legacy interface93016d7 fix tests2fdd8e4 raise symfony/dependency-injection dependencyseesymfony/symfony#40167(Definition::removeMethodCall won't remove all calls)018f7be throw if templates are ambiguous (multiple variants in subfolders)b93d098 CS8db05ee try to find out what's wrong on windows9ca8e27 fix comments8f0bd98 canonicalize pathsfc2af8a fix locator pathsb2f0c62 add shortcut to refresh the loader87d1374 add automatic cache busting4c2c0fc add command to list + refresh the current template hierarchy12780cc fix testsc8474d4 split loader responsibilitiesc2f1f76 refactor ContaoFilesystemLoader + make it only apply to Contao namespacescedf138 make warmer auto refresh in dev modeec092b6 add and improve tests91476a7 improve regular expressions181c099 add more tests3fed107 add more testsfd33065 add more tests1ee756b hash each individual hierarchy chain36a9cfb also load html5 templates + allow extending theme7f0720 refactor legacy block rendering9e72dcd rework cache bustingf65800e add + adjust tests30d68e6 CS + path normalizingbabf65f fix call3647786 fix version constraintsd3cf90e improve block regexCo-authored-by: Martin Auswöger <martin@auswoeger.com>b9bbcf2 improve + harden block parsingee73cf2 revert changes to unaffected codee4243c3 improve naming + commentsc970703 remove static keyword from callback functiond32aa44 always use the FrontendTemplate class to render legacy templates6e55565 WIP7612606 fix filemtime check8f854fa token parsers: improve support for extends/embed + support include2d26645 add context transformer to support closuresc690031 improve debug command1e57321 add todo3b5253f add tests for DebugContaoTwigCommand1711029 add + fix tests for contao twig extension7cef52f fix tests90c4be1 mark classes as final/internal/experimental23de8dd fix tests3262d9b add todo placeholders39d8b4f add tests for ContextHelper77430e9 fix phpstan errors9ae391a fix psalm + php7.3 testsb2ea883 simplify + harden twig_include function call0609427 use named arguments3625509 ask Twig's CoreExtension for the twig_include callable39c4761 add and improve testsbf862f5 simplify callde4f44c improve coverageb84cae3 simplify expression25a3f18 improve coverage12a7fc1 fix tests27ca43d ignore coverage for PHP7.3 code47012da improve coverage79c0534 improve coveragecdc553a fix test dependencyc414250 fix tests on Windowsec65108 add contao_sections + contao_section methods544a95b small improvement for generated PhpTemplateProxyNode code9188b03 refactor/unify Twig extensions + runtimesbef9bab fix tests1c9f019 check for closures instead of callables when transforming the context9edace8 remove redundant variablebcad333 remove redundant return statementCo-authored-by: Martin Auswöger <martin@auswoeger.com>80dcb4f handle filemtime only generates warnings, no errors3b4c877 CS2c6346f CSf65506b remove redundant return value
leofeyer pushed a commit to contao/core-bundle that referenced this pull requestJul 14, 2021
Description-----------| Q | A| -----------------| ---| Fixed issues | Fixes #2303 and potentially #2638| Docs PR or issue | todosupersedes #2612 This PR aims at implementing first steps of native Twig support for Contao.# What's inside### EncodingYou now can write Twig templates like so…```twig<h1>{{ headline }}</h1><p>{{ 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.### LoaderTwig 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 folder2. `Contao_Global` for the global templates folder3. `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 resourcesAlso 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.### InheritanceTwig 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 InteropTwig 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 input9cbb7bbc add NodeVisitor that can alter escape filter strategiese28bb7c4 add InteropExtension that allows registering templates that should get processed by the ContaoEscaperNodeVisitord594574d add our own FilesystemLoader instead of decorating5eda502a add ContaoTwigTemplateLocator + rewire/register locations in TwigPathsPass compiler pass5e9b81de delegate rendering to twig if a template surrogate exists15ebc62b register template for input encoding when rendering via TemplateInheritancec7406f94 fix tests1cd3c1dc add todo note for theme mappingf0f90e6c check for non-existing directories in ContaoTwigTemplateLocator784af57d cleanup3d10b538 add and configure TemplateHierarchy + DynamicExtendsTokenParser0b3e6b35 handle uppercase entities31889173 rework and simplify the conceptf71b882a reimplement contao resources location193cf152 fix service declaration80ee873d make signature compatible to legacy interface93016d7b fix tests2fdd8e4f raise symfony/dependency-injection dependencyseesymfony/symfony#40167(Definition::removeMethodCall won't remove all calls)018f7be3 throw if templates are ambiguous (multiple variants in subfolders)b93d098f CS8db05eea try to find out what's wrong on windows9ca8e273 fix comments8f0bd98f canonicalize pathsfc2af8a3 fix locator pathsb2f0c62f add shortcut to refresh the loader87d13747 add automatic cache busting4c2c0fc6 add command to list + refresh the current template hierarchy12780cc8 fix testsc8474d4f split loader responsibilitiesc2f1f760 refactor ContaoFilesystemLoader + make it only apply to Contao namespacescedf1381 make warmer auto refresh in dev modeec092b61 add and improve tests91476a7d improve regular expressions181c099e add more tests3fed107a add more testsfd330655 add more tests1ee756b6 hash each individual hierarchy chain36a9cfb2 also load html5 templates + allow extending theme7f0720c refactor legacy block rendering9e72dcd5 rework cache bustingf65800eb add + adjust tests30d68e65 CS + path normalizingbabf65f3 fix call36477867 fix version constraintsd3cf90eb improve block regexCo-authored-by: Martin Auswöger <martin@auswoeger.com>b9bbcf29 improve + harden block parsingee73cf23 revert changes to unaffected codee4243c3c improve naming + commentsc9707033 remove static keyword from callback functiond32aa44e always use the FrontendTemplate class to render legacy templates6e55565b WIP7612606c fix filemtime check8f854fa1 token parsers: improve support for extends/embed + support include2d26645b add context transformer to support closuresc6900312 improve debug command1e573214 add todo3b5253fb add tests for DebugContaoTwigCommand17110299 add + fix tests for contao twig extension7cef52f4 fix tests90c4be11 mark classes as final/internal/experimental23de8dd1 fix tests3262d9b7 add todo placeholders39d8b4f5 add tests for ContextHelper77430e96 fix phpstan errors9ae391af fix psalm + php7.3 testsb2ea883f simplify + harden twig_include function call06094279 use named arguments36255096 ask Twig's CoreExtension for the twig_include callable39c47615 add and improve testsbf862f51 simplify callde4f44cd improve coverageb84cae35 simplify expression25a3f187 improve coverage12a7fc1a fix tests27ca43d0 ignore coverage for PHP7.3 code47012daf improve coverage79c0534d improve coveragecdc553a6 fix test dependencyc4142502 fix tests on Windowsec65108a add contao_sections + contao_section methods544a95b2 small improvement for generated PhpTemplateProxyNode code9188b03c refactor/unify Twig extensions + runtimesbef9bab9 fix tests1c9f0199 check for closures instead of callables when transforming the context9edace83 remove redundant variablebcad3331 remove redundant return statementCo-authored-by: Martin Auswöger <martin@auswoeger.com>80dcb4fb handle filemtime only generates warnings, no errors3b4c8771 CS2c6346f4 CSf65506b0 remove redundant return valueSign up for freeto 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::removeMethodCallonly removes the first method call and stops. It should remove all method calls named$method.