Fixes twig bundle project root dir discovery #22570

pounard · 2017-04-28T10:32:24Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22149
License	MIT
Doc PR	none

TwigBundle ExtensionPass uses a directory lookup algorithm to find the composer.json file. When working under open_basedir restrictions, if the project root folder is not allowed PHP runtime to read, the composer.json will never be found, throwing PHP notices along the way and ending up using the filesystem root instead.

Since that 3.3 introduced the kernel getProjectRoot() method and kernel.project_dir variable, the TwigBundle can now leverage it, in order to get more consistent, and allow site builders to override the getProjectRoot() kernel method, hence bypass the open_basedir restrictions in reliable and safe way.

stof · 2017-04-28T10:39:04Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ExtensionPass.php

        $twigLoader = $container->getDefinition('twig.loader.native_filesystem');
        if ($container->has('templating')) {
            $loader = $container->getDefinition('twig.loader.filesystem');
            $loader->setMethodCalls(array_merge($twigLoader->getMethodCalls(), $loader->getMethodCalls()));
-            $loader->replaceArgument(2, $composerRootDir);
+            $loader->replaceArgument(2, $projectDir);


even better: use %kernel.project_dir% directly in the service definition, and remove the handling of this argument from the compiler pass

Ok, got it now, you're right it should work.

pounard

@stof did your changes.

pounard · 2017-04-28T11:13:57Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ExtensionPass.php

        $twigLoader = $container->getDefinition('twig.loader.native_filesystem');
        if ($container->has('templating')) {
            $loader = $container->getDefinition('twig.loader.filesystem');
            $loader->setMethodCalls(array_merge($twigLoader->getMethodCalls(), $loader->getMethodCalls()));
-            $loader->replaceArgument(2, $composerRootDir);
+            $loader->replaceArgument(2, $projectDir);


Ok, got it now, you're right it should work.

pounard · 2017-04-28T11:20:15Z

Hum. I do need to fix tests. I obviously ran the wrong tests the first time.

pounard · 2017-04-28T11:23:47Z

@stof how would you fix it for 3.2 and 3.0 since that the kernel.project_dir variable does not exist in there ? I was think in trying to read it anyway (ie. if ($container->hasParameter('kernel.project_dir')) {) is that a bad idea?

The question is not innocent, we do really experience this bug on production apps, and it needs fixing in current stable, I'm not fond in backporting a variable in stable, but it would help us fixing our env by just setting it.

pounard · 2017-04-28T11:29:10Z

Only PHP 7.1 fails, but for a very deep unknown reason.

pounard · 2017-04-28T11:29:58Z

[pounard@guinevere] /var/www/symfony
 >  uname -a
Linux guinevere 4.10.11-1-ARCH #1 SMP PREEMPT Tue Apr 18 08:39:42 CEST 2017 x86_64 GNU/Linux
[pounard@guinevere] /var/www/symfony
 >  php -v
PHP 7.1.4 (cli) (built: Apr 11 2017 18:45:29) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.1.4, Copyright (c) 1999-2017, by Zend Technologies
    with Xdebug v2.5.3, Copyright (c) 2002-2017, by Derick Rethans
[pounard@guinevere] /var/www/symfony
 >  phpunit -vvv --filter TwigBundle
PHPUnit 5.7.19 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.1.4 with Xdebug 2.5.3
Configuration: /var/www/symfony/phpunit.xml.dist

Testing Symfony Test Suite
........................................                          40 / 40 (100%)

Time: 7.5 seconds, Memory: 96.00MB

OK (40 tests, 143 assertions)

Legacy deprecation notices (1)

Test OK locally.

ro0NL · 2017-04-28T11:42:22Z

You need to bump the http-kernel dep in the twig bundle;
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/TwigBundle/composer.json#L23

This parameter exists as of 3.3

how would you fix it for 3.2 and 3.0 since that the kernel.project_dir variable does not exist in there

Maybe keep the current implementation, and consider it "improved" as of 3.3

pounard · 2017-04-28T12:10:27Z

@ro0NL

Maybe keep the current implementation, and consider it "improved" as of 3.3

The current implementation in 3.0 -> 3.2 is subject to the original bug I experienced: when the project root dir is under open_basedir restriction, TwigBundle skips the folder because it can't read the composer.json file, then the loop goes until it reaches the filesystem root and uses that instead. It needs fixing.

pounard · 2017-04-28T12:12:13Z

@ro0NL in your opinion, should I get the getComposerDir() function back into the master and just conditionally use the kernel.project_root if exists ?

ro0NL · 2017-04-28T12:23:02Z

In master %kernel.project_dir% just exists.. so this PR is fine for master/3.3 i guess :)

The current implementation in 3.0 -> 3.2 is subject to the original bug I experienced

Thing is we cant backport a new feature (the project_dir param).. so we need a different approach on lower branches... ie. the current one.

chalasr · 2017-04-28T12:26:52Z

The current implementation in 3.0 -> 3.2 is subject to the original bug I experienced: when the project root dir is under open_basedir restriction, TwigBundle skips the folder because it can't read the composer.json file, then the loop goes until it reaches the filesystem root and uses that instead. It needs fixing.

Doesn't the same situation occur with kernel.project_dir? The default implementation of Kenrel::getProjectRootDir() looks similar

ro0NL · 2017-04-28T12:28:02Z

True.. but you can override the param way more easily :) and if overriden, we need to use that variant as well :)

