Skip to content

[TwigBundle] fixed template root path #20428

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 9, 2016
Merged

[TwigBundle] fixed template root path #20428

merged 1 commit into from
Nov 9, 2016

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Nov 6, 2016

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets see #20285
License MIT
Doc PR n/a

@fabpot fabpot mentioned this pull request Nov 6, 2016
Merged
@javiereguiluz
Copy link
Member

👍

Status: reviewed

@javiereguiluz javiereguiluz added this to the 3.2 milestone Nov 7, 2016
@linaori
Copy link
Contributor

linaori commented Nov 7, 2016

This is not fool proof though, what about an approach such as here?
https://github.com/hostnet/path-composer-plugin-lib

@fabpot
Copy link
Member Author

fabpot commented Nov 7, 2016

@iltar Can you explain in which circumstances it would not work?

@linaori
Copy link
Contributor

linaori commented Nov 8, 2016 •
edited
Loading

If I understand this PR correctly, it will traverse down until it find a composer.json (based on the kernel root). What if my composer.json is in (e.g.) %kernel.root_dir%/.composer or my kernel is shared between repositories and installed globally? Edge, case would be a weird situation but not impossible, possible not probable. With --working-dir, you can have a composer.json in any location of your system, it doesn't have to be in your repository.

There are just some cases that make this a bit difficult to ensure this would work.

@nicolas-grekas
Copy link
Member

I suggest using the common prefix between the vendor dir and the kernel.root_dir, as done in

symfony/src/Symfony/Bundle/WebProfilerBundle/DependencyInjection/WebProfilerExtension.php

Lines 56 to 66 in b376d92

$baseDir = array();
$rootDir = $container->getParameter('kernel.root_dir');
$rootDir = explode(DIRECTORY_SEPARATOR, realpath($rootDir) ?: $rootDir);
$bundleDir = explode(DIRECTORY_SEPARATOR, __DIR__);
for ($i = 0; isset($rootDir[$i], $bundleDir[$i]); ++$i) {
if ($rootDir[$i] !== $bundleDir[$i]) {
break;
}
$baseDir[] = $rootDir[$i];
}
$baseDir = implode(DIRECTORY_SEPARATOR, $baseDir);

@fabpot
Copy link
Member Author

fabpot commented Nov 9, 2016

@nicolas-grekas I prefer my strategy. Storing the composer.json (which should be in your repository) elsewhere than the root directory is I think not something we need to support (and a nightmare for anyone doing this). But the vendor directory can be anywhere (there is even a configuration for that in composer.json: vendor-dir). As the vendor should not be in your repo, and because this is just a read-only artifact, I can easily see people storing that somewhere else.

@iltar I don't want to support the edge cases you mention, they are non standard and I don't see why it would be a good idea in the first place. Supporting bad practices is not a goal.

@nicolas-grekas
Copy link
Member

👍

1 similar comment
@Tobion
Copy link
Contributor

Tobion commented Nov 9, 2016

👍

@fabpot fabpot merged commit 526b8a0 into symfony:master Nov 9, 2016
fabpot added a commit that referenced this pull request Nov 9, 2016
@fabpot
bug #20428 [TwigBundle] fixed template root path (fabpot)
6f58c76 
This PR was merged into the 3.2-dev branch.

Discussion
----------

[TwigBundle] fixed template root path

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | see #20285
| License       | MIT
| Doc PR        | n/a

Commits
-------

526b8a0 [TwigBundle] fixed template root path
stof
stof reviewed Nov 9, 2016
View reviewed changes
src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ExtensionPass.php
@@ -66,6 +66,7 @@ public function process(ContainerBuilder $container)

if (!$container->has('templating')) {
$loader = $container->getDefinition('twig.loader.native_filesystem');
$loader->replaceArgument(1, $this->getComposerRootDir($container->getParameter('kernel.root_dir')));
Copy link
Member

Choose a reason for hiding this comment

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

why is it configured only for when the templating componnet is disabled ? Should twig.loader.filesystem configure it too ?

@fabpot fabpot mentioned this pull request Nov 17, 2016
Merged
@fabpot fabpot deleted the twig-relative-paths-fix branch January 7, 2017 00:34
@pounard
Copy link
Contributor

pounard commented Mar 24, 2017

This causes problems as soon as you are a bit paranoid with open_basedir or deliver a project without the composer.json file: #22149

@pounard pounard mentioned this pull request Mar 24, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

Assignees
No one assigned
Labels
Bug Status: Reviewed TwigBundle
Projects
None yet
Milestone
3.2
Development

Successfully merging this pull request may close these issues.
8 participants
@fabpot @javiereguiluz @linaori @nicolas-grekas @Tobion @pounard @stof @carsonbot