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

Automatically load the services.yml file if it exists #810

Merged
merged 1 commit into from
Oct 28, 2019

Conversation

leofeyer
Copy link
Member

@leofeyer leofeyer commented Oct 1, 2019

In Symfony 4, the services.yml file will be loaded automatically, therefore we should do it, too.

@leofeyer leofeyer added this to the 4.9 milestone Oct 1, 2019
@leofeyer leofeyer self-assigned this Oct 1, 2019
@leofeyer leofeyer requested a review from aschempp October 1, 2019 21:42
@fritzmg
Copy link
Contributor

fritzmg commented Oct 2, 2019

Same goes for the routing.yml

@leofeyer
Copy link
Member Author

leofeyer commented Oct 2, 2019

The routing file is already loaded automatically. 😉

// Load the routing.yml file if it exists
if ($configFile = $this->getConfigFile('routing.yml')) {
$routes = $this->loader->getResolver()->resolve($configFile)->load($configFile);
if ($routes instanceof RouteCollection) {
$collection->addCollection($routes);
}
}

@fritzmg
Copy link
Contributor

fritzmg commented Oct 2, 2019

Right, I forgot 🙃

@aschempp
Copy link
Member

I'm not sure I like this idea. We should discuss the whole file loading if we want to add this. In Symfony, all files are automatically loaded, not just the services.yml. If we want to add "auto load for everything", we should consider supporting env folders as well etc.

@leofeyer
Copy link
Member Author

leofeyer commented Oct 14, 2019

Are you sure that Symfony autoloads everything?

Because services.yaml and routing.yaml need to be loaded in different contexts.

-- See https://symfony.com/doc/current/setup/flex.html

@aschempp
Copy link
Member

I'm pretty sure it loads everything in packages but I haven't checked how it works exactly.

@fritzmg
Copy link
Contributor

fritzmg commented Oct 14, 2019

Yes, it loads everything in /config/packages but not everything in /config. imho the Managed Edition should support the same.

@leofeyer
Copy link
Member Author

Yes, it loads everything in /config/packages but not everything in /config. imho the Managed Edition should support the same.

Full ACK. 👍

@aschempp
Copy link
Member

There is no config/packages in Contao Managed Edition, and I doubt that really makes sense…?

@aschempp
Copy link
Member

config/packages contains the (default) configuration for packages, they are contained in the manager plugin for Contao.

@fritzmg
Copy link
Contributor

fritzmg commented Oct 14, 2019

Yes but you could put your own bundle configurations there. Supporting the config/packages folder is just a nice to have, since you can do it also via the config.yml.

But loading the /config/services.yml is a must imho.

@leofeyer
Copy link
Member Author

My "Full ACK" meant "do not load everything in the root /config folder", because Symfony does not, either.

@aschempp
Copy link
Member

Maybe we should consider having a toggle somewhere.

  • "old" style to load app/Resources and support the config.yml (only)
  • "new" style to load root folders, have public and support config/packages.yml including environment dirs?

@leofeyer leofeyer requested review from ausi and Toflar October 25, 2019 16:16
@ausi
Copy link
Member

ausi commented Oct 25, 2019

What happens in installations that already have a services.yml which gets loaded via the config.yml. Would they overwrite each other with the same values and therefore don’t cause an issue?

@leofeyer
Copy link
Member Author

Yes, they would. Just like the parameter.yml file, which we also load twice.

Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please mark this up for discussion? I don't like the idea of just loading additional files "because Symfony does that". If we load additional files automatically to somewhat follow Symfony Flex, then we should rething the whole thing (like loading everything in prod and dev folders` etc.)

@leofeyer
Copy link
Member Author

You are talking about two different things here. This PR simply adds autoloading for the services.yml file – nothing more. If you want to add autoloading for everything (including sub-folders), you should create a separate PR that we can discuss.

@leofeyer leofeyer merged commit 2d5832f into master Oct 28, 2019
@leofeyer leofeyer deleted the feature/services-yml branch October 28, 2019 12:16
@aschempp
Copy link
Member

It's not two different things. It's "do we keep our approach" or "do we support things like flex does". Currently you're mixing the two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants