[Routing] allow using compiled matchers and generators without dumping PHP code #28865
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1012b2a to
1d5e088
Compare
Shine-neko
reviewed
Oct 15, 2018
f26ec0e to
51cdea8
Compare
|
Ready, failure unrelated. As is, this PR will enable the much faster CompiledUrlMatcher/Generator by default, even when using a non-dumped one. |
stof
reviewed
Oct 16, 2018
src/Symfony/Bundle/FrameworkBundle/Routing/RedirectableCompiledUrlMatcher.php
Show resolved
Hide resolved
src/Symfony/Component/Routing/Matcher/Dumper/CompiledUrlMatcherDumper.php
Outdated
Show resolved
Hide resolved
51cdea8 to
46819ec
Compare
stof
approved these changes
Oct 17, 2018
This was referenced Nov 6, 2018
javiereguiluz
requested changes
Nov 7, 2018
There was a problem hiding this comment.
Thanks for this! In order to be mergeable, could you please update the PR description to add a simple example that uses this feature? I guess this is only for the stand-alone component, not full Symfony apps. Thanks!
Note: I'm "blocking" this PR with a "Request Changes" review as suggested by the Symfony community on Symfony Slack. Some people suggested to "block" features until they are documented ... so I'm trying that to see how it goes. Thanks!
0e530fd to
8ed58fa
Compare
8ed58fa to
80c6d28
Compare
|
Without more feedback, I'm going to merge this PR this week. |
|
Can you rebase please? |
d5be794 to
f0a519a
Compare
|
rebased! |
nicolas-grekas
added a commit
that referenced
this pull request
Jan 26, 2019
… without dumping PHP code (nicolas-grekas) This PR was merged into the 4.3-dev branch. Discussion ---------- [Routing] allow using compiled matchers and generators without dumping PHP code | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #29590 | License | MIT | Doc PR | symfony/symfony-docs#10790 This is a resurrection of #25909 to make matcher+generator dumpers output PHP arrays instead of PHP code. Don't be fooled by the diff stats, it's mostly fixtures. This PR should contribute to making the Routing component easier to use standalone. On the way back from SFLive USA.  Commits ------- f0a519a [Routing] allow using compiled matchers and generators without dumping PHP code
javiereguiluz
added a commit
to symfony/symfony-docs
that referenced
this pull request
Jan 31, 2019
… (nicolas-grekas) This PR was merged into the master branch. Discussion ---------- Doc CompiledUrlMatcherDumper instead of PhpMatcherDumper Needed after symfony/symfony#28865 Commits ------- 8e8df56 Doc CompiledUrlMatcherDumper instead of PhpMatcherDumper
|
Late review:: Looks fine to me. Being able to achieve the same thing by only dumping the data structure instead of the executing code makes it a little cleaner and more flexible. |
|
One thing though: We should deprecate |
|
I might be able to fix this tomorrow. |
fabpot
added a commit
that referenced
this pull request
Feb 14, 2019
This PR was merged into the 4.3-dev branch. Discussion ---------- [Routing] fix changelog typo | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | | License | MIT | Doc PR | Fix small typos from #28865 Commits ------- 37dbab2 [Routing] fix changelog typos
symfony-splitter
pushed a commit
to symfony/framework-bundle
that referenced
this pull request
Feb 21, 2019
This PR was merged into the 4.3-dev branch. Discussion ---------- [Routing] deprecate some router options | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | symfony/symfony#28865 (comment) | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> <!-- Write a short README entry for your feature/bugfix here (replace this comment block.) This will help people understand your PR and can be used as a start of the Doc PR. Additionally: - Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the master branch. --> Commits ------- bf4cd6164d [Routing] deprecate some router options
fabpot
added a commit
that referenced
this pull request
Feb 21, 2019
This PR was merged into the 4.3-dev branch. Discussion ---------- [Routing] deprecate some router options | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #28865 (comment) | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> <!-- Write a short README entry for your feature/bugfix here (replace this comment block.) This will help people understand your PR and can be used as a start of the Doc PR. Additionally: - Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the master branch. --> Commits ------- bf4cd61 [Routing] deprecate some router options
Merged
nicolas-grekas
added a commit
that referenced
this pull request
Sep 6, 2019
…Fayet) This PR was merged into the 4.3 branch. Discussion ---------- [Router] routing cache crash when using generator_class | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #31807 | License | MIT Since #28865 the Router use, by default, new generator, matcher, and dumpers. This leads to crash when the Router use a custom generator, or matcher based on the old ones. Commits ------- a5b46e5 Fix routing cache broken when using generator_class
fabpot
added a commit
that referenced
this pull request
Sep 23, 2019
This PR was merged into the 4.3 branch. Discussion ---------- Fix version typo in deprecation notice | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | N/A | License | MIT | Doc PR | N/A #28865 was merged into 4.3 Commits ------- bd13271 Fix version typo in deprecation notice
nicolas-grekas
added a commit
that referenced
this pull request
Nov 4, 2019
…eMC) This PR was merged into the 4.3 branch. Discussion ---------- [Routing] Fix URL generator instantiation | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | License | MIT After merging my fix (PR #31023) from the 4.2 branch to 4.3 (which was master at the time) one code path which instantiates the url generator was missed. This new code path was added via #28865 and was missed during the merge because there was no merge conflict there as that code path didn't exist on 4.2. So the current state on the 4.3 branch is: - https://github.com/symfony/symfony/blob/v4.3.5/src/Symfony/Component/Routing/Router.php#L356 (default locale is properly passed to the constructor) - https://github.com/symfony/symfony/blob/v4.3.5/src/Symfony/Component/Routing/Router.php#L372 (default locale is **NOT** properly passed to the constructor) - https://github.com/symfony/symfony/blob/v4.3.5/src/Symfony/Component/Routing/Router.php#L378 (default locale is properly passed to the constructor) I noticed this in my app because tests started to fail on the branch where we are upgrading our app from SF 4.2 to 4.3 due to this. This PR fixes the inconsistency (now the default locale is passed on all three places) which fixes the bug. Commits ------- 9aa66e2 Add tests to ensure defaultLocale is properly passed to the URL generator 16c9baf Fix URL generator instantiation
fabpot
added a commit
that referenced
this pull request
Aug 11, 2020
…ault)
This PR was squashed before being merged into the 5.2-dev branch.
Discussion
----------
[Serializer] Add CompiledClassMetadataFactory
| Q | A
| ------------- | ---
| Branch?
9C83
| master
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | ?
| License | MIT
| Doc PR | todo (issue: symfony/symfony-docs#10706)
This introduce a dumped `ClassMetadataFactoryInterface` implementation to speed up the serializer by leveraging [PHP7 immutable array](https://blog.blackfire.io/php-7-performance-improvements-immutable-arrays.html).
Like for #28865, if the user have the opcache extension enabled, the compilation time will be skipped. The user will also have a performance boost when not using opcache as we are no longer fetching `ClassMetadata` from the PSR-6 cache.
This allow to speed up the normalization (without opcache) by 9-12% depending on how many objects are involved in the graph:
- [SymfonyObjectNormalizerBenchmark, 100 iterations with complexity of 1](https://blackfire.io/profiles/compare/d937a9cc-eebf-47eb-be90-c8e65cdf12b3/graph)
- [SymfonyObjectNormalizerBenchmark, 3 iterations with complexity of 60](https://blackfire.io/profiles/compare/d490542c-9a79-48a0-b7bc-1ed3ca6a9148/graph)
On the `FrameworkBundle` side, I suggest to add a `CacheWarmer` to dump the metadata array from configured class list. The list could have a _good_ default which will load the classes found in `src/Entity`.
## Dumping the `ClassMetadata`
```php
$classMetadatas = [];
foreach([Category::class, Comment::class, Forum::class, Thread::class, User::class] as $class) {
$classMetadatas[] = $this->classMetadataFactory->getMetadataFor($class);
}
file_put_contents('dumped.php', $this->classMetadataFactoryCompiler->compile($classMetadatas));
```
## Using the dumped `ClassMetadata`
```php
$classMetadataFactory = new CompiledClassMetadataFactory(
'dumped.php',
new CacheClassMetadataFactory(
new ClassMetadataFactory(new AnnotationLoader(new AnnotationReader())),
new ApcuAdapter('SymfonyMetadata')
)
);
```
# To do
- [x] Tests.
- [x] Cache warmer.
- [x] Documentation.
- [x] Changelog entry.
Commits
-------
63cbf0a [Serializer] Add CompiledClassMetadataFactory
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a resurrection of #25909 to make matcher+generator dumpers output PHP arrays instead of PHP code.
Don't be fooled by the diff stats, it's mostly fixtures.
This PR should contribute to making the Routing component easier to use standalone.
On the way back from SFLive USA.