laminas-cli icon indicating copy to clipboard operation
laminas-cli copied to clipboard

Modules of a MvcApplication are not properly bootstrapped

Open steffendietz opened this issue 2 years ago • 38 comments

Bug Report

Q A
Version(s) 1.7.0

Summary

When using laminas/laminas-cli in the context of a MvcApplication (ContainerResolver::resolveMvcContainer), modules don't get bootstrapped completely, which results in services potentially missing configuration.

Current behavior

We use the onBootstrap event of our MvcApplication to set up listeners for services or set up service state based on context. When using those services inside of commands executed by laminas/laminas-cli, those services are not fully set up, which results in unexpected behavior, since the application/module is missing the bootstrap lifecycle stage.

Based on the Documentation, the onBootstrap method should/could be used for:

  • module-specific configuration
  • setup event listeners for you module

How to reproduce

Use a service somehow modified or configured in the onBootstrap method in a Command executed through laminas/laminas-cli. The service passed to the command has not gone through the MvcApplications bootstrap lifecycle.

I try to illustrate this on the following pseudo-module:

// Module
class Module implements BootstrapListenerInterface
{
    public function onBootstrap(EventInterface $e)
    {
        $sm = $e->getApplication()->getServiceManager();
        
        $sm->get(SampleService::class)->setBootstrapped(true);
    }
}
// in SampleCommandFactory
public function __invoke(ContainerInterface $container, $requestedName, ?array $options = null)
{
    return new SampleCommand(
        $container->get(SampleService::class)
    );
}
// in SampleCommand
protected function execute(InputInterface $input, OutputInterface $output) : int
{
    return $this->sampleService->isBootstrapped() ? 0 : 1;
}

SampleCommand returns 1.

Expected behavior

From my perspective, using commands through laminas/laminas-cli in the context of an MvcApplication, should guarantee the same initialisation level as with Application::init.

In the above example, SampleCommand should return 0, as it has access to a bootstrapped version of the SampleService.

steffendietz avatar Dec 07 '22 09:12 steffendietz

When using laminas/laminas-cli in the context of a MvcApplication (ContainerResolver::resolveMvcContainer), modules don't get bootstrapped completely, which results in services potentially missing configuration.

laminas-cli do not start an application, not a laminas-mvc, Mezzio or something else. laminas-cli uses the same configuration of the service container of your laminas-mvc application but it will not launch the application or trigger any events. Therefore, the method onBootstrap is not called in modules.

froschdesign avatar Dec 07 '22 09:12 froschdesign

Hey Steffen,

yes, you are right. I am not 100% aware of why we decided to avoid calling bootstrap in CLI. AFAIR, that was outlined in the RFC, maybe @michalbundyra can remember.

As a workaround, you can create a config/container.php file (which we do have in our MVC application as well) where you then bootstrap your application as you need it:

<?php

use Laminas\Mvc\Service\ServiceManagerConfig;
use Laminas\ServiceManager\ServiceManager;
use Psr\Container\ContainerInterface;

chdir(\dirname(__DIR__));

// Setup autoloading
require_once __DIR__ . '/../vendor/autoload.php';

return (static function (): ContainerInterface {
    // Our `application.config.php` does already handle development config merge if exists
    $appConfig = require __DIR__ . '/application.config.php';

    $smConfig = new ServiceManagerConfig($appConfig['service_manager'] ?? []);
    $serviceManager = new ServiceManager();
    $smConfig->configureServiceManager($serviceManager);
    $serviceManager->setService('ApplicationConfig', $appConfig);
    $serviceManager->get('ModuleManager')->loadModules();

    // Fetch `Application` from service manager and bootstrap with configured listeners as done in the `Application#init` method

    // Restore error and exception handlers to avoid something is suppressing errors as applications are usually called via HTTP
    restore_error_handler();
    restore_exception_handler();

    return $serviceManager;
})();

@froschdesign Yes, but I understand that someone expects that when creating a CLI command, it can interact with modules and if modules are only properly setup when calling the bootstrap event, thats actually a problem.

