Fix mvc bootrapping

[FalkHe]

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

Description

Provide option to enable MVC Application bootstrapping.

False by default, to not break any existing app.

See Issue #106 .

[froschdesign]

The current class is a container resolver and not an application initializer. Maybe another listener for the console application would be a better place for this task. 🤔

[steffendietz]

I'm not arguing here, but couldn't a similar argument currently be made regarding the whole module loading?

[froschdesign]

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.

[steffendietz]

Besides my previous comment, which ConsoleEvent could be used for initializing the MvcApplication?

[froschdesign]

You can find the available events with description here: https://symfony.com/doc/current/components/console/events.html

[FalkHe]

You're absolutely right. I did not do this to keep it BC.

• loadModules() is already it this place and is part of ::init(), too
• moving the old code (the half App initialization) to a different place, changes the execution order and might cause a BC break
• separating the "old-way" from "the-new" way to two separate places would require checking the option in two places ... that's a dangerous thing to do, imo. And changes execution order, too, in case you wan't to use the ::init().

This being said: In a release that is allowed to break BC I agree that the whole MVC Init should be outside of the Resolver. ( Including loadModules(), imo ). But with this there would be a lot more to do.

For this quick fix, the best is to keep the footprint small and change as little as possible.

[froschdesign]

For this quick fix…

We want a correct solution, anything else means pushing the problem into the future and touching it again.

[froschdesign]

And please note, I wrote "maybe". That means it is not the solution or even the correct solution. That was just the first thought that came to my mind when I saw the names. 😄

[FalkHe]

Totally legit. Any thoughts are welcome. :-)

That's what I can offer for now. I'd like to write a more clean solution but this will not happen within the next few days. Maybe someone else can hop on earlier.

[steffendietz]

I like it.

Regarding the

When enabling it, make sure not to do any HTTP-Only related stuff on Module's onBootstrap method.

Is there currently a way to tell if we are in a laminas-cli run context?
The old laminas-mvc-console package had a delegator factory which was used to create a ConsoleRequest instead of the HttpRequest.

Would it maybe make sense to define a namespaced const in bin/laminas?
Something like

define('Laminas\Cli\RUN_CONTEXT', true);

So that a hypothetical onBootstrap method could bail out?

public function onBootstrap(EventInterface $e)
{
    // do general purpose bootstrapping
    
    if (defined('Laminas\Cli\RUN_CONTEXT')) {
        return;
    }
    
    // do http-only bootstrapping
}

I'm just spit-balling here. 😅

[froschdesign]

@steffendietz

Would it maybe make sense to define a namespaced const in bin/laminas?

Pollution of the global namespace is not an option and must be avoided.

[steffendietz]

Pollution of the global namespace is not an option and must be avoided.

That's why I was specifically talking about a namespaced constant.

[Ocramius]

Regardless of the patch direction, I expect tests to verify what's being done here.

[FalkHe]

Is there currently a way to tell if we are in a laminas-cli run context?

Not AFAIK.

But: This should not be necessary. As told in the documentation you should not do any heavy stuff in onBoostrap(). Only prepare Services, Listeners, ... things that aren't shouldn't be expensive at all. i.E.: setting up a Listener on MvcEvent::EVENT_DISPATCH ist totally fine. It's not executed, because lamas-cli doesn't fire MvcEvent::EVENT_DISPATCH.

If you've got something that really should not be executed in cli context but is currently located in onBoostrap(), it's very likely not in the right place. Think about putting the code behind an Event that's only fired in http-context (that's what i do most of the time). Use either EVENT_DISPATCH / EVENT_DISPATCH_ERROR, EVENT_RENDER / EVENT_RENDER_ERROR, ...

... but that's OT and should be diskussed in the Forum.

[FalkHe]

Regardless of the patch direction, I expect tests to verify what's being done here.

May i 'expect' your help on this, @Ocramius?

I don't know how to set up an environment for the test. And I've no time to figure it out, atm. I tried some bits, without success. I'm not familiar with the used vfs and don't want to mess the tests up by introducing something totally different.

[froschdesign]

@FalkHe
I'll convert your PR into a draft, because let's find the correct solution first. Then we will also know what the tests have to look like.

[steffendietz]

@FalkHe

I created a failing test case for the original issue. https://github.com/steffendietz/laminas-cli/tree/failing-testcase-for-106

@froschdesign

What are your requirements for a correct solution?
Your wording seems to indicate, that the one discussed here is somehow not correct. Could you share your heuristic for this classification?

[Xerkus]

I would rather prefer Mvc application to provide config/container.php which would init and potentially bootstrap the Mvc before it would return container for laminas-cli to consume.

Bootstrapping Mvc unconditionally is not something I am comfortable with. That is a decision that needs to be made consciously for every specific application.

[FalkHe]

@Xerkus
Any discussion on different solutions / ideas should go into #106.

[Xerkus]

Indeed. My bad.