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

Add config/container.php #67

Merged
merged 1 commit into from
Mar 9, 2023
Merged

Conversation

Xerkus
Copy link
Member

@Xerkus Xerkus commented Mar 9, 2023

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA no

Description

Introduce config/container.php to match mezzio skeleton and make usage of tools like laminas-cli simpler.

See laminas/laminas-cli#106

Signed-off-by: Aleksei Khudiakov <[email protected]>
@froschdesign
Copy link
Member

…to match mezzio skeleton…

Good direction!

@Xerkus Xerkus added this to the 2.0.0 milestone Mar 9, 2023
@Xerkus Xerkus merged commit 4e1f7ce into laminas:2.0.x Mar 9, 2023
@Xerkus Xerkus deleted the feature/config-container branch March 9, 2023 14:24
@FalkHe
Copy link

FalkHe commented Mar 10, 2023

I do like the general approach of having a container.php against "config file only".

But I don't agree that initializing/bootstrapping the application during container initialization is a good idea. That's two different stages in the application lifecycle that should be handled separately. #IMO

  1. prepare dependencies / container => container.php
  2. Initialize the Application => app.php
  3. Execute Something (http, cli) => index.php / laminas-cli / sf-console / whatever

So the first lines of current Application::init() (until $serviceManager->setService('ApplicationConfig', $configuration);) should go into container.php.

And there should be an app.php that roughly looks like:

<?php 

$container = require_once 'container.php';
return Application::init($container); // load modules, attach listeners, bootstrap

With regards to laminas/laminas-cli#106:
laminas-cli could then initialize laminas-mvc just by requiring app.php and then having a clean bootstrapped Application instance.

#my-two-cents

@froschdesign
Copy link
Member

@FalkHe

And there should be an app.php that roughly looks like:

<?php 

$container = require_once 'container.php';
return Application::init($container); // load modules, attach listeners, bootstrap

Loading the modules at this point means that the container configuration is incomplete:

Without loading the modules, you will not get the container configuration from the individual modules, which means that the entire container will not be resolved correctly.

laminas/laminas-cli#110 (comment)

@FalkHe
Copy link

FalkHe commented Mar 13, 2023

This depends on how you define when the Container / the App is meant to be ready. IMO this is after app.php. At this point, loadModules() is executed and the container setup of the modules is done.

Of cause, one could put everything incl. Module Loading into container.php. But then there's no separation between static and dynamic configuration.

In my opinion "Container preparation" is all static. Only definitions of things that might be needed.

App initialization is more dynamic and the first touchpoint that might already use previously defined dependencies. i.E. fetch Module-Manager look for modules and initialize them, fetch event-manager and attach Listeners ...

Indeed difficult is the fact, that Lamainas MVC mixes both parts while loading modules. 🤷

To repeat: If you define App-Initialization (incl. dynamic module loading) as part of Container-Definition, then having everything in container.php is fine.

I'd rather like to separate both stages. But that's just my personal preference.

@froschdesign
Copy link
Member

To repeat: If you define App-Initialization (incl. dynamic module loading) as part of Container-Definition, then having everything in container.php is fine.

I am not defining anything here, I am describing the current behaviour we have to deal with. 😃

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

Successfully merging this pull request may close these issues.

3 participants