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.

Sign up for GitHub

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

Jump to bottom

kernel.projectDir must be set when initializing the kernel #29

Merged
merged 5 commits into from
May 19, 2017
Merged

kernel.projectDir must be set when initializing the kernel #29

merged 5 commits into from
May 19, 2017

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented May 8, 2017

fixes #28

@aschempp aschempp added the bug label May 8, 2017
@aschempp aschempp added this to the 4.4.0 milestone May 8, 2017
@aschempp aschempp requested review from ausi and leofeyer May 8, 2017 13:22
@aschempp aschempp changed the base branch from master to develop May 8, 2017 13:22
@ausi
Copy link
Member

ausi commented May 8, 2017

Couldn’t we just use dirname(dirname(dirname(dirname(dirname(__DIR__))))) in getProjectDir() and not inject the path? Symfony itself also doesn’t inject the project dir but searches for the nearest composer.json file.

@aschempp
Copy link
Member Author

aschempp commented May 8, 2017 via email

@ausi
Copy link
Member

ausi commented May 8, 2017

I think since PHP 7 there is no need for caching PHP files by combining them. But even then, the caching script should handle the __DIR__ constants correctly.

ausi
ausi reviewed May 8, 2017
View reviewed changes
src/HttpKernel/ContaoKernel.php Outdated
@@ -64,6 +91,8 @@ public function getRootDir()
* Sets the application root dir.
*
* @param string $dir
*
* @deprecated Deprecated since version 4.4, to be removed in 5.0.
*/
public function setRootDir($dir)
{
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a @trigger_error(, E_USER_DEPRECATED) here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Valid point, added in b1702b9

ausi
ausi requested changes May 10, 2017
View reviewed changes
Copy link
Member

@ausi ausi left a comment

Choose a reason for hiding this comment

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

If we want to go with the constructor argument, I think the following places have to be updated too:

But I would still prefer the dirname() solution.

Also, isn’t changing the constructor arguments a BC break? E.g. what is if I use a custom app_dev.php in my project that uses the ContaoKernel, wouldn’t that break with an update?

@aschempp aschempp mentioned this pull request May 11, 2017
Merged
@aschempp
Copy link
Member Author

I have fixed the remaining uses of ContaoKernel::setProjectDir. Regarding BC break, afaik the constructor is not a BC promise.

@aschempp aschempp requested review from ausi and Toflar May 12, 2017 14:31
@Toflar
Copy link
Member

Toflar commented May 12, 2017

Regarding BC break, afaik the constructor is not a BC promise.

Of course it is. You can only add arguments that are optional.

@leofeyer leofeyer force-pushed the kernel-project-dir branch 2 times, most recently from 3cf9523 to c71190f Compare May 17, 2017 08:36
@leofeyer
Copy link
Member

I have updated the PR so it is backwards compatible. Please review again.

ausi
ausi reviewed May 18, 2017
View reviewed changes
src/HttpKernel/ContaoKernel.php Outdated
public function getProjectDir()
{
if (null === $this->projectDir) {
$this->projectDir = dirname(dirname(dirname(dirname(dirname(__DIR__)))));
Copy link
Member

Choose a reason for hiding this comment

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

With this line, the ContaoKernel now finds the path to the project dir itself. Why do we need the constructor argument then?

Copy link
Member

Choose a reason for hiding this comment

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

For the unit tests I guess.

Copy link
Member

Choose a reason for hiding this comment

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

In the unit tests we could compare the project dir path against the path of the ContaoKernel.php file.

ausi
ausi approved these changes May 18, 2017
View reviewed changes
Copy link
Member

@ausi ausi left a comment

Choose a reason for hiding this comment

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

One parameter doccomment is wrong IMO, the rest looks good to me 👍

src/HttpKernel/ContaoKernel.php Outdated
@@ -37,6 +42,20 @@ class ContaoKernel extends Kernel
private $bundleLoader;

/**
* Constructor.
*
* @param string|null $projectDir
Copy link
Member

Choose a reason for hiding this comment

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

I think null should not be allowed here.

@leofeyer leofeyer self-assigned this May 19, 2017
@leofeyer leofeyer mentioned this pull request May 19, 2017
Closed
@leofeyer leofeyer merged commit 9cd8cdf into develop May 19, 2017
@leofeyer leofeyer deleted the kernel-project-dir branch May 19, 2017 07:16
@leofeyer leofeyer mentioned this pull request May 23, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofeyer leofeyer leofeyer approved these changes

@ausi ausi ausi approved these changes

@Toflar Toflar Awaiting requested review from Toflar

Assignees

@leofeyer leofeyer

Labels
bug
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aschempp @ausi @Toflar @leofeyer