NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromfabpot:twig-relative-paths-fix

Nov 9, 2016

Merged

[TwigBundle] fixed template root path#20428

fabpot merged 1 commit intosymfony:masterfromfabpot:twig-relative-paths-fix

Nov 9, 2016

Conversation

Copy link

Member

fabpot commentedNov 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

[TwigBundle] fixed template root path

526b8a0

carsonbot added Status: Needs Review TwigBundle Bug labels

Nov 6, 2016

fabpot mentioned this pull request

Nov 6, 2016

[TwigBundle] made Twig cache independent of the project root directory#20285

Merged

Copy link

Member

javiereguiluz commentedNov 7, 2016

👍

Status: reviewed

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Nov 7, 2016

javiereguiluz added this to the3.2 milestone

Nov 7, 2016

Copy link

Contributor

linaori commentedNov 7, 2016

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

Copy link

MemberAuthor

fabpot commentedNov 7, 2016

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

Copy link

Contributor

linaori commentedNov 8, 2016•
edited
Loading

If I understand this PR correctly, it will traverse down until it find acomposer.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 acomposer.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.

Copy link

Member

nicolas-grekas commentedNov 8, 2016

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 inb376d92

	$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);

Copy link

MemberAuthor

fabpot commentedNov 9, 2016

@nicolas-grekas I prefer my strategy. Storing thecomposer.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 thevendor directory can be anywhere (there is even a configuration for that incomposer.json:vendor-dir). As thevendor 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.

Copy link

Member

nicolas-grekas commentedNov 9, 2016

👍

1 similar comment

Copy link

Contributor

Tobion commentedNov 9, 2016

👍

fabpot merged commit526b8a0 intosymfony:master

Nov 9, 2016

fabpot added a commit that referenced this pull request

Nov 9, 2016

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/aCommits-------526b8a0 [TwigBundle] fixed template root path

stof reviewed

Nov 9, 2016

View reviewed changes

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


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