[DependencyInjection] make paths relative to __DIR__ in the generated container #12784
Conversation
c528404 to
e53b716
Compare
|
Tests added, this PR is ready for review/merge |
61ec8b1 to
bfff3b7
Compare
| $suffix = $matches[0][1] + strlen($matches[0][0]); | ||
| $suffix = isset($value[$suffix]) ? '.'.var_export(substr($value, $suffix), true) : ''; | ||
| $dirname = '__DIR__'; | ||
| for ($i = $this->targetDirMaxMatches - count($matches); 0 <= $i; --$i) { |
There was a problem hiding this comment.
Minor comment: don't you think that a new line above this could improve code legibility?
4a3d116 to
b6d8881
Compare
|
Comments addressed, Windows support should be OK |
| @@ -101,13 +113,13 @@ public function testAddService() | |||
| // without compilation | |||
| $container = include self::$fixturesPath.'/containers/container9.php'; | |||
| $dumper = new PhpDumper($container); | |||
| $this->assertEquals(str_replace('%path%', str_replace('\\','\\\\',self::$fixturesPath.DIRECTORY_SEPARATOR.'includes'.DIRECTORY_SEPARATOR), file_get_contents(self::$fixturesPath.'/php/services9.php')), $dumper->dump(), '->dump() dumps services'); | |||
| $this->assertEquals(str_replace('%path%', str_replace('\\', '\\\\', self::$fixturesPath.DIRECTORY_SEPARATOR.'includes'.DIRECTORY_SEPARATOR), file_get_contents(self::$fixturesPath.'/php/services9.php')), $dumper->dump(), '->dump() dumps services'); | |||
There was a problem hiding this comment.
Is this str_replace still necessary?
There was a problem hiding this comment.
Yes it is: __DIR__ relative paths is an opt-in feature, that is not used here, but only in the new test (see 'file' option when calling dump())
|
👍 I've just tested on Heroku and indeed, that fixes the problem we have currently. ping @dzuelke |
b6d8881 to
b6ade35
Compare
|
Failing tests do not seem to be related to these changes. |
|
As I will have to release 2.6.1 soon, I'd really like to include this PR. ping @symfony/deciders |
|
Does this work properly for people symlinking their cache folder to a different place ? |
|
It will work properly but without |
|
this seems an acceptable option. |
|
Your new test does not rely on any path dumped in service arguments (inlined parameter values). Could you add some to avoid regressions ? |
b6ade35 to
c84d668
Compare
|
Test updated |
| do { | ||
| $rx = preg_quote(DIRECTORY_SEPARATOR.$dir[$i], '#').$rx; | ||
| } while (0 < --$i); | ||
| $this->targetDirRegex = '#'.preg_quote($dir[0], '#').$rx.'#'; |
There was a problem hiding this comment.
will this logic work for the Symfony 3.0 directory structure where the cache is in var/cache/dev/.... rather
8000
than app/cache/dev ?
and I think this deserves a comment explaining the goal of this logic
There was a problem hiding this comment.
comments added, the logic is not specific to any dir scheme, so yes, it will
This PR was merged into the 2.3 branch. Discussion ---------- [DependencyInjection] Perf php dumper | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR came up after this comment to reduce the number of calls to dirname(): #12784 (comment) Commits ------- 375f83e Revert "[DependencyInjection] backport perf optim" fcd8ff9 [DependencyInjection] perf optim: call dirname() at most 5x c11535b [DependencyInjection] backport perf optim
[hack] Hacking jms serialization for heroku [hack] Hacking symfony cache due to symfony/symfony#12893 / see symfony/symfony#12784
[hack] Hacking jms serialization for heroku [hack] Hacking symfony cache due to symfony/symfony#12893 / see symfony/symfony#12784
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.