[HttpKernel] removed absolute paths from the generated container #10999

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
@fabpot
Member

fabpot commented May 26, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets first step to resolve #6484, #3079, and #9238
License MIT
Doc PR n/a

This code was merged to 2.3 in (#10894, #10937, and #10979) but it caused too many problems and so was reverted. This PR targets master (aka 2.6) to get plenty of time to figure out the best way to implement this feature without too many BC breaks.

Some issues that should be taken care of before re-merging this into master: #10976, #10943, symfony/symfony-standard#659.

The biggest problem is the usage of dirname which is naive per the PHP documentation: "dirname() operates naively on the input string, and is not aware of the actual filesystem, or path components such as "..".")". So, basically, as we have now .. in the root and cache dirs, using dirname on them is not possible anymore. One solution would be to use dirname calls instead of adding .. specifically for the root and cache dir, but other paths could be used a problem as well. A better fix would be to check usage of all dirname calls and be sure that we operate on a safe path or call realpath when needed.

From the original PR:

This PR converts absolute paths to relative ones in the dumped container. The code is a bit "ugly", but it gets the job done and I'm not sure that there is a more elegant way without breaking everything.

@mvrhov

This comment has been minimized.

Show comment
Hide comment
@mvrhov

mvrhov May 27, 2014

Contributor

@fabpot realpath won't work when the kernel will be inside phar archive.

A long time ago I solved the problem of absolute paths by defining a root dir of the application by a define inside of front controller. In the symfony's case the root dir would be the dir where composer.json resides.

Contributor

mvrhov commented May 27, 2014

@fabpot realpath won't work when the kernel will be inside phar archive.

A long time ago I solved the problem of absolute paths by defining a root dir of the application by a define inside of front controller. In the symfony's case the root dir would be the dir where composer.json resides.

@jameshalsall

This comment has been minimized.

Show comment
Hide comment
@jameshalsall

jameshalsall May 27, 2014

Contributor

@mvrhov That won't work so well when you're running console commands in the standard dist

Contributor

jameshalsall commented May 27, 2014

@mvrhov That won't work so well when you're running console commands in the standard dist

@mvrhov

This comment has been minimized.

Show comment
Hide comment
@mvrhov

mvrhov May 27, 2014

Contributor

For the console command it could be setup inside command file.

Contributor

mvrhov commented May 27, 2014

For the console command it could be setup inside command file.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 27, 2014

Member

The logic available in Composer might help to solve the issue of ..: https://github.com/composer/composer/blob/master/src/Composer/Util/Filesystem.php#L207-358

Member

stof commented May 27, 2014

The logic available in Composer might help to solve the issue of ..: https://github.com/composer/composer/blob/master/src/Composer/Util/Filesystem.php#L207-358

@jockri jockri referenced this pull request in Kunstmaan/KunstmaanAdminBundle May 28, 2014

Merged

Fix path to composer.lock file #310

@jakzal

This comment has been minimized.

Show comment
Hide comment
@jakzal

jakzal May 28, 2014

Member

Practice of putting the cache folder outside of the project root directory is not uncommon. I think we should consider such scenarios (see #11011 for example).

Member

jakzal commented May 28, 2014

Practice of putting the cache folder outside of the project root directory is not uncommon. I think we should consider such scenarios (see #11011 for example).

@Axel29

This comment has been minimized.

Show comment
Hide comment
@Axel29

Axel29 May 30, 2014

Hello,

The bug seems to be corrected with Windows but not with Mac. I have this message with the config.php file:
"Your parameters.yml file has been overwritten with these parameters (in /path/to/symfony/app/cache/dev/../../config/parameters.yml):"

And when I try to create a bundle with console:
"Target directory [/path/to/symfony/app/cache/dev/../src]:"

Does anyone have the solution?

Axel29 commented May 30, 2014

Hello,

The bug seems to be corrected with Windows but not with Mac. I have this message with the config.php file:
"Your parameters.yml file has been overwritten with these parameters (in /path/to/symfony/app/cache/dev/../../config/parameters.yml):"

And when I try to create a bundle with console:
"Target directory [/path/to/symfony/app/cache/dev/../src]:"

Does anyone have the solution?

@dzuelke

This comment has been minimized.

Show comment
Hide comment
@dzuelke

dzuelke May 30, 2014

Contributor

Any chance to roll a release soon with the revert, @fabpot? We're seeing a lot of users hit it with 2.4.5.

Contributor

dzuelke commented May 30, 2014

Any chance to roll a release soon with the revert, @fabpot? We're seeing a lot of users hit it with 2.4.5.

@dmaicher

This comment has been minimized.

Show comment
Hide comment
@dmaicher

dmaicher Jun 3, 2014

Contributor

We ran into the same problem on 2.4.5. Now 2.4.6 has been released and the revert is included ;)

Contributor

dmaicher commented Jun 3, 2014

We ran into the same problem on 2.4.5. Now 2.4.6 has been released and the revert is included ;)

