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

Require Twig 2.x #7629

Merged
merged 8 commits into from Aug 10, 2017

Conversation

Projects
None yet
5 participants
@mbabker
Contributor

mbabker commented Feb 27, 2017

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Related tickets N/A
License MIT

From what I can see, there isn't anything that shouldn't work with Twig 2.x at this point. So, allow it.

@gabiudrescu

This comment has been minimized.

Show comment
Hide comment
@gabiudrescu

gabiudrescu Mar 7, 2017

Contributor

@mbabker can you run a composer update locally so we can have a composer.lock according to what we have in composer.json?

the testing suite from Travis CI seems to complain about that.

Contributor

gabiudrescu commented Mar 7, 2017

@mbabker can you run a composer update locally so we can have a composer.lock according to what we have in composer.json?

the testing suite from Travis CI seems to complain about that.

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Mar 7, 2017

Member

@gabiudrescu 👍, composer update --lock would do the job.

Member

pamil commented Mar 7, 2017

@gabiudrescu 👍, composer update --lock would do the job.

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Mar 7, 2017

Contributor

Is it going to be a problem if I add the config.platform.php key in the composer.json file? I don't have PHP 5.6 installed locally anymore and when I ran the update before sending this PR it installed Twig 2.x (which isn't PHP 5.6 compatible).

Contributor

mbabker commented Mar 7, 2017

Is it going to be a problem if I add the config.platform.php key in the composer.json file? I don't have PHP 5.6 installed locally anymore and when I ran the update before sending this PR it installed Twig 2.x (which isn't PHP 5.6 compatible).

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Mar 7, 2017

Contributor

Nevermind, the --lock flag did the trick. Should be good now.

Contributor

mbabker commented Mar 7, 2017

Nevermind, the --lock flag did the trick. Should be good now.

@gabiudrescu

This comment has been minimized.

Show comment
Hide comment
@gabiudrescu

gabiudrescu Mar 7, 2017

Contributor

so basically, using Twig 2.x in Sylius makes it un-usable to PHP 5.6 ?

Contributor

gabiudrescu commented Mar 7, 2017

so basically, using Twig 2.x in Sylius makes it un-usable to PHP 5.6 ?

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Mar 7, 2017

Contributor

No, running just a normal composer update on a PHP 7 platform would set the composer.lock file to a set of dependencies only usable by PHP 7. So the update either has to be done on a PHP 5.6 platform or the config.platform.php key needs to be set.

But that could be semi problematic if the Travis build matrix were expanded to run a test pass that uses Twig 1.x on PHP 5.6 and Twig 2.x on PHP 7. You'd in essence need two lock files and would have to swap them depending on the version in use.

Contributor

mbabker commented Mar 7, 2017

No, running just a normal composer update on a PHP 7 platform would set the composer.lock file to a set of dependencies only usable by PHP 7. So the update either has to be done on a PHP 5.6 platform or the config.platform.php key needs to be set.

But that could be semi problematic if the Travis build matrix were expanded to run a test pass that uses Twig 1.x on PHP 5.6 and Twig 2.x on PHP 7. You'd in essence need two lock files and would have to swap them depending on the version in use.

@pamil

Rebase needed 🥇

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Mar 16, 2017

Contributor

Rebased

Contributor

mbabker commented Mar 16, 2017

Rebased

@pjedrzejewski pjedrzejewski added this to the v1.0.0-beta.2 milestone Mar 17, 2017

@pjedrzejewski pjedrzejewski reopened this Mar 20, 2017

@pjedrzejewski pjedrzejewski removed this from the v1.0.0-beta.2 milestone Mar 28, 2017

@pjedrzejewski

This comment has been minimized.

Show comment
Hide comment
@pjedrzejewski

pjedrzejewski Mar 28, 2017

Member

We have discussed this with @pamil and decided to hold it, before we figure out a simple way to test it with both versions. Allowing this now and reverting would be a BC break, other way around no.

Member

pjedrzejewski commented Mar 28, 2017

We have discussed this with @pamil and decided to hold it, before we figure out a simple way to test it with both versions. Allowing this now and reverting would be a BC break, other way around no.

@leogout

This comment has been minimized.

Show comment
Hide comment
@leogout

leogout Apr 8, 2017

Hi, I've tried to upgrade to 2.0 and I ran into some troubles.
As you can see on this Twig issue, macros cannot be accessed in children anymore.
I tested from a new Sylius install (following the recommendations), by updating "twig/twig": "^2.0" in the composer.json and by running composer update.
Now the @SyliusAdmin/Crud/[ Create | Update | Index ]/_headerTitle.html.twig are producing this error:

twig-macro-error

Which can be resolved by importing the macros directly in the _headerTitle.html.twig

leogout commented Apr 8, 2017

Hi, I've tried to upgrade to 2.0 and I ran into some troubles.
As you can see on this Twig issue, macros cannot be accessed in children anymore.
I tested from a new Sylius install (following the recommendations), by updating "twig/twig": "^2.0" in the composer.json and by running composer update.
Now the @SyliusAdmin/Crud/[ Create | Update | Index ]/_headerTitle.html.twig are producing this error:

twig-macro-error

Which can be resolved by importing the macros directly in the _headerTitle.html.twig

@mbabker mbabker referenced this pull request May 9, 2017

Closed

Support for Twig 2.x #8029

@pamil pamil dismissed their stale review May 18, 2017

No longer relevant.

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jun 19, 2017

Contributor

I've got a little time on my hands today, so I'm working to update this to address some of the other B/C concerns that are in the layout files (everything I've done so far is in PHP code).

The testing issue is fixed with this commit so that should no longer be a blocker.

Contributor

mbabker commented Jun 19, 2017

I've got a little time on my hands today, so I'm working to update this to address some of the other B/C concerns that are in the layout files (everything I've done so far is in PHP code).

The testing issue is fixed with this commit so that should no longer be a blocker.

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jun 19, 2017

Contributor

This is almost good now. Seems I'm down to one consistent issue:

  • There's a circular dependency issue with the path "sylius.grid.renderer.twig -> twig -> sylius.templating.helper.grid -> sylius.grid.renderer.twig", it's always been there honestly so I'm not sure what this PR is doing to expose it now and inherently cause the CI failure

Just a general note, when the PHP < 7.1 support is dropped, this PR will need updated to run two CI passes still, one for each Twig major version (right now I'm doing Twig 1 on 5.6 and Twig 2 on 7.1).

Contributor

mbabker commented Jun 19, 2017

This is almost good now. Seems I'm down to one consistent issue:

  • There's a circular dependency issue with the path "sylius.grid.renderer.twig -> twig -> sylius.templating.helper.grid -> sylius.grid.renderer.twig", it's always been there honestly so I'm not sure what this PR is doing to expose it now and inherently cause the CI failure

Just a general note, when the PHP < 7.1 support is dropped, this PR will need updated to run two CI passes still, one for each Twig major version (right now I'm doing Twig 1 on 5.6 and Twig 2 on 7.1).

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Jul 19, 2017

Member

What do you think about bumping requirements to Twig 2 after we drop PHP 5.6 support? It will make it supported without bringing the cost to maintain both versions (and it should be easy enough to update).

Member

pamil commented Jul 19, 2017

What do you think about bumping requirements to Twig 2 after we drop PHP 5.6 support? It will make it supported without bringing the cost to maintain both versions (and it should be easy enough to update).

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jul 19, 2017

Contributor

Personally I don't see a problem with it. The only potential pain point is the Symfony bundles in the ecosystem which don't provide Twig 2 support yet (and I can't say I've run into one yet).

Contributor

mbabker commented Jul 19, 2017

Personally I don't see a problem with it. The only potential pain point is the Symfony bundles in the ecosystem which don't provide Twig 2 support yet (and I can't say I've run into one yet).

@pjedrzejewski pjedrzejewski added this to the v1.0.0 milestone Jul 19, 2017

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Jul 30, 2017

Member

Thanks for being patient in spite of our constantly changing plans for this PR :) Since we made it to PHP 7.1 now, we can drop Twig 1 and made the stable release use Twig 2 only.

Member

pamil commented Jul 30, 2017

Thanks for being patient in spite of our constantly changing plans for this PR :) Since we made it to PHP 7.1 now, we can drop Twig 1 and made the stable release use Twig 2 only.

@mbabker mbabker changed the title from Allow Twig 2.x to Require Twig 2.x Jul 30, 2017

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jul 30, 2017

Contributor

Should be good now.

Contributor

mbabker commented Jul 30, 2017

Should be good now.

@pamil

pamil approved these changes Jul 31, 2017

@pamil pamil requested a review from pjedrzejewski Jul 31, 2017

@alcaeus alcaeus referenced this pull request Aug 2, 2017

Closed

Allow installing Twig 2.0 #8364

@pjedrzejewski

This comment has been minimized.

Show comment
Hide comment
@pjedrzejewski

pjedrzejewski Aug 8, 2017

Member

@mbabker Awesome work on this, we really appreciate your time. Could you update the lock file? There are some conflicts, I will keep an extra eye 👀 on this PR and merge it as soon as its green (go ahead @Sylius/core-team if you are first) to avoid further conflicts.

Member

pjedrzejewski commented Aug 8, 2017

@mbabker Awesome work on this, we really appreciate your time. Could you update the lock file? There are some conflicts, I will keep an extra eye 👀 on this PR and merge it as soon as its green (go ahead @Sylius/core-team if you are first) to avoid further conflicts.

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Aug 10, 2017

Contributor

Rebased!

Contributor

mbabker commented Aug 10, 2017

Rebased!

@pamil pamil merged commit 33cb78a into Sylius:master Aug 10, 2017

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Aug 10, 2017

Member

Thanks Michael! 🎉

Member

pamil commented Aug 10, 2017

Thanks Michael! 🎉

@pjedrzejewski

This comment has been minimized.

Show comment
Hide comment
@pjedrzejewski
Member

pjedrzejewski commented Aug 10, 2017

🍺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment