8000 New getProjectDir in MicroKernelTrait is too opinionated · Issue #36719 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

New getProjectDir in MicroKernelTrait is too opinionated #36719

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
Closed
garak opened this issue May 6, 2020 · 4 comments · Fixed by #36721
Closed

New getProjectDir in MicroKernelTrait is too opinionated #36719

garak opened this issue May 6, 2020 · 4 comments · Fixed by #36721
Labels
Bug HttpKernel Status: Needs Review

Comments

@garak
Copy link
Contributor
garak commented May 6, 2020

Symfony version(s) affected: 5.1.0

With changes introduced in commit a689807, is always supposed that Kernel is simply in src directory.
This breaks applications where Kernel is put elsewhere (that, by the way, would work fine with the base definition of getProjectDir in the base kernel class).

I suggest to revert such change and to remove getProjectDir from MicroKernelTrait
@garak garak added the Bug label May 6, 2020
@nicolas-grekas
Copy link
Member
nicolas-grekas commented May 6, 2020 •
edited
Loading

That's just the defaults, you are free to provide your own implementation that fits your layout.

@xabbuh
Copy link
Member
xabbuh commented May 6, 2020

I think the point is that if you were already using the trait in 5.0 the value returned by getProjectDir() now changes when updating to 5.1 and thus one needs to actively change application code when updating.

@garak
Copy link
Contributor Author
garak commented May 6, 2020

That's just the defaults, you are free to provide your own implementation that fits your layout.

But we already have a default (in base Kernel) and it's better. Why to redefine it in trait?

@nicolas-grekas
Copy link
Member

I just ran some benchmarks, I measure no diff when I drop the implem in MicroKernel. I suppose replacing file_exists() by is_file() reclaimed the loss.

Please send a PR @garak

@garak garak mentioned this issue May 6, 2020
Merged
@fabpot fabpot closed this as completed May 6, 2020
fabpot added a commit that referenced this issue May 6, 2020
@fabpot
bug #36721 [FrameworkBundle] remove getProjectDir method from MicroKe…
75e71e3 
…rnelTrait (garak)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[FrameworkBundle] remove getProjectDir method from MicroKernelTrait

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #36719
| License       | MIT
| Doc PR        | not needed

Remove method added in trait, to be able to use same method in base kernel class.

Commits
-------

f2f3eba remove getProjectDir method from MicroKernelTrait
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpKernel Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[FrameworkBundle] remove getProjectDir method from MicroKernelTrait garak/symfony
5 participants
@fabpot @garak @nicolas-grekas @xabbuh @carsonbot
0