NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[HttpKernel] Deprecate usage of getRootDir() and kernel.root_dir#28810

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromfabpot:root_dir-removal

Oct 16, 2018

Merged

[HttpKernel] Deprecate usage of getRootDir() and kernel.root_dir#28810

fabpot merged 1 commit intosymfony:masterfromfabpot:root_dir-removal

Oct 16, 2018

Conversation

Copy link

Member

fabpot commentedOct 10, 2018•
edited
Loading

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

carsonbot added the Status: Needs Review label

Oct 10, 2018

fabpot commented

Oct 10, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Templating/Helper/CodeHelper.php

		* @param string $charset The charset
		*/
		publicfunction__construct($fileLinkFormat,string$rootDir,string$charset)
		publicfunction__construct($fileLinkFormat,string$projectDir,string$charset)

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

We already injectkernel.project_dir in the container definition.

fabpot force-pushed theroot_dir-removal branch 2 times, most recently from799ef32 to086826eCompare

October 10, 2018 16:10

Copy link

Contributor

ogizanagi commentedOct 10, 2018

You meantkernel.root_dir :)

- Remove internal usage of kernel.project_dir when possible+ Remove internal usage of kernel.root_dir when possible

fabpot changed the title~~Remove internal usage of kernel.project_dir when possible~~Remove internal usage of kernel.root_dir when possible

Oct 10, 2018

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

should the new property be private instead ? I don't think it needs to be an extension point

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

of course, done

