Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[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
merged 1 commit into from
Feb 14, 2021

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Feb 12, 2021

It would only remove the first match, leaving the other method call(s) there to exist. This leads to unexpected situations.

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets None
License MIT
Doc PR

I found out that Definition::removeMethodCall only removes the first method call and stops. It should remove all method calls named $method.

@xabbuh xabbuh added this to the 4.4 milestone Feb 12, 2021
@carsonbot carsonbot changed the title Definition::removeMethodCall should remove all matching calls [DependencyInjection] Definition::removeMethodCall should remove all matching calls Feb 12, 2021
@carsonbot
Copy link

Hey!

Excellent, keep up the good work.

I think @atailouloute has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

It would only remove the first match, leaving the other method call(s) there to exist. This leads to unexpected situations.
@fabpot fabpot changed the base branch from 5.x to 4.4 February 14, 2021 11:22
@fabpot fabpot force-pushed the remove-method-call branch from a2ae93d to 944ba23 Compare February 14, 2021 11:22
@fabpot
Copy link
Member

fabpot commented Feb 14, 2021

Thank you @ruudk.

@fabpot fabpot merged commit aad1d09 into symfony:4.4 Feb 14, 2021
@ruudk ruudk deleted the remove-method-call branch February 15, 2021 08:37
This was referenced Mar 4, 2021
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 &copy; @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 &copy; @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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.