Support for twig 3.9

[diffy0712]

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌
Fixed issues	#153

Twig 3.9 introduced a few internal changes that this package relied on:

1. The internal function twig_get_attribute was moved and renamed to CoreExtension::getAttribute.
   See discussion about moving functions to internal in twig:3.9 here.
2. The internals of Twig does not store the html content in the output buffer anymore (but it still reads from it). This means when this package called the View::endPage function to replace the placeholders it failed to do so, because the ob was empty.

[schmunk42]

This might mitigate the problem and should be released as 2.4.3, but after that we can remove the if and should lift the constraint to "twig/twig": "~3.9" and release a 2.5.0.

As an additional upgrading notice we could add an information that you have to use something like conflict: twig/twig>=3.9 when using yii2-twig <= 2.4.2

[diffy0712]

This patch broke one test case so I still have to look at that.

After that I can send another pr for the 2.5 release

[schmunk42]

As far as I can tell your patch did not break things. It's rather another side-effect of twig 3.9, but I have no idea what exactly, the test does not find a script tag from the asset bundle.

See https://github.com/schmunk42/yii2-twig/actions/runs/8799762201 - which just has this additional commit applied schmunk42@f2bb14b (prevent installation of twig 3.9 and above) - test are passing.

I'd suggest to add this to the PR and take care about the actual problem later.

CC: @samdark @bizley @rob006

[diffy0712]

Yes, I have looked at it, but did not yet found the source of the problem.
What I've found is that they introduced the 'use_yield' option (false by default), which they mention in the changelog. The cause might come from the fact that they have rewrote some of the internals that uses ob_ functions.

It seems that the yii2 view helpers(end_body, start_body etc...) twig extensions are broken in twig3.9 for some reason.

see commit twigphp/Twig@3b6cbf98d

I agree, that first there should be a conflicting composer patch, so no problematic updates happens.

[diffy0712]

This pr already contains a partial fix for 3.9. Do you think there should be another pr for just the conflict or use this and open another with the later actual fix?

[schmunk42]

This pr already contains a partial fix for 3.9. Do you think there should be another pr for just the conflict or use this and open another with the later actual fix?

See #155

[diffy0712]