We have a pretty large MVC application but I never liked the bootstrap event. We do not configure anything in there but used service delegators so that services which do need runtime configuration can be configured in their factories rather than in some event listeners. The major benefit when using delegators is, that you only bootstrap stuff which is actually used at runtime. But I have no use-case where I would have a need for the "bootstrap" event and thus, I probably can't relate properly.

boesing avatar Dec 07 '22 09:12 boesing

@froschdesign Yes, I understand that, but as @boesing mentioned that is kind of the whole point of the bug report.

Currently it means, that from a users point of view, a service acquired through the Container in a laminas-cli context has a different level of setup than when acquired in the web context.

I find this is kind of difficult to explain. The ContainerResolver is even aware that we are inside of a MvcApplication context and calls the loaded config $appConfig.

Replacing the current ContainerResolver::resolveMvcContainer with something like

    private function resolveMvcContainer(string $path): ContainerInterface
    {
        /**
         * @psalm-suppress UnresolvableInclude
         * @psalm-var array<array-key, mixed> $appConfig
         */
        $appConfig = include $path;
        Assert::isMap($appConfig);

        $developmentConfigPath = sprintf('%s/config/development.config.php', $this->projectRoot);
        if (file_exists($developmentConfigPath)) {
            $devConfig = include $developmentConfigPath;
            Assert::isMap($devConfig);

            $appConfig = ArrayUtils::merge($appConfig, $devConfig);
            Assert::isMap($appConfig);
        }

        $application = MvcApplication::init($appConfig);

        return $application->getServiceManager();
    }

would in my opinion completely solve the issue, since it lets application bootstrapping as well as module loading and container initialization remain the concern of the MvcApplication.

Edit: What would even be the intended way of executing the missed setup steps? Calling something like:

$container->get('Application')->bootstrap();

in the CommandFactory?

steffendietz avatar Dec 07 '22 10:12 steffendietz

@boesing regarding the use of the onBootstrap. I know there are probably more elegant ways to do things nowadays. But as of today the bootstrap is part of the documented application life-cycle and as such should be supported.

Also, laminas-cli is published as a replacement for laminas-console and laminas-mvc-console and as such should probably support the pecularities of its ecosystem.

Bottomline is: If this is not considered a bug, then I would like to have a pointer to the "idiomatic" laminas-mvc way of integrating laminas-cli.

steffendietz avatar Dec 07 '22 10:12 steffendietz

@boesing @steffendietz Please don't misunderstand me, I didn't say that it has to be this way or that I like it that way. I have only written what it is like. There have already been several questions or the expectation of this. (see also in the forum)

froschdesign avatar Dec 07 '22 10:12 froschdesign

My expectation is quite clear that the individual modules will be loaded and booted.

The documentation doesn't say - don't use this onBootstrap method.

Some settings for the ServiceManager can be regulated in this way, and it is precisely these settings that are then missing in the ServiceManager within a CLI.

This just feels wrong and not thought through to the end. If the framework offers a modification path, it must also be observed in any case.

nusphere avatar Dec 07 '22 10:12 nusphere

@nusphere

This just feels wrong and not thought through to the end. If the framework offers a modification path, it must also be observed in any case.

Every pull request is welcome. Go for it! 👍🏻

froschdesign avatar Dec 07 '22 10:12 froschdesign

is something open or done?

nusphere avatar Jan 06 '23 12:01 nusphere

@nusphere not AFAIK

Ocramius avatar Jan 06 '23 15:01 Ocramius

To give a use case:

