Skip to content

[DI] Fix Preload #34833

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
Closed
wants to merge 1 commit into from
Closed

[DI] Fix Preload #34833

wants to merge 1 commit into from

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Dec 5, 2019 •
edited
Loading

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

The regexp to targetDir was malformatted when the number of folder was lower than 8
Here is the value of Regexp.

/foo                => /foo/var(/cache(/prod)?)? <=== this is my issue. `var` is compulsory here
/foo/bar            => /foo/bar(/var(/cache(/swoole)?)?)?
/foo/bar/baz        => /foo/bar(/baz(/var(/cache(/swoole)?)?)?)?
/foo/bar/baz/x/y/z: => /foo/bar/baz/x(/y(/z(/var(/cache(/swoole)?)?)?)?)?"

@jderusse jderusse changed the base branch from master to 4.4 December 5, 2019 14:30
@nicolas-grekas
Copy link
Member

I think #34757 fixed the issue already.

@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Dec 5, 2019
@stof
Copy link
Member

stof commented Dec 5, 2019

Wouldn't this cause issues in case multiple Composer class loaders are registered in memory at the time the container is dumped ? This could happen for instance if you run Behat from a phar (which uses the composer ClassLoader inside the phar) or a global composer install (or whatever else custom install also using composer) and the Behat run has to refresh the container cache. This might also happen when using simple-phpunit rather than Behat btw, because it also uses a composer-based installation for PHPUnit but independent from the vendor folder
That's why there was a check to ensure the autoload file being found is actually the one belonging to the project (this check might have been broken, but it is necessary).

@stof
Copy link
Member

stof commented Dec 5, 2019

@nicolas-grekas do we have a test asserting the output of the dumper for the preload.php file ?

@nicolas-grekas
Copy link
Member

@stof yes we do

@jderusse
Copy link
Member Author

jderusse commented Dec 5, 2019

Ohh indeed, My Patch was totally wrong, because the issue was somewhere else

when project is installed in /foo it doesn't work. BUT when it' installed in /foo/bar it's fine.

I update the patch according to the original issue

jderusse
jderusse commented Dec 5, 2019
View reviewed changes
src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
@@ -209,7 +209,7 @@ public function dump(array $options = [])

if (3 <= $i) {
$regex = '';
$lastOptionalDir = $i > 8 ? $i - 5 : 3;
$lastOptionalDir = $i > 8 ? $i - 5 : $i - 3;
Copy link
Member Author

Choose a reason for hiding this comment

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

3 is var, cache, env
I don't get why the projet dir + parent dir are optionnal when the path contains more than 8 directories

@jderusse
Copy link
Member Author

jderusse commented Dec 5, 2019

fixed by #34757

@jderusse jderusse closed this Dec 5, 2019
@jderusse jderusse deleted the preload branch March 5, 2020 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
4 participants
@jderusse @nicolas-grekas @stof @carsonbot