Migrate to Twig ~3.0

[filcius]

• Add global variables from View::shared
• Remove Laravel events

[filcius]

• Unit tests that test commands are waiting for user's input stale indefinitely.
• I don't know how to attribute another version. This PR include breaking changes, so if we want to follow semver, we should increase the first number.

[coveralls]

Coverage increased (+0.5%) to 60.34% when pulling eaa494d12f1d51475e6795cb3cfb9cac02579fa4 on camellia-sinensis-it:twig3 into 5d6dc0c907c5db476cf0caf3210eb10e44a78369 on rcrowe:master.

[barryvdh]

So it's no longer possible to fire those events?

[filcius]

It is no longer possible to use a custom Template class in Twig3. Twigbridge's custom Template was firing those events. To think of it, it could be possible if we copy paste a bit of code from Twig, but we risk making it harder to update Twig in the future.

[filcius]

For your information, I will start using this proposed branch in production next week. It could be wise to wait a little before approving this request and see if I encounter any bugs.

[nVuln]

we can make an extension (node visitor) to re-enable events feature

[filcius]

we can make an extension (node visitor) to re-enable events feature

Are you sure of that? I searched for occurrences of "traverse" in Twig's code and it seems to be called only in the Parser, not in the Template parent class.

[filcius]

However, this example look promising. It seems to add a non-displayed node in the AST:

$node->setNode('display_start', new Node([new EnterProfileNode($this->extensionName, Profile::TEMPLATE, $node->getTemplateName(), $varName), $node->getNode('display_start')]));

Source: https://github.com/twigphp/Twig/blob/6b2e6e6d05a7511c2cac4636b5e3179b110622c1/src/Profiler/NodeVisitor/ProfilerNodeVisitor.php#L46

[nVuln]

we can make an extension (node visitor) to re-enable events feature

Are you sure of that? I searched for occurrences of "traverse" in Twig's code and it seems to be called only in the Parser, not in the Template parent class.

yup, we cannot made a change in Template parent class, but we can inject a line of code to call composer event in Parser add a "event" Node to display_start in ModuleNode Source: https://github.com/twigphp/Twig/blob/3.x/src/Node/ModuleNode.php#L309

[nVuln]

I double checked, we cannot change or make any compile in display_start node because it's can be trouble in sandbox mode Source: https://github.com/twigphp/Twig/blob/3.x/src/NodeVisitor/SandboxNodeVisitor.php#L102 so I changed body node of ModuleNode, like this:

if (!$node instanceof ModuleNode) {
       return $node;
}

$newBodyNode = new Node([new EventNode(), $node->getNode('body')]);
$node->setNode('body', $newBodyNode);

with EventNode is custom node to append event caller

[filcius]

I double checked, we cannot change or make any compile in display_start node because it's can be trouble in sandbox mode Source: https://github.com/twigphp/Twig/blob/3.x/src/NodeVisitor/SandboxNodeVisitor.php#L102 so I changed body node of ModuleNode, like this:

if (!$node instanceof ModuleNode) {
       return $node;
}

$newBodyNode = new Node([new EventNode(), $node->getNode('body')]);
$node->setNode('body', $newBodyNode);

with EventNode is custom node to append event caller

I think we should test it with adding a node to display_start, because it is the recommended way in Twig's comments: "consider adding nodes to the following nodes: display_start, display_end, constructor_start, constructor_end, and class_end".

I don't see why SandboxNodeVisitor would have an impact. Is it running the code from display_start twice?!

[nVuln]

Is it running the code from display_start twice?!

yup, on Sandbox mode it will compile once in constructor_end, at this time I don't know how to ignore it, hope you can

[filcius]

Is it running the code from display_start twice?!

yup, on Sandbox mode it will compile once in constructor_end, at this time I don't know how to ignore it, hope you can

We could add a $__twigBridge_XXX attribute flag to the template class to make sure the events are not called more than once. Anyhow, thanks for the tip, I will make sure to test both in Sandbox mode and not.

[filcius]

I made some unit tests tonight (they are not pushed yet).

Events can modify the data in the view context. It works for the parent template, an included template and an extended template so far. I still have to test it with the Sandbox mode mentioned above.

These tests are not in pushed yet. I will see if I can make a separate PR for those tests, in order to execute them using the TwigBridge prior to the Twig3 migration.

[bytestream]

Take a look at https://github.com/rcrowe/TwigBridge/pull/369/files for an example of a node visitor.

[filcius]

Take a look at https://github.com/rcrowe/TwigBridge/pull/369/files for an example of a node visitor.

I have a version with Events support, but I am waiting for this Pull Request to be merged to have a baseline on which to make my tests : https://github.com/rcrowe/TwigBridge/pull/395

[filcius]

Take a look at https://github.com/rcrowe/TwigBridge/pull/369/files for an example of a node visitor.

I have a version with Events support, but I am waiting for this Pull Request to be merged to have a baseline on which to make my tests : #395

It has a "EventCaller implements NodeVisitorInterface" which inject a "LaravelEventsNode" that inject php code in the compiled template to call Laravel's View events

[bytestream]

Tests need fixing before it can be merged.

I would also think it's best to do anything to get Twig 3 to work in this PR rather than delete a key feature and add it back later. But @barryvdh opinion would be best on this.

[filcius]

I would also think it's best to do anything to get Twig 3 to work in this PR rather than delete a key feature and add it back later.

My opinion indeed. Fixes to add Events are on a shelf on my computer right now, but I need the other PR with Unit tests on Laravel events to be merged first to prove that my Twig3 implementation is compatible.

[filcius]

Event support is there again.

I had to make a weird check in LaravelEventNodeVisitor to detect Embedded nodes and don't call the Events twice

[filcius]

Twig 3 support "php": ">=7.2.5",

So, this means that twigBridge has to follow and the travis job for php7.1 is not necessary anymore

[filcius]

Could be a good idea to add tests on php7.4. I tried, but they failed : https://travis-ci.org/github/rcrowe/TwigBridge/builds/717397164

It was not purpose of this PR, so I only kept php7.2 and 7.3 tests

[kblais]

Hi there !

Thanks for your work on this library I use since I started using Laravel 6 years ago.

Yet, I'd like to know if there are any plans to merge Twig 3.* compatibility in the near future ? @filcius did you manage to use this branch in production, and did it work well ?

I'd love to help, but I don't really have a good knowledge of Twig's internal's...

[filcius]

@kblais It is working perfectly fine for me in production. I am not using Laravel View events in my production code. All unit tests I added to the master version of twigbridge are working perfectly in both the master branch and this branch.

[kblais]

@filcius thanks for your reply !

Well, then maybe we could merge this ! @barryvdh what do you think about it ?

[barryvdh]

Sure, unfortunately merge conflict though

[filcius]

Sure, unfortunately merge conflict though

@barryvdh Conflicts are now resolved.

[kblais]

Nice ! Thanks @barryvdh @filcius :)

[kblais]

@filcius @barryvdh found an issue with view composers, will create an issue

EDIT : found out it was about missing extensions, still found an error in the upgrade guide

[filcius]

@filcius @barryvdh found an issue with view composers, will create an issue

EDIT : found out it was about missing extensions, still found an error in the upgrade guide

If there is something missing in the upgrade guide, it may be easily addressed. I will gladly fix it if you tell me what you think is missing.

[kblais]

@filcius addressed it in #410 ;)

[barryvdh]

This will be released as 0.13.x soon. Kept the 0.12 branch for legacy Twig versions.