I usually set up EventListeners within onBoostrap() or via the 'listeners' config key. Both are omitted when running a command via laminas-cli. That's neither obvious nor documented and there's no clean way to attach the same EventListeners for both scenarios (mvc+cli). (correct me if i'm wrong)

FalkHe avatar Mar 08 '23 15:03 FalkHe

yes, you are right. I am not 100% aware of why we decided to avoid calling bootstrap in CLI. AFAIR, that was outlined in the RFC, maybe @michalbundyra can remember.

Because request and response are in container, a lot MVC applications in the wild do http request specific initialization in onBootstrap leading to some horrible breakages.

Xerkus avatar Mar 08 '23 16:03 Xerkus

I gave it a shot. Let me know your thoughts.

FalkHe avatar Mar 09 '23 09:03 FalkHe

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.

Change like this applied to application would solve this without assumptions from this component.

https://github.com/Xerkus/laminas-mvc-skeleton/commit/1a242e41135d49f159a5d45cb5eec8c9639c1682

Xerkus avatar Mar 09 '23 13:03 Xerkus

@boesing

What RFC are you refferring to? Could you please explain the reason/source for this decision?

But I have no use-case where I would have a need for the "bootstrap" event

How/When/Where do you attach your EventListeners? This needs to be done during boot. And here is no other place except onBootstrap to do this, AFAIK.

FalkHe avatar Mar 09 '23 13:03 FalkHe

Changes now were made to MVC skeleton application to include config/container.php which will fire MVC onBootstrap() events. https://github.com/laminas/laminas-mvc-skeleton/pull/67

I suggest what happens here is we update documentation detailing how to apply same changes to existing MVC applications to utilize laminas-cli. Drop MVC specific container resolver in the next major.

Xerkus avatar Mar 09 '23 14:03 Xerkus

@boesing

What RFC are you refferring to? Could you please explain the reason/source for this decision?

Maybe I had this in mind and just did not remembered correct:

https://github.com/laminas/laminas-cli/issues/92#issuecomment-1083172593

@xerkus we should deprecate listener stuff in init and remove it in v4. listeners should get registered with delegator rather than manual config extraction.

boesing avatar Mar 09 '23 17:03 boesing

@steffendietz @nusphere Can you check the option via configuration file for the container (container.php) in your applications?

Please see the related changes in the skeleton application:

  • https://github.com/laminas/laminas-mvc-skeleton/blob/8179275f7e9bf0178ae93ea87c065dff05223190/config/container.php
  • https://github.com/laminas/laminas-mvc-skeleton/blob/8179275f7e9bf0178ae93ea87c065dff05223190/public/index.php#L34-L37

Can you give feedback on this? Thanks in advance! 👍🏻

froschdesign avatar Mar 14 '23 13:03 froschdesign

@froschdesign As @Xerkus changes to the skeleton application basically replicated the workaround @boesing proposed 3 months ago, I can confirm that this indeed mitigates the problem. :+1:

Concerning this component (laminas/laminas-cli) however, I fear the initial issue is still not resolved, since the ContainerResolver::resolveMvcContainer code path still omits the bootstrapping life-cycle stage of modules. Also, there is no clear answer from one of the core maintainers, how this missing life-cycle step should be triggered, although this question came up two times in this thread.

Additionally, inspired by the comment of @boesing, I looked at some open issues. At least #92 and #85 in part struggled with the same underlying problem.

At the very least, a deprecation message or better yet, a warning would need to be displayed, if the container was resolved using resolveMvcContainer, so that the user is made aware that the modules are not bootstrapped properly.

steffendietz avatar Mar 15 '23 11:03 steffendietz

TBH, we made bad decisions in the past - or at least it feels to me as that is the case. Having application magic (as we do have in mezzio aswell with all these Application#pipe, Application#get, Application#post methods) to configure the application is problematic and will most probably not work with laminas-cli. Pretty sure these methods were introduced to have syntax sugar when starting with mezzio but when introducing a CLI command, that won't have access to that index.php initialized stuff.

So you will never have those routes or middlewares available in laminas-cli when passed these are only configured in index.php as in https://github.com/mezzio/mezzio-skeleton/blob/4f60efe8a5d96b0178d446268c6fc2586f88df6b/public/index.php#L26 or via Application#init. Thats a problem by-design and IMHO it should have never been implemented like this.

Configuring the application should be part of the config and not a procedural thing as it is right now. When fetching applications from container, it should be configured and immutable (usually same to the container, but that is not always possible I guess).

So if you are using mezzio with the latest skeleton, you will have issues with missing routes as well. Thats why I also encourage every1 who is asking me to use https://github.com/mezzio/mezzio/blob/3.17.x/src/Container/ApplicationConfigInjectionDelegator.php along with configuration-driven routes and pipeline so that these will be registered via config (even tho, the routes should be registered via the router directly but I guess thats another point).

So pretty sure, whatever solution we might find for this specific problem here, there will be other issues where other stuff is not properly working. I guess, the only thing, why this is MVC problem is a thing, is, that we encourage projects to migrate from laminas-console which actually bootstrapped MVC applications and thus thats some kind of a problem when migrating to laminas-cli.

I wonder if we should provide some kind of decorator to allow projects to actually have BC feeling when migrating from laminas-console.

Thoughts @laminas/technical-steering-committee ?

boesing avatar Mar 15 '23 12:03 boesing

@steffendietz First, thank you very much for your response! 👍🏻

Concerning this component (laminas/laminas-cli) however, I fear the initial issue is still not resolved, since the ContainerResolver::resolveMvcContainer code path still omits the bootstrapping life-cycle stage of modules.

It may be frustrating, but we have to deal with the problem: https://github.com/laminas/laminas-cli/issues/106#issuecomment-1462047887

Because request and response are in container, a lot MVC applications in the wild do http request specific initialization in onBootstrap leading to some horrible breakages.

I understand the desire to solve this quickly and easily in the resolveMvcContainer method and call the init method of the application there. Unfortunately, we create new problems because existing applications then break because it is not checked if the route match exists or not, for example. Therefore, the concern and caution here.

You may call the container.php a workaround, I see it as a single step towards a future-proof solution. And yes, some work is still needed on Mezzio, laminas-mvc and laminas-eventmanager to simplify the entire process and add a complete solution:

  • https://github.com/laminas/laminas-mvc/issues/151
  • https://github.com/mezzio/mezzio/issues/147

(Positive side effect: a harmonisation of laminas-mvc and Mezzio.)

froschdesign avatar Mar 15 '23 13:03 froschdesign

ContainerResolver::resolveMvcContainer code path still omits the bootstrapping life-cycle stage of modules.

It is a part of MVC application lifecycle. It should not be used for anything else.

Legacy Application::init() fires bootstrap event and it is way more convenient to add onBootstrap() to module than any other approach like container delegator factories or registering listener for ModuleEvent::EVENT_LOAD_MODULES_POST event from Module::init(). With the frankly lacking documentation a lot of things end up in onBootstrap() that should not be there.

Just recently a user looking for support had their services initialized in listener on MvcEvent:EVENT_ROUTE. From my own experience I had applications breaking more often than not by initializing container for cli tooling like doctrine-migrations using Application::init()->getServiceManager()

We can not make assumption for all from a new library and decision was made not to bootstrap MVC as a proper approach. What we failed to do is to provide documentation clearly outlining that.

User application can make assumptions and make application specific decision to fire bootstrap events to setup container. It is why it is acceptable to be done in mvc skeleton but IMO not here.

Only tangentially related, but in MVC v4 module manager will be removed. When it is released assumptions made here will become invalid.

Xerkus avatar Mar 15 '23 13:03 Xerkus

Thats why I also encourage every1 who is asking me to use https://github.com/mezzio/mezzio/blob/3.17.x/src/Container/ApplicationConfigInjectionDelegator.php along with configuration-driven routes and pipeline so that these will be registered via config (even tho, the routes should be registered via the router directly but I guess thats another point).

I actually don't recommend that, in part because the configuration is so difficult to write, difficult to debug, and because configuration merging issues are rampant. I tend to recommend using a delegator on the Application instance or the RouteCollectorInterface, and then register using that service. This ensures that the routes are present for your CLI commands as well, as you will get them when pulling the related services.

The idea behind config/routes.php was reasonable for the purpose of RAD-style microframework utilization, but not great as we expanded the framework.

weierophinney avatar Mar 15 '23 15:03 weierophinney

I tend to recommend using a delegator on the Application instance or the RouteCollectorInterface, and then register using that service. This ensures that the routes are present for your CLI commands as well, as you will get them when pulling the related services.

I would love to do so as well but sadly there is no such delegator as of now. I do not expect every1 to write such a delegator himself and thus point to the application config injection delegator.

Might be worth creating one in mezzio/mezzio which then replaces the existing one. Gonna create a feature request for that.

boesing avatar May 08 '23 21:05 boesing

I would love to do so as well but sadly there is no such delegator as of now. I do not expect every1 to write such a delegator himself and thus point to the application config injection delegator.

Correct, that cannot be the goal, that a user has to create this himself.

froschdesign avatar May 09 '23 07:05 froschdesign

Hello, the same case: migrated to laminas-cli and got onBootstrap-issue.

in onBootstrap mostly we:

  • attach events
  • set_error_handler
  • register_shutdown_function

i have read the whole discussing, but not clearly understood what is the suggested way to use laminas-cli and onBootstrap-logic?

logic here adds code inside index.php, which AFAIU is not used by laminas-cli (it runs via ./vendor/bin/laminas)

Question

what is the workaround? where we should put our logic to make it work for web and cli simultaneously?

Thanks!

lon9man avatar Oct 13 '23 12:10 lon9man

@lon9man

logic here adds code inside index.php, which AFAIU is not used by laminas-cli (it runs via ./vendor/bin/laminas)

The important part is not the change in the index.php file but the new file container.php in the config folder. This file will be used by laminas-cli:

https://github.com/laminas/laminas-cli/blob/97930db3fd2550da0374a3ccbdc7f9b9d6177319/src/ContainerResolver.php#L55-L60

This calls the init method of Laminas\Mvc\Application:

https://github.com/laminas/laminas-mvc/blob/403c310d07f66f67ebbe26f39071ab859bb38e3b/src/Application.php#L221-L240

And then the bootstrap method:

https://github.com/laminas/laminas-mvc/blob/403c310d07f66f67ebbe26f39071ab859bb38e3b/src/Application.php#L116-L126

Which includes the MvcEvent::EVENT_BOOTSTRAP event:

https://github.com/laminas/laminas-mvc/blob/403c310d07f66f67ebbe26f39071ab859bb38e3b/src/Application.php#L138-L148

froschdesign avatar Oct 13 '23 12:10 froschdesign

@froschdesign, thanks for the fast and valuable response! it seems works.

maybe it should be some big red note 'DANGER-DANGER' in docs for this important edge case? module-manager laminas-cli

lon9man avatar Oct 13 '23 12:10 lon9man

Just loading modules should be enough to have application setup for basic usage.

Logic not specific to MVC and web should not really be in onBootstrap(). That logic however is in onBootstrap().

config/container.php from skeleton is a compromise that does questionable if not wrong thing and fires bootstrap event. Because it is decided in application and not in this package it is acceptable. It allows application to easily ditch bootstrap event for cli usage.

Xerkus avatar Oct 13 '23 12:10 Xerkus

@froschdesign, thanks for the fast and valuable response! it seems works.

maybe it should be some big red note 'DANGER-DANGER' in docs for this important edge case? module-manager laminas-cli

Why add this to the documentation? let the idiots spoil their nerves and then switch to Laravel

oleg-moseyko avatar Oct 13 '23 12:10 oleg-moseyko

@lon9man

in onBootstrap mostly we:

  • attach events

Can be done in the configuration: https://docs.laminas.dev/laminas-eventmanager/application-integration/usage-in-a-laminas-mvc-application/#register-listener

maybe it should be some big red note 'DANGER-DANGER' in docs for this important edge case?

Correct, the documentation needs to be expanded, but a warning about a danger is a bit too much.

froschdesign avatar Oct 13 '23 12:10 froschdesign