added Twig_Source to hold information about the original template #2182

fabpot · 2016-10-15T19:35:15Z

Alternative to #2170

nicolas-grekas · 2016-10-17T09:00:17Z

lib/Twig/Node/Module.php

-            'source' => $source,
-            'filename' => $filename,
+            'source' => $source->getCode(),
+            // to be renamed in 2.0 to name


then let's add name right now?

nicolas-grekas · 2016-10-17T09:00:42Z

lib/Twig/Node/Module.php

+            'source' => $source->getCode(),
+            // to be renamed in 2.0 to name
+            'filename' => $source->getName(),
+            // to be renamed in 2.0 to filename


but that would be a hard break, no continuous upgrade path

stof · 2016-10-17T09:02:46Z

lib/Twig/Loader/Filesystem.php

@@ -14,7 +14,7 @@
 *
 * @author Fabien Potencier <fabien@symfony.com>
 */
-class Twig_Loader_Filesystem implements Twig_LoaderInterface, Twig_ExistsLoaderInterface
+class Twig_Loader_Filesystem implements Twig_LoaderInterface, Twig_ExistsLoaderInterface, Twig_SourceContextLoaderInterface


the chain loader should implement Twig_SourceContextLoaderInterface too, otherwise we loose the capabilities when using multiple loaders

stof · 2016-10-17T09:04:29Z

lib/Twig/Template.php

