[HttpKernel] remove all deprecated code from http kernel #31672
Merged
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
40ed935 to
0711912
Compare
|
@nicolas-grekas tests are failling because I changed the usage directly in this PR maybe I should do 2 PR ? removed the event and then change the usage ? |
nicolas-grekas
requested changes
May 29, 2019
There was a problem hiding this comment.
Please also turn all @final since Symfony X.Y to just @final,
same for @internal etc
PRs must be green yes. If splitting them makes it easier, then that's the way.
Please be very careful when doing removal PRs, there are many things to take care of (eg see attached review)
src/Symfony/Bundle/FrameworkBundle/EventListener/ResolveControllerNameSubscriber.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/DataCollector/ConfigDataCollector.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/Event/ControllerArgumentsEvent.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Firewall/LegacyListenerTrait.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Firewall/ListenerInterface.php
Outdated
Show resolved
Hide resolved
This was referenced May 29, 2019
[MonologBridge] RouteProcessor class is now final to ease the the removal of deprecated event
#31700
Merged
675aeaa to
5160040
Compare
nicolas-grekas
added a commit
that referenced
this pull request
May 31, 2019
…ase the the removal of deprecated event (Simperfit) This PR was merged into the 4.4 branch. Discussion ---------- [MonologBridge] RouteProcessor class is now final to ease the the removal of deprecated event | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | none <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | none <!-- required for new features --> <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained 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. --> To ease the removal of deprecated events this class has been made final as said in #31672 (comment) Commits ------- 9646364 [MonologBridge] RouteProcessor class is now final to ease the the removal of deprecated event
6cff730 to
1da219c
Compare
1da219c to
e9496ea
Compare
fabpot
added a commit
that referenced
this pull request
Jun 4, 2019
…er:action notation (Simperfit) This PR was merged into the 5.0-dev branch. Discussion ---------- [FrameworkBundle] Remove support for the bundle:controller:action notation | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | none <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR |none no mention in the docs <!-- required for new features --> <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained 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. --> This remove the ResolveControllerNameSubscriber since it is deprecated, it ease the cleaning of HttpKernel see #31672 (comment) Commits ------- 29e3c2f [FrameworkBundle] remove deprecater controller a:b:c notation
|
PR Rebased |
e9496ea to
a2db61a
Compare
fabpot
requested changes
Jun 5, 2019
| @@ -87,9 +87,9 @@ | |||
|
|
|||
| <service id="file_locator" class="Symfony\Component\HttpKernel\Config\FileLocator"> | |||
| <argument type="service" id="kernel" /> | |||
| <argument>%kernel.root_dir%/Resources</argument> | |||
| <argument>%kernel.project_dir%/Resources</argument> | |||
There was a problem hiding this comment.
That looks suspicious: root_dir and project_dir are not equivalent.
There was a problem hiding this comment.
What will be the equivalent since this is to me the only thing I can think of to replace root_dir here?
There was a problem hiding this comment.
I had to look in the code what this does: This is the global directory where resources can be loaded from. %kernel.project_dir%/Resources and %kernel.project_dir% from below are the fallback directories for any resources. So in SF 3 this means
app/Resourcescan be used to overwrite resources from Bundlesapp/can be used to load any resource if they are not found relative to the current directory the source is loaded from
But that behavior has never officially been unsed in a SF 2/3 symfony-standard or SF 4 flex kernel. They are either loaded relative to the current dir or via glob pattern, not by a global fallback.
Overwriting bundle resources should also not be done like this since forever. See https://symfony.com/doc/current/bundles/override.html
It should be done with compiler passes for services or twig namespace (which uses the /templates/ dir) for templates or translation domains for translations.
If we change this to use project_dir, it means the fallback dir is not app/Resources as in SF 3 or src/Resources in SF 4, but Resources/ directly in SF 5. Whatever we put here, it does not make sense and is not documented anywhere.
I suggest to deprecate this global fallback dir in SF 4.4 and remove it in 5. Then this problem is gone.
I already have an idea how to implement the deprecation.
There was a problem hiding this comment.
Thanks for looking into it @Tobion If you have time to implement the deprecation go ahead, if oyu want me to do it, tell me what you think is the best way to do it!
There was a problem hiding this comment.
I extracted this issue into #31915. Just merge it like this now and we can deal with this problem separately.
There was a problem hiding this comment.
I reverted the removal of the kernel.root_dir parameter from this PR so we can work on #31915 before.
a2db61a to
b5e3309
Compare
Tobion
reviewed
Jun 5, 2019
Tobion
reviewed
Jun 5, 2019
b5e3309 to
be0f5ff
Compare
b140913 to
9caa2bf
Compare
nicolas-grekas
added a commit
that referenced
this pull request
Jun 6, 2019
…(Simperfit) This PR was merged into the 4.4 branch. Discussion ---------- Several components are incompatible with HttpKernel 5.0 | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | none <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | none <!-- required for new features --> <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained 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 branch 4.4. - Legacy code removals go to the master branch. --> This will help to finish the HttpKernel dead code removal (#31672) as this conflict with the component since the type-hint of several listener has been updated. Commits ------- a0e4ad3 Several components are incompatible with HttpKernel 5.0
2956955 to
b0884d7
Compare
|
The events test has been fixed, still missing the resolution of #31672 (comment) and the some kernel.root_dir test are broken, this needs to be looked at to maybe raise the dependencies. |
|
To me, the events part should be split in its own PR. |
nicolas-grekas
added a commit
that referenced
this pull request
Jun 8, 2019
…ir use (Simperfit, nicolas-grekas) This PR was merged into the 5.0-dev branch. Discussion ---------- [HttpKernel] remove all deprecated event and replace their use | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | none <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR |to check<!-- required for new features --> <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained 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. --> Extract the whole event & listener part from #31672 (comment) to its own PR to ease the removal of deprecated code from HttpKernel. Commits ------- be5f641 Fix annotations 48073dd [HttpKernel] remove all deprecated event and replace their use
424d6d8 to
4cba074
Compare
nicolas-grekas
approved these changes
Jun 8, 2019
4cba074 to
48351d5
Compare
|
Thank you @Simperfit. |
nicolas-grekas
added a commit
that referenced
this pull request
Jun 8, 2019
… (Simperfit) This PR was merged into the 5.0-dev branch. Discussion ---------- [HttpKernel] remove all deprecated code from http kernel | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | none <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR |to check<!-- required for new features --> <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained 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. --> REmove all deprecated code from HttpKernel, update all listener and all tests Commits ------- 48351d5 [HttpKernel] remove all deprecated code from http kernel
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.
REmove all deprecated code from HttpKernel, update all listener and all tests