@fabpot fabpot added the HttpKernel label Jun 16, 2014

@toretto460

This comment has been minimized.

Show comment
Hide comment
@toretto460

toretto460 Jul 19, 2014

Contributor

Also the Heroku infrastructure seems to be affected by this issue, heroku/heroku-buildpack-php#64

Contributor

toretto460 commented Jul 19, 2014

Also the Heroku infrastructure seems to be affected by this issue, heroku/heroku-buildpack-php#64

@ricardclau

This comment has been minimized.

Show comment
Hide comment
@ricardclau

ricardclau Aug 5, 2014

Contributor

+1 on removing absolute paths from the files in the cache folder and make everything relative using __DIR__ where possible

If you want to build deb / rpm packages or similar, the absolute paths force you to run cache:warm --env=prod in every server you deploy to. And even if you use rsync or any similar method, if you are deploying to 150 servers this is a lot of useless work.

Ok, you can have a build server matching the same folders structure but this is a hacky solution

Having said that, agreed that there are many players and infrastructures to take into account so 2.6 or even 2.7 should be the target for that

Contributor

ricardclau commented Aug 5, 2014

+1 on removing absolute paths from the files in the cache folder and make everything relative using __DIR__ where possible

If you want to build deb / rpm packages or similar, the absolute paths force you to run cache:warm --env=prod in every server you deploy to. And even if you use rsync or any similar method, if you are deploying to 150 servers this is a lot of useless work.

Ok, you can have a build server matching the same folders structure but this is a hacky solution

Having said that, agreed that there are many players and infrastructures to take into account so 2.6 or even 2.7 should be the target for that

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 20, 2014

Member

Looking at this issue again, I think there is no easy fix. Using dirname() will indeed work but as noted by @jakzal, the cache dir can be outside of the root dir. In this case, using dirname() won't help.

I cannot see any way to make it reliable in all cases, so I propose to make this an opt-in feature. You would opt-in by defining a constant (something like SYMFONY_ROOT_DIR). When defined, Symfony would make paths relative to the constant.

We can add this easily in Symfony Standard Edition (in autoload.php to make it available for the Web and in the CLI):

// using realpath here is required
define('SYMFONY_ROOT_DIR', realpath(__DIR__.'/...'));
Member

fabpot commented Sep 20, 2014

Looking at this issue again, I think there is no easy fix. Using dirname() will indeed work but as noted by @jakzal, the cache dir can be outside of the root dir. In this case, using dirname() won't help.

I cannot see any way to make it reliable in all cases, so I propose to make this an opt-in feature. You would opt-in by defining a constant (something like SYMFONY_ROOT_DIR). When defined, Symfony would make paths relative to the constant.

We can add this easily in Symfony Standard Edition (in autoload.php to make it available for the Web and in the CLI):

// using realpath here is required
define('SYMFONY_ROOT_DIR', realpath(__DIR__.'/...'));
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 20, 2014

Member

A refinement of the above could be to not require the constant when the cache dir is indeed under the root_dir.

Member

fabpot commented Sep 20, 2014

A refinement of the above could be to not require the constant when the cache dir is indeed under the root_dir.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 25, 2014

Member

@fabpot why do we need the constant in most cases ? We have $kernel->getRootDir() to give us the path, and it is available before working with the cache as it comes from the kernel. Then, we can make build a relative path. The logic has been implemented under MIT license in Composer already: https://github.com/composer/composer/blob/31eadc6920cd1866bc061fb0087798c37e2b7d14/src/Composer/Util/Filesystem.php#L300-382 We can borrow it.
The only case where we would face issues is when the path between your root folder and your cache folder changes between your different environments (locally or in prod) and you want to be able to copy the cache).

The real pain point is actually being able to identify all paths in the container needing to be rewritten, because not all the code is inside the kernel root dir. Or were you suggesting to have SYMFONY_ROOT_DIR being the root of the project itself, not the app/ folder ? In this case, it could indeed work easier to identify the paths, but it should have a different name to avoid confusion.

And rather than a constant, couldn't we use a method in the Kernel class ? This way, it would not require any change for existing projects (considering that the root of the project is %kernel.root_dir%/..

Member

stof commented Sep 25, 2014

