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] Fix kernel.project_dir extensibility #22728

Merged
merged 1 commit into from May 25, 2017

Conversation

Projects
None yet
@chalasr
Member

chalasr commented May 17, 2017

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

Alternative to #22727 that makes use of the existing public api.

@xabbuh

xabbuh approved these changes May 17, 2017

👍

@aschempp

This comment has been minimized.

Show comment
Hide comment
@aschempp

aschempp May 17, 2017

Contributor

This would be inconsistent with the rootDir imho?

Contributor

aschempp commented May 17, 2017

This would be inconsistent with the rootDir imho?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 17, 2017

Contributor

yes - but we dont care :)

if you want consistency use the method as extension hook for both.

Contributor

ro0NL commented May 17, 2017

yes - but we dont care :)

if you want consistency use the method as extension hook for both.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 17, 2017

Member

Yes it's inconsistent, but allows for better maintainability and user experience at the end. There is a lot protected members in the core codebase, but since a long time, we do not introduce new ones without a strong reason, and we even made some private afterwards (generally through "opportunity").
Also, getProjectDir() makes rootDir obsolete, we should remove it on the long run, no need for changing this now.

Member

chalasr commented May 17, 2017

Yes it's inconsistent, but allows for better maintainability and user experience at the end. There is a lot protected members in the core codebase, but since a long time, we do not introduce new ones without a strong reason, and we even made some private afterwards (generally through "opportunity").
Also, getProjectDir() makes rootDir obsolete, we should remove it on the long run, no need for changing this now.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
Member

ogizanagi commented May 17, 2017

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 17, 2017

Member

@ogizanagi is right, but I think it's better to always call the method. If the constructor behavior changes a day, and given the method itself return the property if initialized, I think it's sane to use it rather than the prop.

Member

chalasr commented May 17, 2017

@ogizanagi is right, but I think it's better to always call the method. If the constructor behavior changes a day, and given the method itself return the property if initialized, I think it's sane to use it rather than the prop.

@jvasseur

This comment has been minimized.

Show comment
Hide comment
@jvasseur

jvasseur May 17, 2017

Contributor

@ogizanagi you could add a method in your kernel that modifies the return value of getProjectDir between the moment the object is created and the moment the container is compiled.

Contributor

jvasseur commented May 17, 2017

@ogizanagi you could add a method in your kernel that modifies the return value of getProjectDir between the moment the object is created and the moment the container is compiled.

@aschempp

This comment has been minimized.

Show comment
Hide comment
@aschempp

aschempp May 17, 2017

Contributor

you could add a method in your kernel that modifies the return value of getProjectDir between the moment the object is created and the moment the container is compiled.

That is exactly the point. Originally, we had a setRootDir method to tell the Kernel where it's root dir is located. Because our kernel is distributed with a bundle but the root dir is still /app. Now that does no longer work with projectDir, as we cannot override it after __construct (even though getKernelParameters has not been called yet).

Contributor

aschempp commented May 17, 2017

you could add a method in your kernel that modifies the return value of getProjectDir between the moment the object is created and the moment the container is compiled.

That is exactly the point. Originally, we had a setRootDir method to tell the Kernel where it's root dir is located. Because our kernel is distributed with a bundle but the root dir is still /app. Now that does no longer work with projectDir, as we cannot override it after __construct (even though getKernelParameters has not been called yet).

@aschempp

This comment has been minimized.

Show comment
Hide comment
@aschempp

aschempp May 17, 2017

Contributor

If this is to be merged, the method must not be called on the constructor, otherwise calling the method in getKernelParameters does not make much sense to me.

Contributor

aschempp commented May 17, 2017

If this is to be merged, the method must not be called on the constructor, otherwise calling the method in getKernelParameters does not make much sense to me.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 17, 2017

Contributor

Agree; not sure what's the lazy initialization is about. But it doesnt harm really :)

Contributor

ro0NL commented May 17, 2017

Agree; not sure what's the lazy initialization is about. But it doesnt harm really :)

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 17, 2017

Member

otherwise calling the method in getKernelParameters does not make much sense to me.

I don't think so. The getter is the extension point. The private property is set at construction so that it is always available internally; and the method is called when needed. As pointed by @jvasseur, the kernel state can change after construction, always calling the method is just safe-guard, that's fine to me.

Member

chalasr commented May 17, 2017

otherwise calling the method in getKernelParameters does not make much sense to me.

I don't think so. The getter is the extension point. The private property is set at construction so that it is always available internally; and the method is called when needed. As pointed by @jvasseur, the kernel state can change after construction, always calling the method is just safe-guard, that's fine to me.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi May 17, 2017

Member

you could add a method in your kernel that modifies the return value of getProjectDir between the moment the object is created and the moment the container is compiled.

the kernel state can change after construction

