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

[HttpKernel] remove all deprecated code from http kernel #31672

Conversation

Projects
None yet
6 participants
@Simperfit
Copy link
Contributor

commented May 29, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR to check

REmove all deprecated code from HttpKernel, update all listener and all tests

@Simperfit Simperfit force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch from 40ed935 to 0711912 May 29, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@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 nicolas-grekas added this to the 5.0 milestone May 29, 2019

@nicolas-grekas
Copy link
Member

left a 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)

@Simperfit Simperfit force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch 2 times, most recently from 675aeaa to 5160040 May 31, 2019

nicolas-grekas added a commit that referenced this pull request May 31, 2019

feature #31700 [MonologBridge] RouteProcessor class is now final to e…
…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

@Simperfit Simperfit force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch 5 times, most recently from 6cff730 to 1da219c May 31, 2019

@Simperfit Simperfit force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch from 1da219c to e9496ea Jun 4, 2019

fabpot added a commit that referenced this pull request Jun 4, 2019

minor #31702 [FrameworkBundle] Remove support for the bundle:controll…
…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🅱️c notation
@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

PR Rebased

@Simperfit Simperfit force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch from e9496ea to a2db61a Jun 5, 2019

Show resolved Hide resolved UPGRADE-5.0.md Outdated
Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md Outdated
@@ -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>

This comment has been minimized.

Copy link
@fabpot

fabpot Jun 5, 2019

Member

That looks suspicious: root_dir and project_dir are not equivalent.

This comment has been minimized.

Copy link
@Simperfit

Simperfit Jun 5, 2019

Author Contributor

What will be the equivalent since this is to me the only thing I can think of to replace root_dir here?

This comment has been minimized.

Copy link
@Tobion

Tobion Jun 5, 2019

Member

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/Resources can be used to overwrite resources from Bundles
  • app/ 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.

This comment has been minimized.

Copy link
@Simperfit

Simperfit Jun 5, 2019

Author Contributor

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!

This comment has been minimized.

Copy link
@Tobion

Tobion Jun 6, 2019

Member

I extracted this issue into #31915. Just merge it like this now and we can deal with this problem separately.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 8, 2019

Member

I reverted the removal of the kernel.root_dir parameter from this PR so we can work on #31915 before.

Show resolved Hide resolved src/Symfony/Component/HttpKernel/CHANGELOG.md Outdated

@Simperfit Simperfit force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch from a2db61a to b5e3309 Jun 5, 2019

@Simperfit Simperfit force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch from b5e3309 to be0f5ff Jun 5, 2019

@Simperfit Simperfit force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch 3 times, most recently from b140913 to 9caa2bf Jun 6, 2019

nicolas-grekas added a commit that referenced this pull request Jun 6, 2019

minor #31890 Several components are incompatible with HttpKernel 5.0 …
…(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

@Simperfit Simperfit force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch 3 times, most recently from 2956955 to b0884d7 Jun 6, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

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.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

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

minor #31918 [HttpKernel] remove all deprecated event and replace the…
…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

@nicolas-grekas nicolas-grekas force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch 3 times, most recently from 424d6d8 to 4cba074 Jun 8, 2019

@nicolas-grekas nicolas-grekas force-pushed the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch from 4cba074 to 48351d5 Jun 8, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Thank you @Simperfit.

@nicolas-grekas nicolas-grekas merged commit 48351d5 into symfony:master Jun 8, 2019

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jun 8, 2019

minor #31672 [HttpKernel] remove all deprecated code from http kernel…
… (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

@Simperfit Simperfit deleted the Simperfit:deprecated/remove-deprecated-code-from-http-kernel branch Jun 8, 2019

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.