@fabpot why do we need the constant in most cases ? We have $kernel->getRootDir() to give us the path, and it is available before working with the cache as it comes from the kernel. Then, we can make build a relative path. The logic has been implemented under MIT license in Composer already: https://github.com/composer/composer/blob/31eadc6920cd1866bc061fb0087798c37e2b7d14/src/Composer/Util/Filesystem.php#L300-382 We can borrow it.
The only case where we would face issues is when the path between your root folder and your cache folder changes between your different environments (locally or in prod) and you want to be able to copy the cache).

The real pain point is actually being able to identify all paths in the container needing to be rewritten, because not all the code is inside the kernel root dir. Or were you suggesting to have SYMFONY_ROOT_DIR being the root of the project itself, not the app/ folder ? In this case, it could indeed work easier to identify the paths, but it should have a different name to avoid confusion.

And rather than a constant, couldn't we use a method in the Kernel class ? This way, it would not require any change for existing projects (considering that the root of the project is %kernel.root_dir%/..

@jakzal jakzal referenced this pull request Oct 8, 2014

Closed

Opcache + Twig Templates #12172

@dzuelke

This comment has been minimized.

Show comment
Hide comment
@dzuelke

dzuelke Nov 12, 2014

Contributor

The logic is here as well, right? Symfony\Component\Filesystem\Filesystem::makePathRelative()?

Contributor

dzuelke commented Nov 12, 2014

The logic is here as well, right? Symfony\Component\Filesystem\Filesystem::makePathRelative()?

@eddiejaoude

This comment has been minimized.

Show comment
Hide comment
@eddiejaoude

eddiejaoude Nov 30, 2014

Contributor

Has this been resolved, as still as issue on Heroku deployment?

Contributor

eddiejaoude commented Nov 30, 2014

Has this been resolved, as still as issue on Heroku deployment?

@dzuelke

This comment has been minimized.

Show comment
Hide comment
@dzuelke

dzuelke Nov 30, 2014

Contributor

Two steps that help mitigate this:

  1. make sure you heroku config:set SYMFONY_ENV=prod before pushing
  2. in composer.json's post-install-cmd list, move the clearCache command to the very end
Contributor

dzuelke commented Nov 30, 2014

Two steps that help mitigate this:

  1. make sure you heroku config:set SYMFONY_ENV=prod before pushing
  2. in composer.json's post-install-cmd list, move the clearCache command to the very end
@eddiejaoude

This comment has been minimized.

Show comment
Hide comment
@eddiejaoude

eddiejaoude Nov 30, 2014

Contributor

Thanks. I have step 1 already. Step 2 removed that error but now I get no routes are found, always 404 on Heroku, locally & amazon work fine.

Contributor

eddiejaoude commented Nov 30, 2014

Thanks. I have step 1 already. Step 2 removed that error but now I get no routes are found, always 404 on Heroku, locally & amazon work fine.

@desyncr desyncr referenced this pull request in heroku/heroku-buildpack-php Nov 30, 2014

Closed

Unable to deploy Symfony application #64

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Dec 1, 2014

Member

@eddiejaoude can you please test #12784 and see if it helps?

Member

nicolas-grekas commented Dec 1, 2014

@eddiejaoude can you please test #12784 and see if it helps?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Dec 1, 2014

Member

closing in favor of #12784

Member

fabpot commented Dec 1, 2014

closing in favor of #12784

@fabpot fabpot closed this Dec 1, 2014

@eddiejaoude

This comment has been minimized.

Show comment
Hide comment
@eddiejaoude

eddiejaoude Dec 2, 2014

Contributor

👍

Contributor

eddiejaoude commented Dec 2, 2014

👍

@dzuelke

This comment has been minimized.

Show comment
Hide comment
@dzuelke

dzuelke Dec 2, 2014

Contributor

👍

Contributor

dzuelke commented Dec 2, 2014

👍

fabpot added a commit that referenced this pull request Dec 2, 2014

bug #12784 [DependencyInjection] make paths relative to __DIR__ in th…
…e generated container (nicolas-grekas)

This PR was merged into the 2.3 branch.

Discussion
----------

[DependencyInjection] make paths relative to __DIR__ in the generated container

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #6484, #3079, partially #9238, #10894, #10999
| License       | MIT
| Doc PR        | n/a

This is an alternative approach to #10999 for removing absolute paths from the generated container:
instead of trying to fix the container file after it has been dumped, telling to the PhpDumper where its output will be written allows it to replace parts of strings by an equivalent value based on `__DIR__`.
This should be safe, thus the PR is on 2.3.

Commits
-------

edd7057 [DependencyInjection] make paths relative to __DIR__ in the generated container
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment