fixed regression when registering two extensions with the same class #2200

fabpot · 2016-10-22T16:54:52Z

GwendolenLynch · 2016-10-22T19:00:09Z

@fabpot With this diff applied, against twig/twig 1.26.1 (with symfony/twig-bridge 2.8.12), I m still seeing the exception below (reverting to `twig/twig:1.25 symfony/twig-bridge:2.8.11 obviously falls back to expected behaviour).

Fatal error.
Class: Twig_Error_Syntax
Message: Unknown "users" function in "gawain.twig" at line 7.
Code: 0
Twig_ExpressionParser->getFunctionNodeClass()
[root]//vendor/twig/twig/lib/Twig/ExpressionParser.php, line 351

Twig_ExpressionParser->getFunctionNode()
[root]//vendor/twig/twig/lib/Twig/ExpressionParser.php, line 144

Twig_ExpressionParser->parsePrimaryExpression()
[root]//vendor/twig/twig/lib/Twig/ExpressionParser.php, line 84

Twig_ExpressionParser->getPrimary()
[root]//vendor/twig/twig/lib/Twig/ExpressionParser.php, line 41

Twig_ExpressionParser->parseExpression()
[root]//vendor/twig/twig/lib/Twig/TokenParser/For.php, line 32

Twig_TokenParser_For->parse()
[root]//vendor/twig/twig/lib/Twig/Parser.php, line 187

Twig_Parser->subparse()
[root]//vendor/twig/twig/lib/Twig/TokenParser/Block.php, line 38

Twig_TokenParser_Block->parse()
[root]//vendor/twig/twig/lib/Twig/Parser.php, line 187

Twig_Parser->subparse()
[root]//vendor/twig/twig/lib/Twig/Parser.php, line 100

Twig_Parser->parse()
[root]//vendor/twig/twig/lib/Twig/Environment.php, line 645

Twig_Environment->parse()
[root]//vendor/twig/twig/lib/Twig/Environment.php, line 705

Twig_Environment->compileSource()
[root]//vendor/twig/twig/lib/Twig/Environment.php, line 406

Tobion · 2016-10-22T20:34:13Z

test/Twig/Tests/EnvironmentTest.php

@@ -538,6 +559,21 @@ public function getName()
    }
 }

+class Twig_Tests_EnvironmentTest_Extension_DynamicWithDeprecatedName extends Twig_Extension
+{
+    public $name;


this doesn't need to be public, does it?

stof · 2016-10-22T23:30:11Z

lib/Twig/Environment.php

+        if (null !== $legacyName) {
+            if (isset($this->legacyExtensions[$legacyName])) {
+                unset($this->extensions[$class], $this->legacyExtensions[$legacyName]);
+                @trigger_error(sprintf('The possibility to register the same extension twice ("%s") is deprecated since version 1.23 and will be removed in Twig 2.0. Use proper PHP inheritance instead.', $legacyName), E_USER_DEPRECATED);


what if $this->extensions[$class] is set ?

It is now always set, we maintain two arrays in parallel. If you mean the same class once registered as a class and once as a legacy name, I don't think we need to support that.

fabpot · 2016-10-23T00:58:26Z

@GawainLynch Can you tell me how I can reproduce this (I suppose you are having this with Bolt)? I have a blank page. And when I try to debug that, I have a problem not related to this one on your Twig filesystem loader, which breaks the contract of the original one as your override of findTemplate() returns a FileInterface object and not a string like the one in Twig.

fabpot · 2016-10-23T01:22:24Z

Forgot to mention that when applying the patch on 2.6.1, I can use the Bolt interface without any issue. So, that's why I tried to use the latest version of Twig and found the issue mentioned above.

GwendolenLynch · 2016-10-23T07:15:02Z

I have a blank page.

Yeah, you're probably using < Bolt 3.2 … It's a long slow process to get the code base up to scratch, but the in the upcoming 3.2 (RC tomorrow) I ripped out the various attempts and exception handling and have switched us to symfony/debug … not to mention the registration/request cycle abuse that we're finally almost away from.

on your Twig filesystem loader, which breaks the contract of the original one as your override of findTemplate() returns a FileInterface object and not a string like the one in Twig.

Yeah, we are hoping to engage with you on something around this in Twig 2. We abstract most of our file system, 3.3 should allow most mounts to be on services like S3, and as Twig internally assumes a local filesystem we had to make a choice there … To be continued. (ping @CarsonF)

Can you tell me how I can reproduce this

I have created a couple of quick extensions repositories, to minimise code steps, and below (tested here) gets you up an running in core-developer mode (not recommended, obviously, for real use).

git clone https://github.com/bolt/bolt.git bolt-twig-debug
cd bolt-twig-debug/
git checkout release/3.2 
composer update
mkdir -p extensions/local/bolt
git clone https://github.com/GawainLynch/bolt-extension-test-twig-a.git extensions/local/bolt/test-twig-a
git clone https://github.com/GawainLynch/bolt-extension-test-twig-b.git extensions/local/bolt/test-twig-b
./app/nut extensions:setup
php -S localhost:8080 -t . index.php

Load http://localhost:8080/ enter first user data, and you should be directed to the admin page.

Click the link in the flash message "It seems there's no content in the database. To get started quickly, add some Lorem Ipsum dummy content." to create dummy content.

Navigate to http://localhost:8080/bolt/file/edit/themes/base-2016/index.twig

After line 5 add:

    {{ twig_a()|raw }}
    {{ twig_b()|raw }}

Save and visit http://localhost:8080/

Now update Twig to twig/twig:^1.26

Note: I have separated this step out, as the Twig extensions breakage won't allow you to complete initial set-up

Visit http://localhost:8080/ 💥

fabpot · 2016-10-23T15:28:29Z

@GawainLynch Thank you so much. I can now reproduce the issue and I've a quick fix. I need to think a bit more about the final patch, but should be available today or tomorrow.

fabpot · 2016-10-23T16:37:32Z

@GawainLynch Just pushed a new version. Can you check that it works well for you?

GwendolenLynch · 2016-10-23T20:11:07Z

@fabpot Fixed confirmed, thank you! Coffee is on me next time you're back in Europe.

…s name

fabpot · 2016-10-23T20:12:53Z

Thank you for helping me figure out this one, your reproducing steps helped a lot.

…ame class (fabpot) This PR was merged into the 1.x branch. Discussion ---------- fixed regression when registering two extensions with the same class fixed #2165 Commits ------- 47432f3 fixed regression when registering two extensions having the same class name

fabpot mentioned this pull request Oct 22, 2016

Upgraded to version 1.26.0. Now templates aren't rendering #2165

Closed

Tobion reviewed Oct 22, 2016

View reviewed changes

stof reviewed Oct 22, 2016

View reviewed changes

fabpot force-pushed the bc-break-fix branch from 714ee45 to 93861a7 Compare October 23, 2016 01:28

fabpot force-pushed the bc-break-fix branch 3 times, most recently from 9fd5495 to 767daf6 Compare October 23, 2016 16:31

fabpot mentioned this pull request Oct 23, 2016

[TwigBundle] The "core" extension is not enabled. symfony/symfony#20284

Closed

fixed regression when registering two extensions having the same clas…

47432f3

…s name

fabpot force-pushed the bc-break-fix branch from 767daf6 to 47432f3 Compare October 23, 2016 20:12

fabpot merged commit 47432f3 into twigphp:1.x Oct 23, 2016

fabpot deleted the bc-break-fix branch October 24, 2016 15:29

0xdfgp mentioned this pull request Dec 15, 2016

PHP7 issue: "The "Twig_Extension_Core" extension is not enabled" #2297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed regression when registering two extensions with the same class #2200

fixed regression when registering two extensions with the same class #2200

fabpot commented Oct 22, 2016

GwendolenLynch commented Oct 22, 2016

Tobion Oct 22, 2016

stof Oct 22, 2016

fabpot Oct 23, 2016

fabpot commented Oct 23, 2016

fabpot commented Oct 23, 2016

GwendolenLynch commented Oct 23, 2016

fabpot commented Oct 23, 2016

fabpot commented Oct 23, 2016

GwendolenLynch commented Oct 23, 2016

fabpot commented Oct 23, 2016

fixed regression when registering two extensions with the same class #2200

fixed regression when registering two extensions with the same class #2200

Conversation

fabpot commented Oct 22, 2016

GwendolenLynch commented Oct 22, 2016

Tobion Oct 22, 2016

Choose a reason for hiding this comment

stof Oct 22, 2016

Choose a reason for hiding this comment

fabpot Oct 23, 2016

Choose a reason for hiding this comment

fabpot commented Oct 23, 2016

fabpot commented Oct 23, 2016

GwendolenLynch commented Oct 23, 2016

fabpot commented Oct 23, 2016

fabpot commented Oct 23, 2016

GwendolenLynch commented Oct 23, 2016

fabpot commented Oct 23, 2016