Skip to content

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

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 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Jun 20, 2019 •
edited by nicolas-grekas
Loading

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 June 20, 2019 15:13
@lyrixx lyrixx changed the title [Kernel+DIC] Fixed as_files habavior [Kernel+DIC] Fixed as_files behavior Jun 20, 2019
nicolas-grekas
nicolas-grekas reviewed Jun 20, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

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

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 20, 2019
lyrixx
lyrixx commented Jul 1, 2019
View reviewed changes
Copy link
Member Author

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

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/

@lyrixx lyrixx force-pushed the dic-no-files branch 3 times, most recently from 4ab8996 to 32563e5 Compare July 1, 2019 13:54
@lyrixx
Copy link
Member Author

lyrixx commented Jul 1, 2019

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

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

lyrixx commented Jul 4, 2019

Are you OK with this one?

nicolas-grekas
nicolas-grekas reviewed Jul 4, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

still for 4.4 to me, sorry about that :)

@lyrixx lyrixx requested review from dunglas, sroze and xabbuh as code owners July 8, 2019 15:30
@lyrixx lyrixx changed the base branch from 3.4 to 4.4 July 8, 2019 15:30
@lyrixx
Copy link
Member Author

lyrixx commented Jul 8, 2019 •
edited
Loading

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 2 commits July 16, 2019 10:45
@lyrixx
[Kernel+DIC] Fixed as_files behavior
312e6e9 
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
Simplified code
45bf554
@lyrixx
Copy link
Member Author

lyrixx 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 dic-no-files branch 3 times, most recently from c95a05a to d5702c3 Compare July 17, 2019 12:08
@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
Copy link
Member

nicolas-grekas commented Jul 17, 2019 •
edited
Loading

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 nicolas-grekas force-pushed the dic-no-files branch 8 times, most recently from a62b056 to 45bf554 Compare July 17, 2019 15:56
@nicolas-grekas nicolas-grekas mentioned this pull request Jul 17, 2019
Merged
@nicolas-grekas
Copy link
Member

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

@lyrixx
Copy link
Member Author

lyrixx commented Jul 17, 2019

@nicolas-grekas What about the cleaning ?

nicolas-grekas added a commit that referenced this pull request Jul 31, 2019
@nicolas-grekas
feature #32581 [DI] Allow dumping the container in one file instead o…
85058f5 
…f many files (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

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

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

Replaces #32119
As spotted by @lyrixx, putting all service factories in one big container can be easier to manage with workers. It could also play well with PHP7.4's preloading.

This PR adds a `container.dumper.inline_factories` parameter to enable this behavior.
When it is set to true, a single big container file is created.

Commits
-------

c893986 [DI] Allow dumping the container in one file instead of many files
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
@lyrixx lyrixx deleted the dic-no-files branch May 12, 2021 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@dunglas dunglas Awaiting requested review from dunglas

@sroze sroze Awaiting requested review from sroze

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Feature Status: Needs Review
Projects
None yet
Milestone
4.4
Development

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