Changing this value after initialization looks weird to me honestly, so I don't really get the point, but ¯\_(ツ)_/¯.

Member

ogizanagi commented May 17, 2017

you could add a method in your kernel that modifies the return value of getProjectDir between the moment the object is created and the moment the container is compiled.

the kernel state can change after construction

Changing this value after initialization looks weird to me honestly, so I don't really get the point, but ¯\_(ツ)_/¯.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 17, 2017

Member

Not only the value itself, but some other properties on which the custom getProjectRootdir() could rely on... it's all about weird edge cases, but preventing them in the parent class doesn't harm, and since we can do it easily...

Member

chalasr commented May 17, 2017

Not only the value itself, but some other properties on which the custom getProjectRootdir() could rely on... it's all about weird edge cases, but preventing them in the parent class doesn't harm, and since we can do it easily...

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi May 17, 2017

Member

@chalasr: What about removing the the assignation on this line in this PR too, as discussed (as it's already done in the Kernel::getProjectDir() implementation) ?

Member

ogizanagi commented May 17, 2017

@chalasr: What about removing the the assignation on this line in this PR too, as discussed (as it's already done in the Kernel::getProjectDir() implementation) ?

Show outdated Hide outdated src/Symfony/Component/HttpKernel/Tests/KernelTest.php
$kernel->boot();
$r = new \ReflectionMethod($kernel, 'getKernelParameters');
$r->setAccessible(true);

This comment has been minimized.

@ro0NL

ro0NL May 17, 2017

Contributor

You're cheating ;-)

@ro0NL

ro0NL May 17, 2017

Contributor

You're cheating ;-)

This comment has been minimized.

@ro0NL

ro0NL May 17, 2017

Contributor

Besides due the constructor initialization this test also passes without your change :) so we actually need to test some weird edge case here. Perhaps return different values on consecutive calls?

@ro0NL

ro0NL May 17, 2017

Contributor

Besides due the constructor initialization this test also passes without your change :) so we actually need to test some weird edge case here. Perhaps return different values on consecutive calls?

@aschempp

This comment has been minimized.

Show comment
Hide comment
@aschempp

aschempp May 17, 2017

Contributor

The private property is set at construction so that it is always available internally; and the method is called when needed.

The property must not be used internally (except for the getter method) if the getter is to be the extension point! So there's no point in initializing it.

Contributor

aschempp commented May 17, 2017

The private property is set at construction so that it is always available internally; and the method is called when needed.

The property must not be used internally (except for the getter method) if the getter is to be the extension point! So there's no point in initializing it.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 17, 2017

Contributor

Tend to agree with that :) it also makes the test valid.

Contributor

ro0NL commented May 17, 2017

Tend to agree with that :) it also makes the test valid.

@@ -604,7 +604,7 @@ protected function getKernelParameters()
return array_merge(
array(
'kernel.root_dir' => realpath($this->rootDir) ?: $this->rootDir,
'kernel.project_dir' => realpath($this->projectDir) ?: $this->projectDir,
'kernel.project_dir' => realpath($this->getProjectDir()) ?: $this->getProjectDir(),

This comment has been minimized.

@ro0NL

ro0NL May 17, 2017

Contributor

Should we use the getter variant for other params too, while at it?

@ro0NL

ro0NL May 17, 2017

Contributor

Should we use the getter variant for other params too, while at it?

This comment has been minimized.

@chalasr

chalasr May 22, 2017

Member

Yea I think, I'll do it separately for lower branches once this merged

@chalasr

chalasr May 22, 2017

Member

Yea I think, I'll do it separately for lower branches once this merged

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 18, 2017

Member

constructor assignment removed, changed the test to better show what this prevents.

Member

chalasr commented May 18, 2017

constructor assignment removed, changed the test to better show what this prevents.

@chalasr chalasr changed the base branch from master to 3.3 May 18, 2017

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone May 18, 2017

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh
Member

xabbuh commented May 22, 2017

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion May 22, 2017

Member

👍

Member

Tobion commented May 22, 2017

👍

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot May 25, 2017

Member

Thank you @chalasr.

Member

fabpot commented May 25, 2017

Thank you @chalasr.

@fabpot fabpot merged commit 3230fc7 into symfony:3.3 May 25, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 25, 2017

bug #22728 [HttpKernel] Fix kernel.project_dir extensibility (chalasr)
This PR was merged into the 3.3 branch.

Discussion
----------

[HttpKernel] Fix kernel.project_dir extensibility

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

Alternative to #22727 that makes use of the existing public api.

Commits
-------

3230fc7 Fix kernel.project_dir extensibility

@chalasr chalasr deleted the chalasr:get-proj-dir branch May 26, 2017

@fabpot fabpot referenced this pull request May 29, 2017

Merged

Release v3.3.0 #22949

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment