Skip to content
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

Make Twig Bridge compatible with Twig 3.0 #236

Merged
merged 2 commits into from
Nov 27, 2019

Conversation

mhujer
Copy link
Contributor

@mhujer mhujer commented Nov 24, 2019

Twig provides namespaced class aliases since 1.38.1 and 2.12.1, so this should work on all recently released Twig versions.

composer.json Outdated
@@ -38,6 +38,9 @@
"zendframework/zend-servicemanager": "~2.2",
"zendframework/zend-view": "~2.2"
},
"conflict": {
"twig/twig": "<1.38.1||<2.12.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ensures that the library is not installed along with an incompatible Twig version.

@mhujer mhujer force-pushed the twig-3.0-compatibility branch from 0353310 to ec55ba1 Compare November 24, 2019 08:37
@@ -23,7 +24,7 @@
* @copyright 2012-2015 Florian Eckerstorfer
* @license http://www.opensource.org/licenses/MIT The MIT License
*/
class SlugifyExtension extends \Twig_Extension
class SlugifyExtension extends AbstractExtension
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non-namespaced classes were removed in 3.0

@mhujer mhujer force-pushed the twig-3.0-compatibility branch 2 times, most recently from 68df2ea to 782bc49 Compare November 24, 2019 08:59
@mhujer mhujer mentioned this pull request Nov 24, 2019
@florianeckerstorfer
Copy link
Member

Awesome, could you update the PR with the latest changes from master and see if the tests pass?

@mhujer mhujer force-pushed the twig-3.0-compatibility branch 4 times, most recently from 9be1983 to e76d76d Compare November 26, 2019 18:17
@mhujer
Copy link
Contributor Author

mhujer commented Nov 26, 2019

@florianeckerstorfer rebased, but the build is failing for <PHP7.0 probably because of the conflict rule. I spent some time trying to figure it out, but without success.

Either the conflict rule can be removed, or the support for long-dead PHP versions can be dropped.

@kubawerlos
Copy link
Contributor

@mhujer the conflict you have added - <1.38.1||<2.12.1 works like this:

  • Hey, I am Twig 1.39, can I be installed?
  • Let's check: 1.39 < 1.38.1 || 1.39 < 2.12.1 => false || true => true => CONFLICT! - sorry mate, you have raised a conflict, you can't be installed

We can go 2 ways:

Twig provides namespaced class aliases since 1.38.1 and 2.12.1, so this should work on all recently released Twig versions.
Silex is deprecated anyway.
@mhujer mhujer force-pushed the twig-3.0-compatibility branch from 0340ef8 to fbabff8 Compare November 26, 2019 19:26
@mhujer
Copy link
Contributor Author

mhujer commented Nov 26, 2019

@kubawerlos thanks for pointing that out! I didn't occur to me before.

I fixed it by specifying the ranges with lower band:

    "conflict": {
        "twig/twig": ">=1,<1.38.1|>=2,<2.12.1"
    },

(got inspiration from https://github.com/Roave/SecurityAdvisories/blob/master/composer.json - they have quite a few conflict rules there).

@florianeckerstorfer
Copy link
Member

I have absolutely no knowledge about the conflict rules in Composer, @kubawerlos do you think this is fine now?

@kubawerlos
Copy link
Contributor

@florianeckerstorfer looks fine

@florianeckerstorfer florianeckerstorfer merged commit 3a331ae into cocur:master Nov 27, 2019
@mhujer mhujer deleted the twig-3.0-compatibility branch November 27, 2019 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants