Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@Simperfit
Copy link
Contributor

@SimperfitSimperfit commentedMay 29, 2019•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsnone
LicenseMIT
Doc PRto check

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

@SimperfitSimperfitforce-pushed thedeprecated/remove-deprecated-code-from-http-kernel branch from40ed935 to0711912CompareMay 29, 2019 06:25
@Simperfit
Copy link
ContributorAuthor

@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-grekasnicolas-grekas added this to the5.0 milestoneMay 29, 2019
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

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)

@SimperfitSimperfitforce-pushed thedeprecated/remove-deprecated-code-from-http-kernel branch 2 times, most recently from675aeaa to5160040CompareMay 31, 2019 08:43
nicolas-grekas added a commit that referenced this pull requestMay 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     <!-- seehttps://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 peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://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
@SimperfitSimperfitforce-pushed thedeprecated/remove-deprecated-code-from-http-kernel branch 5 times, most recently from6cff730 to1da219cCompareJune 1, 2019 08:58
@SimperfitSimperfitforce-pushed thedeprecated/remove-deprecated-code-from-http-kernel branch from1da219c toe9496eaCompareJune 4, 2019 10:25
fabpot added a commit that referenced this pull requestJun 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     <!-- seehttps://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 peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://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
@Simperfit
Copy link
ContributorAuthor

PR Rebased

@SimperfitSimperfitforce-pushed thedeprecated/remove-deprecated-code-from-http-kernel branch frome9496ea toa2db61aCompareJune 5, 2019 05:31
<serviceid="file_locator"class="Symfony\Component\HttpKernel\Config\FileLocator">
<argumenttype="service"id="kernel" />
<argument>%kernel.root_dir%/Resources</argument>
<argument>%kernel.project_dir%/Resources</argument>
Copy link
Member

Choose a reason for hiding this comment

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

That looks suspicious:root_dir andproject_dir are not equivalent.

Copy link
ContributorAuthor

@SimperfitSimperfitJun 5, 2019•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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. Seehttps://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 notapp/Resources as in SF 3 orsrc/Resources in SF 4, butResources/ 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.

Simperfit reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

@TobionTobionJun 6, 2019•
edited
Loading

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

@SimperfitSimperfitforce-pushed thedeprecated/remove-deprecated-code-from-http-kernel branch froma2db61a tob5e3309CompareJune 5, 2019 06:29
@SimperfitSimperfitforce-pushed thedeprecated/remove-deprecated-code-from-http-kernel branch 3 times, most recently fromb140913 to9caa2bfCompareJune 6, 2019 18:22
nicolas-grekas added a commit that referenced this pull requestJun 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     <!-- seehttps://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 peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://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
@SimperfitSimperfitforce-pushed thedeprecated/remove-deprecated-code-from-http-kernel branch 3 times, most recently from2956955 tob0884d7CompareJune 6, 2019 20:01
@Simperfit
Copy link
ContributorAuthor

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
Copy link
Member

To me, the events part should be split in its own PR.

Simperfit reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestJun 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     <!-- seehttps://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 peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://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 annotations48073dd [HttpKernel] remove all deprecated event and replace their use
@nicolas-grekasnicolas-grekasforce-pushed thedeprecated/remove-deprecated-code-from-http-kernel branch 3 times, most recently from424d6d8 to4cba074CompareJune 8, 2019 14:09
@nicolas-grekasnicolas-grekasforce-pushed thedeprecated/remove-deprecated-code-from-http-kernel branch from4cba074 to48351d5CompareJune 8, 2019 14:24
@nicolas-grekas
Copy link
Member

Thank you@Simperfit.

@nicolas-grekasnicolas-grekas merged commit48351d5 intosymfony:masterJun 8, 2019
nicolas-grekas added a commit that referenced this pull requestJun 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     <!-- seehttps://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 peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://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 testsCommits-------48351d5 [HttpKernel] remove all deprecated code from http kernel
@SimperfitSimperfit deleted the deprecated/remove-deprecated-code-from-http-kernel branchJune 8, 2019 14:53
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@chalasrchalasrchalasr left review comments

@fabpotfabpotfabpot requested changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@TobionTobionTobion left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

6 participants

@Simperfit@nicolas-grekas@fabpot@Tobion@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp