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.

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

Config directory should be in root instead of app folder #566

Merged
merged 8 commits into from Jul 11, 2019

Conversation

Projects
None yet
4 participants
@aschempp
Copy link
Contributor

commented Jul 8, 2019

This migrates from /app/config to /config in Contao 4.8.

@aschempp aschempp requested review from Toflar, sheeep, bytehead and leofeyer Jul 8, 2019

@leofeyer leofeyer added the feature label Jul 9, 2019

@leofeyer leofeyer added this to the 4.8 milestone Jul 9, 2019

@leofeyer
Copy link
Member

left a comment

There are some inconsistencies that we need to discuss.

  1. You are loading the new config file before the legacy one, but you are loading the legacy routing file before the new one. Shouldn't we always load the legacy resources first?

  2. If there is a /config/parameters.yml file, shouldn't we ignore the app/config/parameters.yml file?

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

You are loading the new config file before the legacy one, but you are loading the legacy routing file before the new one. Shouldn't we always load the legacy resources first?

No, this depends on the loading order:

  • For parameters, the first one will win. So we have to load the most important/correct one first (imho /config/parameters.yml)
  • For configuration, the last one will win, because it's an array_merge of the container configuration
  • For routing, the last one will win, because the previous key will simply override the old one.

That's why:

  1. The parameters.yml is loaded first in the ContaoKernel, otherwise the defaults loaded by plugins would win.
  2. Why we fetch routing information from the plugin loader in reverse order (last argument true).
@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

For parameters, the first one will win.

Ok, then we should check the following files in the following order:

  • config/parameters.yml <- new
  • app/config/parameters.yml <- legacy

We do not need to check any .dist files, because they belong to Symfony 3 and Contao 4.8 requires at least Symfony 4.2.

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Ok, then we should check the following files in the following order:

  • config/parameters.yml <- new
  • app/config/parameters.yml <- legacy

We already do. https://github.com/contao/contao/pull/566/files#diff-d5bac182e2834819fa55077f6a3a3841R178
The second loading of parameters.yml is actually only if you create services there, because then that should overwrite/win again 😆

We do not need to check any .dist files, because they belong to Symfony 3 and Contao 4.8 requires at least Symfony 4.2.

Loading dist files is only relevant to the install tool, where empty parameters might be required to correctly initialize the system.

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

We need to agree on whether both folder should be supported at the same time, or we only use /config and skip app/config if the root one exist. Depending on that decision I can update the PR.

/cc @leofeyer @Toflar @ausi @bytehead @sheeep


@leofeyer leofeyer modified the milestones: 4.8, 4.9 Jul 10, 2019

@leofeyer leofeyer force-pushed the master branch 2 times, most recently from b2174a9 to 8728020 Jul 10, 2019

@leofeyer leofeyer modified the milestones: 4.9, 4.8 Jul 11, 2019

$configDir = $this->getProjectDir().'/app/config';
$configDir = $this->getProjectDir().'/config';
if (!file_exists($configDir)) {

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jul 11, 2019

Member

What if the config directory exists but the parameters.yml file is still in app/config? Shouldn't we apply the same file_exists($rootDir.'/config/parameters.yml') || !file_exists($rootDir.'/app/config/parameters.yml') logic here?

This comment has been minimized.

Copy link
@aschempp

aschempp Jul 11, 2019

Author Contributor

yes I think we should

@leofeyer leofeyer force-pushed the bugfix/config-folder branch 3 times, most recently from b1ac92a to f0f2a98 Jul 11, 2019

@leofeyer leofeyer force-pushed the bugfix/config-folder branch from 1e09316 to 80b2458 Jul 11, 2019

@leofeyer leofeyer merged commit 31f11d3 into master Jul 11, 2019

0 of 3 checks passed

Travis CI - Pull Request Build Errored
Details
Travis CI - Branch Build Canceled
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Thank you @aschempp

@aschempp aschempp deleted the bugfix/config-folder branch Jul 11, 2019

@fritzmg

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

Quick question: do DCA and translation changes go into /config/contao/… now?

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

Nope, they go into the /contao folder (which you have to add manually).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.