chalasr · 2017-04-28T12:28:52Z

sure :)

stof · 2017-04-28T12:35:49Z

Using the project dir is the right approach for 3.3+. So to fix the build for deps=low, you just need to bump the min required version in TwigBundle

…ment

pounard · 2017-04-28T12:39:57Z

Ok, I bumped the version, now the real tricky question arise: how do I fix it for 3.0 -> 3.2 versions knowing I have real sites in real productions buggy because of this ?

ro0NL · 2017-04-28T12:43:06Z

you need to bump to 3.3 ;-) the param does not exists in 3.2.2.

Maybe for lower branches we should start checking for open_basedir, and if we traversed to the filesystem root, but without a composer.json return the current dir as well 🤔

pounard · 2017-04-28T12:44:39Z

Oh sorry, just stupidly ran the test locally and pushed...

pounard · 2017-04-28T13:31:03Z

w00t, sorry for the noise, it should be OK now.

nicolas-grekas

👍

fabpot · 2017-04-28T17:17:03Z

Thank you @pounard.

This PR was squashed before being merged into the 3.3-dev branch (closes #22570). Discussion ---------- Fixes twig bundle project root dir discovery | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22149 | License | MIT | Doc PR | none  TwigBundle ExtensionPass uses a directory lookup algorithm to find the composer.json file. When working under open_basedir restrictions, if the project root folder is not allowed PHP runtime to read, the composer.json will never be found, throwing PHP notices along the way and ending up using the filesystem root instead. Since that 3.3 introduced the kernel getProjectRoot() method and kernel.project_dir variable, the TwigBundle can now leverage it, in order to get more consistent, and allow site builders to override the getProjectRoot() kernel method, hence bypass the open_basedir restrictions in reliable and safe way. Commits ------- 3cb2e5b Fixes twig bundle project root dir discovery

#22149 - fixes twig bundle project root dir discovery

f36325d

carsonbot added Status: Needs Review Bug labels Apr 28, 2017

pounard mentioned this pull request Apr 28, 2017

TwigBundle's ExtensionPass does extensive filesystem lookup, and fail when project root dir is under open_basedir restrictions #22149

Closed

stof requested changes Apr 28, 2017

View reviewed changes

move kernel.project_dir arguments to twig services definitions

5798cb9

pounard commented Apr 28, 2017

View reviewed changes

fixes TwigBundle unit tests

cd08369

symfony/http-kernel dependency version bump to ^3.2.2 minimum require…

98d3691

…ment

symfony/http-kernel dependency version bump to ^3.3 minimum requirement

fcfb094

nicolas-grekas added this to the 3.3 milestone Apr 28, 2017

nicolas-grekas approved these changes Apr 28, 2017

View reviewed changes

fabpot closed this Apr 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes twig bundle project root dir discovery #22570

Fixes twig bundle project root dir discovery #22570

pounard commented Apr 28, 2017

stof Apr 28, 2017

pounard Apr 28, 2017

pounard left a comment

pounard Apr 28, 2017

pounard commented Apr 28, 2017 •

edited

pounard commented Apr 28, 2017 •

edited

pounard commented Apr 28, 2017

pounard commented Apr 28, 2017

ro0NL commented Apr 28, 2017

pounard commented Apr 28, 2017 •

edited

pounard commented Apr 28, 2017

ro0NL commented Apr 28, 2017 •

edited

chalasr commented Apr 28, 2017 •

edited

ro0NL commented Apr 28, 2017 •

edited

chalasr commented Apr 28, 2017

stof commented Apr 28, 2017

pounard commented Apr 28, 2017

ro0NL commented Apr 28, 2017 •

edited

pounard commented Apr 28, 2017

pounard commented Apr 28, 2017

nicolas-grekas left a comment

fabpot commented Apr 28, 2017

Fixes twig bundle project root dir discovery #22570

Fixes twig bundle project root dir discovery #22570

Conversation

pounard commented Apr 28, 2017

stof Apr 28, 2017

Choose a reason for hiding this comment

pounard Apr 28, 2017

Choose a reason for hiding this comment

pounard left a comment

Choose a reason for hiding this comment

pounard Apr 28, 2017

Choose a reason for hiding this comment

pounard commented Apr 28, 2017 • edited

pounard commented Apr 28, 2017 • edited

pounard commented Apr 28, 2017

pounard commented Apr 28, 2017

ro0NL commented Apr 28, 2017

pounard commented Apr 28, 2017 • edited

pounard commented Apr 28, 2017

ro0NL commented Apr 28, 2017 • edited

chalasr commented Apr 28, 2017 • edited

ro0NL commented Apr 28, 2017 • edited

chalasr commented Apr 28, 2017

stof commented Apr 28, 2017

pounard commented Apr 28, 2017

ro0NL commented Apr 28, 2017 • edited

pounard commented Apr 28, 2017

pounard commented Apr 28, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Apr 28, 2017

pounard commented Apr 28, 2017 •

edited

pounard commented Apr 28, 2017 •

edited

pounard commented Apr 28, 2017 •

edited

ro0NL commented Apr 28, 2017 •

edited

chalasr commented Apr 28, 2017 •

edited

ro0NL commented Apr 28, 2017 •

edited

ro0NL commented Apr 28, 2017 •

edited