Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Add ability to override name of the config directory#42218
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedJul 21, 2021
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
carsonbot commentedJul 21, 2021
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
sstok commentedJul 21, 2021
Please use a descriptive title. FYI. This will require an update to Flex-plugin as well as recipes expect this convention. |
stof commentedJul 21, 2021
If we don't change the default, I don't think it will impact Flex (other parts of Flex already assume you use the standard symfony structure rather than a customized one) |
ivanstan commentedJul 21, 2021
I think preferred approach would be to first implement a function that returns a config directory path without changing the default, with a possibility for a user to override it on his own responsibility. This will give some time to the flex and recipe developers to start relying on this function before decision is made if actually the directory name is gonna change. |
Tobion commentedJul 21, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@sstok probably means it somewhat requires an update of the recipes, e.g.https://github.com/symfony/recipes/blob/master/symfony/framework-bundle/5.4/src/Kernel.php to make use of the config dir method. I'm also 👎 as there are is not enough demand for such a change. People who really want to diverge from documentation and flex support etc, can just overwrite the relevant kernel methods themselves. |
Nyholm commentedJul 22, 2021
Hm.. I agree with@Tobion that it is not a great demad for this. However, this patch is super small and Im not sure we have any other hard coded path like this. This will (or SHOULD) obviously only be used by super advanced that have really good reasons by doing so. Just "I want to follow Unix standards" is not really a strong argument. So, Yes, (almost) nobody should use this. But Im fine with this small patch anyways. @ivanstan, may I ask you if you have a good reason for using |
ivanstan commentedJul 22, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Nyholm To be honest its just my OCD. Other than the fact that all other directories are configurable. |
Nyholm commentedJul 22, 2021
Hehe. I’ve been there. But I guess that is something you need to fight though or override registerContainerConfiguration() and registerBundles() yourself. I’m leaning towards closing this. I’ll wait an opinion from some other maintainer. |
ivanstan commentedJul 22, 2021
@Nyholm Yes, I have those methods overridden in most of my projects :) |
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would be +0 if the method were private.
ro0NL commentedJul 22, 2021
yceruto left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm also not sure if it will be worth it, from what I've read in other comments, and becauseconfig/ dir matches package standards (https://github.com/php-pds/skeleton).
If we do it for apps, why don't we do the same for bundles?
I'm -1 so far.
| $kernelDefinition->addTag('routing.route_loader'); | ||
| $container->addObjectResource($this); | ||
| $container->fileExists($this->getProjectDir().'/config/bundles.php'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are other occurrences inFrameworkBundle/DependencyInjection/FrameworkExtension.php (<project_dir>/config/validator and<project_dir>/config/serializer ).
Also, we are hardcoding it here:
| ->scalarNode('vault_directory')->defaultValue('%kernel.project_dir%/config/secrets/%kernel.runtime_environment%')->cannotBeEmpty()->end() |
fabpot commentedOct 29, 2021
Done as part of#42991. Thank you. |
Uh oh!
There was an error while loading.Please reload this page.
For way too long Symfony is following Linux directory structure for all of its directories except for config, if we don't plan to change
/configto/etcat least we can avoid hard-coding it and allow overriding of the methodgetConfigDirin Kernel.php