src/Symfony/Component/HttpKernel/Kernel.php Outdated


		returnarray(
		/**
		* @deprecated since Symfony 3.3

Copy link

Member

Choose a reason for hiding this comment

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

version looks wrong (otherwise, it would already be gone in 4.0).

And for that one, being able to warn when using the parameter looks useful.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

version is the right one, it was deprecated in 3.3, but we did not add the deprecation notice and didn't find time/energy/way to make it before 4.0. I can change to 4.2, no big deal.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Moved to 4.2.

fabpot force-pushed theroot_dir-removal branch 4 times, most recently fromd8188bc to78416adCompare

October 10, 2018 17:29

fabpot changed the title~~Remove internal usage of kernel.root_dir when possible~~[HttpKernel] Deprecate usage of getRootDir() and kernel.root_dir

Oct 10, 2018

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

This is the default value in the Symfony Flex recipe.

fabpot commented

Oct 10, 2018

View reviewed changes

src/Symfony/Component/HttpKernel/Kernel.php

		publicfunctiongetLogDir()
		{
		return$this->rootDir.'/logs';
		return$this->getProjectDir().'/var/log';

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

This is the default value in the Symfony Flex recipe.

fabpot force-pushed theroot_dir-removal branch from78416ad to5129d63Compare

October 10, 2018 17:32

OskarStark reviewed

Oct 10, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Tests/Command/CacheClearCommand/CacheClearCommandTest.php OutdatedShow resolvedHide resolved

carsonbot removed the Status: Needs Review label

Oct 10, 2018

OskarStark approved these changes

Oct 10, 2018

View reviewed changes

carsonbot added the Status: Reviewed label

Oct 10, 2018

ogizanagi added the Deprecation label

Oct 10, 2018

ro0NL reviewed

Oct 11, 2018

View reviewed changes

Copy link

Contributor

ro0NL left a comment

Choose a reason for hiding this comment

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

fixes#24293

src/Symfony/Component/HttpKernel/KernelInterface.php OutdatedShow resolvedHide resolved

chalasr reviewed

Oct 11, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/TranslationUpdateCommand.phpShow resolvedHide resolved

fabpot force-pushed theroot_dir-removal branch 2 times, most recently from11c7519 toefffda4Compare

October 11, 2018 11:46

nicolas-grekas added this to thenext milestone

Oct 14, 2018

fabpot force-pushed theroot_dir-removal branch fromefffda4 tofb5688eCompare

October 15, 2018 15:52

ro0NL reviewed

Oct 15, 2018

View reviewed changes

Copy link

Contributor

ro0NL left a comment

Choose a reason for hiding this comment

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

UPGRADE files need to be updated

given the parameters are not deprecated in code (trigger error) i suggest a mention to manually search your code base

for UPGRADE-5.0 these should be listed as BC break (preferably put on top)

src/Symfony/Component/HttpKernel/CHANGELOG.md OutdatedShow resolvedHide resolved

Copy link

Contributor

ro0NL commentedOct 16, 2018•
edited
Loading

@fabpot can you clarify why deprecatingKernel::getRootDir() but notKernelInterface::getRootDir(), this looks odd.

Do you aim to rename KernelInterface::getRootDir() to getProjectDir() on the interface?

edit: wait.. in 5.0KernelInterface::getRootDir() will return the project dir at implementation level i guess :) cool 👍

Copy link

MemberAuthor

@ro0NL It's more pragmatic than that :) As we are usinggetRootDir() in our own code (translation commands), having@deprecatedmeans that there is no way to be deprecation-free... unless I'm missing something of course.

Copy link

Member

chalasr commentedOct 16, 2018

IIRC the debug component mutes deprecations from the same vendor so implementing getRootDir() should keep the core deprecation free.@nicolas-grekas am I wrong?

Copy link

Contributor

ro0NL commentedOct 16, 2018•
edited
Loading

i was assuming the same yes :)

in 5.0 KernelInterface::getRootDir() will return the project dir at implementation level

should we actually consider this? Does it make sense? We could benefit from it, as we cant addgetProjectDir() to the interface. But this means another big step in 5.0 (basically favoringKernel::getRootDir() again instead ofgetProjectDir() 😂) Not sure it's worth it actually =/

But then we need to decide what's the purpose ofKernelInterface::getRootDir() in 5.0

Copy link

Member

nicolas-grekas commentedOct 16, 2018•
edited
Loading

IIRC the debug component mutes deprecations from the same vendor

I don't think so - that would prevent detecting when we use our own internal deprecated code actually.

5.0 KernelInterface::getRootDir() will return the project dir

no way, as you spotted :) in fact,getProjectDir() is used externally only once in the core, inAboutCommand. This means there is no need to have it on the interface to me.

we are using getRootDir() in our own code (translation commands),

how will this be implemented in 5.0?
do we want to deprecate using these folders?

nicolas-grekas mentioned this pull request

Oct 16, 2018

[HttpKernel] fix deprecating KernelInterface::getRootDir()#28890

Merged

Copy link

MemberAuthor

fabpot commentedOct 16, 2018

ThegetRootDir usages are for legacy directories that are not relevant in Symfony 4, that we need/can remove in 5.0.

Copy link

Member

nicolas-grekas commentedOct 16, 2018•
edited
Loading

This means we need to trigger a deprecation whenever these dirs contain anything that we load currently, right?
Anyone willing to give it a try?

Copy link

MemberAuthor

fabpot commentedOct 16, 2018

It was not possible in 3.4, but we can definitely do it in 4.2.

Copy link

Member

yceruto commentedOct 16, 2018

We talking about these directories%kernel.root_dir%/Resources/views and%kernel.root_dir%/Resources/translations right? I can takeTwigBundle by now, maybe translations commands too.

Copy link

Member

chalasr commentedOct 16, 2018

@yceruto I already started the work for translation commands locally :)

yceruto mentioned this pull request

Oct 16, 2018

[TwigBundle] Deprecating support for legacy templates directories#28891

Merged

1 task

fabpot added a commit that referenced this pull request

Oct 16, 2018

minor#28890[HttpKernel] fix deprecating KernelInterface::getRootDir…

557f85d

…() (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[HttpKernel] fix deprecating KernelInterface::getRootDir()| Q             | A| ------------- | ---| Branch?       | 4.2| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Follow up of#28810.Did I get it right?Commits-------819f525 [HttpKernel] fix deprecating KernelInterface::getRootDir()

ro0NL mentioned this pull request

Oct 17, 2018

[HttpKernel] Added KernelInterface::getProjectDir()symfony/symfony-docs#10503

Closed

fabpot added a commit that referenced this pull request

Oct 21, 2018

feature#28891[TwigBundle] Deprecating support for legacy templates …

c620a3b

…directories (yceruto)This PR was merged into the 4.2-dev branch.Discussion----------[TwigBundle] Deprecating support for legacy templates directories| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        |symfony/symfony-docs#10500go ahead with#28810 (comment)- [x] Fix testsCommits-------8b390f3 Deprecating support for legacy templates directories

yceruto mentioned this pull request

Oct 26, 2018

[FrameworkBundle] Deprecating support for legacy translations directory#28997

Merged

fabpot added a commit that referenced this pull request

Oct 27, 2018

feature#28997[FrameworkBundle] Deprecating support for legacy trans…

c333d85

…lations directory (yceruto)This PR was merged into the 4.2-dev branch.Discussion----------[FrameworkBundle] Deprecating support for legacy translations directory| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#28810 (comment)| License       | MIT| Doc PR        | TODOCommits-------acdece7 Deprecating support for legacy translations directory

jvasseur mentioned this pull request

Oct 30, 2018

[HttpKernel] Fix BC break in Kernel name#29032

Merged

Tobion added a commit that referenced this pull request

Oct 31, 2018

minor#29032[HttpKernel] Fix BC break in Kernel name (jvasseur)

887af35

This PR was merged into the 4.2-dev branch.Discussion----------[HttpKernel] Fix BC break in Kernel name| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Fix a BC break introduced by#28810 where the name of the kernel where change form the name of the folder containing the kernel class to the name of the folder containing the project.This introduced a bug with deployment processes where the cache warmup is done before moving the application to a folder with a different name and removing the possibility to compile the container (either by moving to a read-only filesystem or by removing the config directory).Commits-------872a772 Fix BC break in Kernel name

nicolas-grekas modified the milestones:next,4.2

Nov 1, 2018

This was referencedNov 3, 2018

Release v4.2.0-BETA1#29072

Closed

Release v4.2.0-BETA1#29073

Merged

fabpot deleted the root_dir-removal branch

January 14, 2019 11:01

phansys mentioned this pull request

Apr 5, 2019

[Flex] Tell thatvar/logs/ directory was renamedsymfony/symfony-docs#11299

Closed

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request

Apr 5, 2019

minor#11299[Flex] Tell thatvar/logs/directory was renamed (phan…

2b0aec2

…sys)This PR was submitted for the master branch but it was merged into the 4.2 branch instead (closes#11299).Discussion----------[Flex] Tell that `var/logs/` directory was renamedRef:symfony/symfony#28810 (comment)Commits-------2fcd9bc [Flex] Tell that `var/logs/` directory was renamed

jzawadzki mentioned this pull request

Nov 27, 2019

[minor] remove usage of Kernel::$rootDir from docssymfony/symfony-docs#12719

Merged

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request

Nov 30, 2019

minor#12719[minor] remove usage of Kernel::$rootDir from docs (jzaw…

a9aa86a

…adzki)This PR was submitted for the 5.0 branch but it was squashed and merged into the 4.3 branch instead (closes#12719).Discussion----------[minor] remove usage of Kernel::$rootDir from docsAs persymfony/symfony#28810 $rootDir has been removed in 5.0 - use getProjectDir in docs instead - small change to be consistentCommits-------9da96e1 [minor] remove usage of Kernel::$rootDir from docs

This was referencedJan 1, 2020

Symfony 5 parameter fixdoctrine/DoctrineMigrationsBundle#295

Closed

Symfony 5 parameter doc fixschmittjoh/JMSSerializerBundle#783

Merged