+     */
+    public function getSourceContext()
+    {
+        return new Twig_Source('');


$this->getTemplateName() should be passed as second argument here

stof · 2016-10-17T09:04:55Z

lib/Twig/Source.php

+    private $name;
+    private $filename;
+
+    public function __construct($code, $name = null, $filename = null)


please add phpdoc to document the variable types

stof · 2016-10-17T09:05:11Z

lib/Twig/SourceContextLoaderInterface.php

+
+interface Twig_SourceContextLoaderInterface
+{
+    /**


missing phpdoc for the argument

stof · 2016-10-17T09:06:38Z

lib/Twig/TokenStream.php

+            if (null !== $filename || null !== $source) {
+                @trigger_error(sprintf('Passing a string as the $filename argument of %s() is deprecated since version 1.27. Pass a Twig_Source instance instead.', __METHOD__), E_USER_DEPRECATED);
+            }
+            $this->source = new Twig_Source($filename, $source);


this is wrong. the first argument of Twig_Source is the code

Please also add legacy tests covering the deprecated APIs. They would catch such mistakes (and forbid breaking BC by mistake)

fixed, tests coming

stof · 2016-10-17T09:08:35Z

lib/Twig/TokenParser/AutoEscape.php

@@ -39,7 +39,7 @@ public function parse(Twig_Token $token)
        } else {
            $expr = $this->parser->getExpressionParser()->parseExpression();
            if (!$expr instanceof Twig_Node_Expression_Constant) {
-                throw new Twig_Error_Syntax('An escaping strategy must be a string or a bool.', $stream->getCurrent()->getLine(), $stream->getFilename());
+                throw new Twig_Error_Syntax('An escaping strategy must be a string or a bool.', $stream->getCurrent()->getLine(), $this->parser->getFilename());


why not using $stream->getSourceContext()->getName() directly to avoid 1 level in the stack each time ? the parser will forward the call to the stream anyway

I've deprecated Parser::getFilename() as having a proxy does not make sense and because the method is wrong anyway, it returns the logical name, not the filename.

fabpot · 2016-10-17T18:35:01Z

I've extracted the deprecation of Parser::getFilename() in #2183 to make code review easier. I'm also going to drop filename and use path or filepath instead. So, name is the logical template name and path its filesystem path. That should avoid any confusion.

fabpot · 2016-10-17T18:46:51Z

Rebased on top of #2183, reviewing the second commit only gives you less noise. Renamed getFilename() to get getPath(). I think it's much better that way.

This PR was merged into the 1.x branch. Discussion ---------- deprecated Parser::getFilename() The name is misleading as it returns the logical template name and not the proper filesystem path. In #2182, this is then replaced with a proper object. This PR simplifies the code review for #2182 by avoiding the pollution of all the changes in the token parser classes. Commits ------- d77f3dd deprecated Parser::getFilename()

fabpot · 2016-10-17T19:30:49Z

Tests added, not WIP anymore

…l template (fabpot) This PR was merged into the 1.x branch. Discussion ---------- added Twig_Source to hold information about the original template Alternative to #2170 Commits ------- 6c76583 added Twig_Source to hold information about the original template

This PR was merged into the 2.7 branch. Discussion ---------- [Twig] removed deprecations added in Twig 1.27 | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | see twigphp/Twig#2182 | License | MIT | Doc PR | n/a Commits ------- f0849d8 [TwigBridge] removed deprecations added in Twig 1.27

This PR was merged into the 2.7 branch. Discussion ---------- [Twig] removed deprecations added in Twig 1.27 | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | see twigphp/Twig#2182 | License | MIT | Doc PR | n/a Commits ------- f0849d8 [TwigBridge] removed deprecations added in Twig 1.27

stof · 2016-10-18T07:57:34Z

lib/Twig/Loader/Chain.php

+        $exceptions = array();
+        foreach ($this->loaders as $loader) {
+            if (!$loader instanceof Twig_SourceContextLoaderInterface) {
+                continue;


this is broken in case this loader was the one loading the template, but a next loader could load a template with the same name

fixed in #2192

marc1706 · 2016-11-03T20:15:59Z

Causes a BC break when upgrading from 1.26 and prior to twig 1.27 as described here:
6c76583#commitcomment-19686237

This addresses some issues with symfony that resulted in issues when open_basedir restrictions were enabled, as well as issues with JPEG images in fast-image-size. The phpBB class extending the twig lexer also had to be modified to ensure compatibility after BC was broken by the PR twigphp/Twig#2182 for twig 1.27. PHPBB3-14716

xabbuh · 2016-11-04T09:23:36Z

@stof But that doesn't mean that someone could have implemented the interface before which will no indeed break their code when updating to Twig 1.27.

marc1706 · 2016-11-04T09:28:13Z

My reply to @stof 's comment:
6c76583#commitcomment-19693147

Should probably continue the discussion here as that's more accessible to other users than having the discussion in the commit.

edit: Created issue as pointed out by stof

This addresses some issues with symfony that resulted in issues when open_basedir restrictions were enabled, as well as issues with JPEG images in fast-image-size. The phpBB class extending the twig lexer also had to be modified to ensure compatibility after BC was broken by the PR twigphp/Twig#2182 for twig 1.27. PHPBB3-14716

fabpot mentioned this pull request Oct 15, 2016

[Twig] removed deprecations added in Twig 1.27 symfony/symfony#20224

Merged

fabpot force-pushed the filename-debuginfo branch from c762379 to d57a59b Compare October 15, 2016 20:28

nicolas-grekas mentioned this pull request Oct 15, 2016

Added source file in debug info when available #2170

Closed

nicolas-grekas reviewed Oct 17, 2016

View reviewed changes

stof reviewed Oct 17, 2016

View reviewed changes

fabpot force-pushed the filename-debuginfo branch from 9a885eb to f63d882 Compare October 17, 2016 18:15

fabpot mentioned this pull request Oct 17, 2016

deprecated Parser::getFilename() #2183

Merged

fabpot force-pushed the filename-debuginfo branch from f63d882 to b09c9ce Compare October 17, 2016 18:46

fabpot force-pushed the filename-debuginfo branch 5 times, most recently from 5868aa5 to 8303f4a Compare October 17, 2016 19:04

fabpot force-pushed the filename-debuginfo branch 2 times, most recently from a989b79 to 25495ef Compare October 17, 2016 19:29

added Twig_Source to hold information about the original template

6c76583
A3DB

fabpot force-pushed the filename-debuginfo branch from 25495ef to 6c76583 Compare October 17, 2016 20:33

fabpot merged commit 6c76583 into twigphp:1.x Oct 17, 2016

fabpot deleted the filename-debuginfo branch October 17, 2016 21:14

stof reviewed Oct 18, 2016

View reviewed changes

marc1706 mentioned this pull request Nov 3, 2016

[ticket/14716] Update dependencies to latest versions phpbb/phpbb#4501

Merged

4 tasks

marc1706 mentioned this pull request Nov 4, 2016

Change to LexerInterface in 1.27 causes BC break with previous implementations #2218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

added Twig_Source to hold information about the original template #2182

added Twig_Source to hold information about the original template #2182

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

added Twig_Source to hold information about the original template #2182

added Twig_Source to hold information about the original template #2182

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!