It seems that yii2 view helper functions like 'begin_body' 'begin_page' etc. will mark the parts of the page with '' comments and in the last 'end_page' view method call will read 'ob_get_clean()' and replace these blocks with the tags that needs to be placed there. (in case of the failing test it is 'jquery.js' file.

For some reason this does not happen with twig 3.9.
I see that the ob_get_clean returns an empty string instead of the html of the view with the inserted tags.

[eluhr]

That's the same thing I found. At some point the output buffer is emptied where it shouldn't be. At least that would be my first guess.

[diffy0712]

I think the ob clear happens here.

https://github.com/twigphp/Twig/blob/3.x/src/Template.php#L362
and since the function does not echo anything to the buffer it will stay empty.

[eluhr]

Yup, looks like that‘s the spot

[diffy0712]

It seems that they now only capture and clear whats in the ob but not echoing anymore, so it seems to me that we are unable to change the html in the buffer.

[diffy0712]

I think this issue is related to our problem. twigphp/Twig#4034

[eluhr]

If I'm interpreting the code correctly, the twig guys are turning everything completely inside out, at least when it comes to the compilation of twig.

An error is explicitly triggered here if “echo” is used:

• https://github.com/twigphp/Twig/blob/3.x/src/Compiler.php#L78
• https://github.com/twigphp/Twig/blob/3.x/src/Compiler.php#L103

I don't see an easy way to get this running with twig version 3.9.0 and above without having to touch the view component.

[diffy0712]

If I understood correctly, they are preparing the code for v4 when it will only use the new yield with no ob buffer.

Maybe if we could somehow access and modify the twig content from the TwigFunction, then we could use ob inside the viewHelper to capture the yii helper's echo, in case of end_body the ob would need to have the twig content so the function can replace the custom placeholders.

Or create a custom View as u said, but I think that would be a bit inconvenient and breaking change.

[eluhr]

I'm currently trying out exactly what you're suggesting.

and as you say, I wouldn't see a custom view component as an option either

[eluhr]

That is my current approach. I try to use the existing viewHelper method and the EVENT_AFTER_RENDER event of the view component to get the rendered output and write it to the output buffer:

public function viewHelper($context, $name = null)
{
    if ($name !== null && isset($context['this'])) {
        if ($name == 'end_page') {
            // tapping into the after render event of the view component to load the rendered content into the output buffer
            Event::on($context['this']::class, View::EVENT_AFTER_RENDER, function ($event) {
                // TODO: Is loading stuff in output buffer the way?
                // $output = $event->output;
            });
        }
        $this->call($context['this'], Inflector::variablize($name));
    }
}

[diffy0712]

Were you able to get it working?

I can think of another solution(to work without yield and use the current yii views)
As far as I know, the only yii view method that actually modifies the html content (in the ob) is the end_body, which should basically always happen at the end of the twig template if it is called ofc.
So if we add a flag 'end_body_enabled', and instead of running the endBody view method in the twig function once it is called, we could just enable this flag, so when the ViewRenderer render (

yii2-twig/src/ViewRenderer.php

Line 179 in 0ed174a

return $this->twig->render(pathinfo($file, PATHINFO_BASENAME), $params);

) method renders the page 'using twig' as alway, but with the fleg enabled, capture the variable, put it in ob and call View::endPage so it can replace the placeholders and return that from the render method.

I think this can work, but I can only test it tomorrow.

[diffy0712]

Just to sum up the problem that is braking the test (as I understand it)
This package until now, relies on the fact that the twig engine stored the output html in the output buffer (ob), so the yii2 view could echo into it and could modify the whole content (replace all the yii placeholder comments with assets and stuff).
but since twig 3.9 this internal behaviour changed, and they now store the content in a new internal variable (uses yielding). https://github.com/twigphp/Twig/blob/4a7de4ab5603ad8f199222cae3951fc271a8c45d/src/Template.php#L366

So this package was modifiing twig's global variable(the ob), which it has no access to modify(only append using echo) since twig:3.9.

[diffy0712]

I can think of another solution(to work without yield and use the current yii views) As far as I know, the only yii view method that actually modifies the html content (in the ob) is the end_body, which should basically always happen at the end of the twig template if it is called ofc. So if we add a flag 'end_body_enabled', and instead of running the endBody view method in the twig function once it is called, we could just enable this flag, so when the ViewRenderer render (

yii2-twig/src/ViewRenderer.php

Line 179 in 0ed174a

return $this->twig->render(pathinfo($file, PATHINFO_BASENAME), $params);

) method renders the page 'using twig' as alway, but with the fleg enabled, capture the variable, put it in ob and call View::endPage so it can replace the placeholders and return that from the render method.

This seems to be working correctly! I will push the changes soon.

[diffy0712]

@schmunk42 @eluhr I think this workaround should work with twig 3.9 version as well! Could you please check it out?

I think the best solution would be to support use_yield as it will be the only option in twig 4.0, but still I do not see any good solution for it without a breaking change in this library.

ps: This relies on the fact that the only yii2 View method that modifies the output buffer is the endPage. I've checked and it seems to be true, but this needs another check for sure

update: if you dont think we need to support older twig versions, I could remove the version checks and update composer.json to conflict with older versions. That is up to you, which would be the better choice for the project :)

[diffy0712]

I do not think this is the best way to check the version. Do anyone have a better idea?

[eluhr]

Can't think of a better way to check this and since the non-existence of the function is the source of our current problems, I think this is actually a pretty good solution.

[eluhr]

@diffy0712 with your current patch i am getting the following error: ob_clean(): Failed to delete buffer. No buffer to delete wrapping the ob_clean call in the ViewRenderer into an if condition like this does the trick (for me):

  if ($content !== false) {
      ob_clean();
  }

But even after that fix i get an HeadersAlreadySentException which is caused by the echo $content in the ViewRenderer. But maybe thats a problem from my current setup.

What is your way for recreating the bug? I tested it by using using the layout.twig from the tests as my web application layout

 'layout' => '@vendor/yiisoft/yii2-twig/tests/views/layout.twig'

[schmunk42]

@yiisoft/core-developers Given these esoteric issues, I'd even think about releasing a new major version of yii2-twig and require ^3.9.0.

Because twig can be configured with several plugins, custom functions, probably usage of "protected" functions in your templates and so on. It looks highly likely to me that upgrading to twig/twig:3.9 will raise a lot of errors in existing applications.

[diffy0712]

@diffy0712 with your current patch i am getting the following error: ob_clean(): Failed to delete buffer. No buffer to delete wrapping the ob_clean call in the ViewRenderer into an if condition like this does the trick (for me):

  if ($content !== false) {
      ob_clean();
  }

But even after that fix i get an HeadersAlreadySentException which is caused by the echo $content in the ViewRenderer. But maybe thats a problem from my current setup.

What is your way for recreating the bug? I tested it by using using the layout.twig from the tests as my web application layout

 'layout' => '@vendor/yiisoft/yii2-twig/tests/views/layout.twig'

I've done the fix by running the unit tests. It seems that I have to check out in my project as well, because it seems that there is some problems with the buffer part of the code.

[diffy0712]

@yiisoft/core-developers Given these esoteric issues, I'd even think about releasing a new major version of yii2-twig and require ^3.9.0.

Because twig can be configured with several plugins, custom functions, probably usage of "protected" functions in your templates and so on. It looks highly likely to me that upgrading to twig/twig:3.9 will raise a lot of errors in existing applications.

Yes that is true, the same issue that the end_page twig function failed with twig:3.9 it can happen in anyone's project using yii2-twig.
Releasing as a major version it would at least "warn" people when upgrading, and it should be stated in the Changelog as well with detailed explanation on the possible issues.

In this case I would remove the old version support from this pr and update the compose.json as well.

[schmunk42]

In this case I would remove the old version support from this pr and update the compose.json as well.

@diffy0712 Yes please, I think we should make a hard cut.

@samdark I don't want to be annoying, but is there something preventing you from creating the 2.4.3 release?
Currently we have to manually add a twig contraint to 3.8.0, which we need to remove later on.

[diffy0712]

@eluhr I've fixed the render output buffer issue. Could you please check if it is working for you as well?
It was an issue that the $view->endPage(); method did close the output buffer, which caused a problem. I have tested it with the unit tests, which apparently has another output buffer layer started so it caused some problem :D
Hope it is working correctly now.

[diffy0712]

@schmunk42 I've removed the support for twig below 3.9 and added the updated composer version dependency for twig to 3.9.
Do you think that is enough or should I add something else?

[diffy0712]

I still need to add description to the Changelog for the new release to explain this change and the situation with > twig 3.9 better. I will try to do it tomorrow.

[samdark]

@schmunk42 not really. Just very busy.

[eluhr]

@eluhr I've fixed the render output buffer issue. Could you please check if it is working for you as well?

@diffy0712 LGTM!

[schmunk42]

I also ran our phd5 tests against this patch - all green 👍

So I'd revise my statement about a new major version :)
But at least there needs to be a new minor version with information about the deprecation of internal twig functions.

[diffy0712]

Great news, thank you.
Is it ok to modify the minimum twig dependency version in a minor release?

I think the twig internal change that might affect others the most is the fact that it now does not store the contents in the output buffer. So if others relied on this and modified the output buffer in twig function calls (like yii2-twig did with view->endPage twigFunction) it will break with twig 3.9. Twig with 'use_yield=false' will still read the output buffer as expected in twigFunction calls.
I have to note that twig4 will remove the ob support completely according to their changelog, which this version does not support at all. I am not yet sure how to support yieling, but that is not needed yet.

I try to update the changelog today.

[samdark]

@schmunk42 so do we need to release from master before merging this PR?

[schmunk42]

Yes, I’d recommend to release current master as 2.4.3 And the PR as 2.5.0 or 3.0.0

[diffy0712]

I've updated the changelog as well. I think this pr is ready for review.

[diffy0712]

@schmunk42 how to continue with this?

[schmunk42]

@schmunk42 how to continue with this?

I think it should be merged and released as 2.5.0.

[samdark]

@schmunk42 done. Thanks, @diffy0712

[schmunk42]

@samdark Don't forget to release ;) https://github.com/yiisoft/yii2-twig/tags