Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DI] Allow dumping the container in one file instead of many files #32119

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@lyrixx
Copy link
Member

commented Jun 20, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? -
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Replaced by #32581

@lyrixx lyrixx requested a review from nicolas-grekas Jun 20, 2019

@lyrixx lyrixx changed the title [Kernel+DIC] Fixed as_files habavior [Kernel+DIC] Fixed as_files behavior Jun 20, 2019

@nicolas-grekas
Copy link
Member

left a comment

That's for 4.4, it's a new feature :)

Show resolved Hide resolved src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpKernel/Kernel.php Outdated

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 20, 2019

@lyrixx lyrixx force-pushed the lyrixx:dic-no-files branch from e109e8a to 39113e2 Jul 1, 2019

@lyrixx
Copy link
Member Author

left a comment

Ok, I have reworked this PR. there was a bug (did not worked in env test), now it's 👍
Bonus: Now it's only affect the kernel, not the DIC \o/

Show resolved Hide resolved src/Symfony/Component/HttpKernel/Kernel.php Outdated
Show resolved Hide resolved src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php Outdated

@lyrixx lyrixx force-pushed the lyrixx:dic-no-files branch 3 times, most recently from 4ab8996 to 32563e5 Jul 1, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

The diff is easier to read thank to ?w=1

@lyrixx lyrixx force-pushed the lyrixx:dic-no-files branch from 32563e5 to 5d5ff11 Jul 1, 2019

@lyrixx lyrixx changed the title [Kernel+DIC] Fixed as_files behavior [Kernel] Fixed as_files behavior Jul 2, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

Are you OK with this one?

@nicolas-grekas
Copy link
Member

left a comment

still for 4.4 to me, sorry about that :)

Show resolved Hide resolved src/Symfony/Component/HttpKernel/Kernel.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpKernel/Kernel.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpKernel/Kernel.php
Show resolved Hide resolved src/Symfony/Component/HttpKernel/Kernel.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpKernel/Kernel.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpKernel/Kernel.php

@lyrixx lyrixx force-pushed the lyrixx:dic-no-files branch from 5d5ff11 to 7da6759 Jul 8, 2019

@lyrixx lyrixx requested review from dunglas, sroze and xabbuh as code owners Jul 8, 2019

@lyrixx lyrixx changed the base branch from 3.4 to 4.4 Jul 8, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

OK, I have added a test, and addressed your comments.
I also added an extra commit where I simplified the code. (about fresh / oldContainer) May be I miss something, but I think you are wrong, and the test suite is still green. Anyway, It's an extra commit, so I would remove it easily!
So it could be better to review the first commit first

lyrixx added some commits Jun 20, 2019

[Kernel+DIC] Fixed as_files behavior
Many issue about this:

* Kernel does not handle when the dumper returns a string
* Kernel does not handle when the compiled Container file does not
return a new instance of itself
* Kernel try to old load and clean old container, but if the container
does not change its name, it tries to load 2 time the same file, and so
the same class

This patch fixed all theses case. Now we can use with ease a multifiles
container or a simple unique file.

@lyrixx lyrixx force-pushed the lyrixx:dic-no-files branch from e726221 to 45bf554 Jul 16, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

I rebased this PR. It's now ready and green 💚

@nicolas-grekas nicolas-grekas changed the title [Kernel] Fixed as_files behavior [HttpKernel] Allow dumping the container in one file instead of many files Jul 16, 2019

@lyrixx lyrixx added Feature and removed Bug labels Jul 17, 2019

@nicolas-grekas nicolas-grekas force-pushed the lyrixx:dic-no-files branch 3 times, most recently from c95a05a to d5702c3 Jul 17, 2019

@nicolas-grekas nicolas-grekas changed the title [HttpKernel] Allow dumping the container in one file instead of many files [DI] Allow dumping the container in one file instead of many files Jul 17, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

I pushed a completely different implementation on your branch: instead of patching the Kernel (I kept your cleanup), this now patches the PhpDumper. This allows keeping the anonymous class proxy, which is desired to have safe cache clears. See updated PR description.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

Replaced by #32581
Thanks @lyrixx for raising the issue and giving it a try.

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

@nicolas-grekas What about the cleaning ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.