8000 [Doc] Add entry for `container.dumper.inline_class_loader` param at `UPGRADE-3.4.md` and `UPGRADE-4.0.md` by phansys · Pull Request #26276 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Doc] Add entry for container.dumper.inline_class_loader param at UPGRADE-3.4.md and UPGRADE-4.0.md #26276

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
Merged
merged 1 commit into from
Mar 2, 2018
Merged

[Doc] Add entry for container.dumper.inline_class_loader param at UPGRADE-3.4.md and UPGRADE-4.0.md #26276

merged 1 commit into from
Mar 2, 2018

Conversation

phansys
Copy link
Contributor
@phansys phansys commented Feb 23, 2018
Q A
Branch 3.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26269
License MIT
Doc PR n/a

@phansys
Copy link
Contributor Author
phansys commented Feb 23, 2018

I don't know if the provided description is enough, please let me know if any change is required.
I think there are no changes for this param at 4.0, so I didn't upgrade UPGRADE-4.0.md at all.

@phansys phansys force-pushed the inline_class_loader_doc branch from 5fec6b7 to 0cab194 Compare February 23, 2018 03:22
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Feb 23, 2018 •
edited
Loading

I think it does make sense also for 4.0 (same reasoning as #26278 vs #26251)

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Feb 23, 2018
@nicolas-grekas
Copy link
Member

Status: needs work

@phansys phansys force-pushed the inline_class_loader_doc branch from 0cab194 to 9e1e21c Compare February 27, 2018 18:50
@phansys phansys changed the title [Doc] Add entry for container.dumper.inline_class_loader param at UPGRADE-3.4.md [Doc] Add entry for container.dumper.inline_class_loader param at UPGRADE-3.4.md and UPGRADE-4.0.md Feb 27, 2018
@phansys
Copy link
Contributor Author
phansys commented Feb 27, 2018

Added entry at UPGRADE-4.0.md.

nicolas-grekas
nicolas-grekas reviewed Feb 28, 2018
View reviewed changes
UPGRADE-4.0.md Outdated
@@ -236,6 +236,9 @@ DependencyInjection

* The `ExtensionCompilerPass` has been moved to before-optimization passes with priority -1000.

* Parameter `container.dumper.inline_class_loader` is configured by default with
Copy link
Member
@nicolas-grekas nicolas-grekas Feb 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

actually, the default is false also
it is true when starting a new Flex or Std edition though (in 3.4 or 4.0)
since this is an UPGRADE file, I'd assume we can always suggest to turn it to true, same message as in 3.4

nicolas-grekas
nicolas-grekas reviewed Feb 28, 2018
View reviewed changes
UPGRADE-3.4.md Outdated
@@ -72,6 +72,15 @@ DependencyInjection
* The `ResolveDefinitionTemplatesPass` class is deprecated and will be removed in 4.0.
Use the `ResolveChildDefinitionsPass` class instead.

* Parameter `container.dumper.inline_class_loader` was added in order to provide
control in the way used to dump the loaded classes. If configured with value `true`,
they will be dumped inline:
Copy link
Member

Choose a reason for hiding this comment

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

Unless you're using a custom autoloader, you should enable the container.dumper.inline_class_loader parameter. This can dramatically improve DX by reducing the time to load classes in the "dev" environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we use "drastically" instead of "dramatically"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think DependencyInjection component doesn't know about environments. Could we replace "classes in the "dev" environment" with something like "classes when the DebugClassLoader is enabled"? However, I don't know if this is the only change which has impact on the performance improvement.

Copy link
Member

Choose a reason for hiding this comment

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

sure :)

Copy link
Member

Choose a reason for hiding this comment

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

Talking about DebugClassLoader might be a bit abstract for ppl, that's why I mentioned the "dev" env.

this is the only change which has impact on the performance improvement

bypassing the "prod" autoloader is also beneficial, but by a quite thin margin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but IMO referencing the "environment" concept here goes against using DependencyInjection component decoupled from the framework.

Copy link
Member

Choose a reason for hiding this comment

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

this is about the parameter, which means configuring the component, thus by definition: for integration with something else
I wouldn't mind personally...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Precisely, something else should be anything consuming this dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reworded the description in order to address your suggestion: https://github.com/symfony/symfony/pull/26276/files#diff-8c6ccda31d4a8303738b07ac0f325881R75

@phansys phansys force-pushed the inline_class_loader_doc branch 2 times, most recently from 3d9a17c to 34dfc84 Compare March 1, 2018 17:32
@phansys phansys force-pushed the inline_class_loader_doc branch from 34dfc84 to 66ea9e0 Compare March 1, 2018 19:28
@phansys phansys force-pushed the inline_class_loader_doc branch from 66ea9e0 to d6e2b81 Compare March 2, 2018 00:43
@nicolas-grekas
Copy link
Member

Thank you @phansys.

@nicolas-grekas nicolas-grekas merged commit d6e2b81 into symfony:3.4 Mar 2, 2018
nicolas-grekas added a commit that referenced this pull request Mar 2, 2018
@nicolas-grekas
minor #26276 [Doc] Add entry for `container.dumper.inline_class_loade…
8f1a6b5 
…r` param at `UPGRADE-3.4.md` and `UPGRADE-4.0.md` (phansys)

This PR was merged into the 3.4 branch.

Discussion
----------

[Doc] Add entry for `container.dumper.inline_class_loader` param at `UPGRADE-3.4.md` and `UPGRADE-4.0.md`

|Q            |A     |
|---          |---   |
|Branch       |3.4   |
|Bug fix?     |no    |
|New feature? |no    |
|BC breaks?   |no    |
|Deprecations?|no    |
|Tests pass?  |yes   |
|Fixed tickets|#26269|
|License      |MIT   |
|Doc PR       |n/a   |

Commits
-------

d6e2b81 Add entry for `container.dumper.inline_class_loader` param at `UPGRADE-3.4.md` and `UPGRADE-4.0.md`
@phansys phansys deleted the inline_class_loader_doc branch March 2, 2018 15:00
@brusch brusch mentioned this pull request Dec 5, 2019
Closed
@nicolas-grekas nicolas-grekas mentioned this pull request Dec 18, 2019
Merged
nicolas-grekas added a commit that referenced this pull request Dec 18, 2019
@nicolas-grekas
bug #35021 [DI] Enable inline_class_loader in debug mode (nicolas-gre…
40c1351 
…kas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[DI] Enable inline_class_loader in debug mode

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Similar to symfony/recipes#713
Check https://twitter.com/nicolasgrekas/status/929032213815005184
and #26276

Commits
-------

317ce6d [DI] Enable inline_class_loader in debug mode
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 approved these changes

@Simperfit Simperfit Simperfit approved these changes

Assignees
No one assigned
Labels
DependencyInjection Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@phansys @nicolas-grekas @Simperfit @xabbuh @carsonbot
0