8000 [FrameworkBundle] Avoid calling getProjectDir() on KernelInterface by ro0NL · Pull Request #28897 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] Avoid calling getProjectDir() on KernelInterface #28897

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
Merged
merged 1 commit into from
Oct 17, 2018
Merged

[FrameworkBundle] Avoid calling getProjectDir() on KernelInterface #28897

merged 1 commit into from
Oct 17, 2018

Conversation

ro0NL
Copy link
Contributor
@ro0NL ro0NL commented Oct 16, 2018
Q A
Branch? master (might be applied on 3.4 as well)
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

Removes the last call to getProjectDir() in core (tests/kernel itself excluded). Yay :)

@nicolas-grekas
Copy link
Member

Thank you @ro0NL.

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 17, 2018
@nicolas-grekas nicolas-grekas merged commit 894c155 into symfony:master Oct 17, 2018
nicolas-grekas added a commit that referenced this pull request Oct 17, 2018
@nicolas-grekas
minor #28897 [FrameworkBundle] Avoid calling getProjectDir() on Kerne…
7048679 
…lInterface (ro0NL)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[FrameworkBundle] Avoid calling getProjectDir() on KernelInterface

| Q             | A
| ------------- | ---
| Branch?       | master (might be applied on 3.4 as well)
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Removes the last call to getProjectDir() in core (tests/kernel itself excluded). Yay :)

Commits
-------

894c155 [FrameworkBundle] Avoid calling getProjectDir() on KernelInterface
@ro0NL ro0NL deleted the about branch October 17, 2018 07:14
@fabpot
Copy link
Member
fabpot commented Oct 17, 2018

I' 👎 for this change. We are replacing a perfectly valid method call (IDE-supported, ...) by a magic call to a parameter from the container. I don't see how this is better.

@ro0NL
Copy link
Contributor Author
ro0NL commented Oct 17, 2018

It's not IDE supported, that would require some instanceof Kernel check (here we obtain the KernelInterface).

To me getContainer()->getParameter('kernel.project_dir') is the interface-bound variant of getProjectDir(), assuming in SF context it'll always be set.

I considered injecting %kernel.project_dir% instead, but IMHO makes less sense as we need the value tied to the current project, not some arbitrary path.

We do the same here:

symfony/src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php

Line 96 in 7048679

$targetArg = $kernel->getContainer()->getParameter('kernel.project_dir').'/'.$targetArg;

I believe when revamping console commands as a service, we discussed to actually inject the parameter here (where it does makes sense), but at that time we had some issues with root_dir still. Could be updated today i guess.

@ro0NL
Copy link
Contributor Author
ro0NL commented Oct 17, 2018

image

@nicolas-grekas
Copy link
Member

Adding an @method annotation on the interface would fix that. Does phpstorm support it @ro0NL?

@ro0NL
Copy link
Contributor Author
ro0NL commented Oct 17, 2018
8000

Yep, totally. Using @method string getProjectDir().

I discovered https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#54-inline-phpdoc but that's not working, so the actual doc should stay in Kernel.

Should we do it as of 3.4? How does it affect BC in general?

@nicolas-grekas
Copy link
Member

Great, so let's revert here, use getProjectDir in AssetsInstallCommand also, and add the annotation on the interface instead.
This should be done on 4.2, since adding the annotation is the same as adding a deprecation (we would then need a separate PR to make DebugClassLoader throw a notice in this situation).
Up for submitting this @ro0NL ?

This was referenced Oct 17, 2018
Merged
Merged
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Oct 17, 2018
@fabpot
minor #28901 [HttpKernel] Introduce KernelInterface::getProjectDir() …
8e34fc5 
…(ro0NL)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[HttpKernel] Introduce KernelInterface::getProjectDir()

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | not yet
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | symfony/symfony#28897 (comment)
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Removes the last call to getParameter('kernel.project_dir') during runtime in core. Yay :)

cc @fabpot @nicolas-grekas

Commits
-------

b046f414e7 [HttpKernel] Introduce KernelInterface::getProjectDir()
fabpot added a commit that referenced this pull request Oct 17, 2018
@fabpot
minor #28901 [HttpKernel] Introduce KernelInterface::getProjectDir() …
dd432c4 
…(ro0NL)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[HttpKernel] Introduce KernelInterface::getProjectDir()

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | not yet
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #28897 (comment)
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Removes the last call to getParameter('kernel.project_dir') during runtime in core. Yay :)

cc @fabpot @nicolas-grekas

Commits
-------

b046f41 [HttpKernel] Introduce KernelInterface::getProjectDir()
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
symfony-splitter pushed a commit to symfony/debug that referenced this pull request Jan 5, 2019
@fabpot
feature #28902 [Debug] Detect virtual methods using @method (ro0NL)
b04503a 
This PR was squashed before being merged into the 4.3-dev branch (closes #28902).

Discussion
----------

[Debug] Detect virtual methods using @method

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | symfony/symfony#28897 (comment)
| License       | MIT
| Doc PR        | symfony/symfony-docs#10504

My first Debug PR, so im still on it. But early feedback welcome.

In #28901 we'll introduce a new virtual interface method using `@method` annotation. IIUC the idea is to trigger whenever such a method is overridden.

Commits
-------

38877c32ac [Debug] Detect virtual methods using @method
fabpot added a commit that referenced this pull request Jan 5, 2019
@fabpot
feature #28902 [Debug] Detect virtual methods using @method (ro0NL)
5c2cee5 
This PR was squashed before being merged into the 4.3-dev branch (closes #28902).

Discussion
----------

[Debug] Detect virtual methods using @method

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #28897 (comment)
| License       | MIT
| Doc PR        | symfony/symfony-docs#10504

My first Debug PR, so im still on it. But early feedback welcome.

In #28901 we'll introduce a new virtual interface method using `@method` annotation. IIUC the idea is to trigger whenever such a method is overridden.

Commits
-------

38877c3 [Debug] Detect virtual methods using @method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature FrameworkBundle Status: Needs Review
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
4 participants
@ro0NL @nicolas-grekas @fabpot @carsonbot
0