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

Modules of a MvcApplication are not properly bootstrapped #106

Open
steffendietz opened this issue Dec 7, 2022 · 34 comments
Open

Modules of a MvcApplication are not properly bootstrapped #106

steffendietz opened this issue Dec 7, 2022 · 34 comments
Labels
Bug Something isn't working

Comments

@steffendietz
Copy link

steffendietz commented Dec 7, 2022

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 steffendietz added the Bug Something isn't working label Dec 7, 2022
@froschdesign
Copy link
Member

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.

@boesing
Copy link
Member

boesing commented Dec 7, 2022

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.

@steffendietz
Copy link
Author

steffendietz commented Dec 7, 2022

@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
Copy link
Author

@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.

@froschdesign
Copy link
Member

@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)

@nusphere
Copy link

nusphere commented Dec 7, 2022

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.

@froschdesign
Copy link
Member

@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! 👍🏻

@nusphere
Copy link

nusphere commented Jan 6, 2023

is something open or done?

@Ocramius
Copy link
Member

Ocramius commented Jan 6, 2023

@nusphere not AFAIK

@FalkHe
Copy link

FalkHe commented Mar 8, 2023

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)

@Xerkus
Copy link
Member

Xerkus commented Mar 8, 2023

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.

@FalkHe
Copy link

FalkHe commented Mar 9, 2023

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

steffendietz pushed a commit to steffendietz/laminas-cli that referenced this issue Mar 9, 2023
@Xerkus
Copy link
Member

Xerkus commented Mar 9, 2023

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.

Xerkus/laminas-mvc-skeleton@1a242e4

@FalkHe
Copy link

FalkHe commented Mar 9, 2023

@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.

@Xerkus
Copy link
Member

Xerkus commented Mar 9, 2023

Changes now were made to MVC skeleton application to include config/container.php which will fire MVC onBootstrap() events. laminas/laminas-mvc-skeleton#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.

@boesing
Copy link
Member

boesing commented Mar 9, 2023

@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:

#92 (comment)

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

@froschdesign
Copy link
Member

@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:

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

@steffendietz
Copy link
Author

steffendietz commented Mar 15, 2023

@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. 👍

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.

@boesing
Copy link
Member

boesing commented Mar 15, 2023

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 ?

@froschdesign
Copy link
Member

@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: #106 (comment)

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:

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

@Xerkus
Copy link
Member

Xerkus commented Mar 15, 2023

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.

@weierophinney
Copy link
Member

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.

@boesing
Copy link
Member

boesing commented May 8, 2023

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.

@froschdesign
Copy link
Member

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.

@lon9man
Copy link

lon9man commented Oct 13, 2023

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!

@froschdesign
Copy link
Member

froschdesign commented Oct 13, 2023

@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:

$mezzioContainer = sprintf('%s/config/container.php', $this->projectRoot);
Assert::stringNotEmpty($mezzioContainer);
if (file_exists($mezzioContainer)) {
return $this->resolveContainerFromAbsolutePath($mezzioContainer);
}

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

@lon9man
Copy link

lon9man commented Oct 13, 2023

@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

@Xerkus
Copy link
Member

Xerkus commented Oct 13, 2023

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.

@oleg-moseyko

This comment was marked as abuse.

@froschdesign
Copy link
Member

@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.

@lon9man
Copy link

lon9man commented Oct 13, 2023

@Xerkus

for basic usage

only hello-world is basic) any else very often is custom.

Logic not specific to MVC and web should not really be in onBootstrap()

ok, onBootstrap - bad place.
what is the good place or way?

class Module
{
	public function getConfig(): array
	{
		$provider = new ConfigProvider();

		return $provider->getConfig();
	}

	public function onBootstrap( Event $e )
	{
		$application    = $e->getApplication();
		$serviceManager = $application->getServiceManager();
		$eventManager   = $e->getTarget()->getEventManager();

		$notificationListener = $serviceManager->get( EventListener::class );
		$notificationListener->attach( $eventManager );
	}
}

EventListener::class globally tracks insert/update/delete of models into database.
others onBootstrap looks similarly with different target functionality (events of LmcUser and etc).

so what is the correct place (without onBootstrap) nowadays, which will cover web and cli simultaneously?

Thanks!

UPD. froschdesign answered already, what is your point of view?

@Xerkus
Copy link
Member

Xerkus commented Oct 13, 2023

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

Ideally in container configuration. Factories/delegator factories.

For example, this delegator factory adds listeners to listener provider instance https://github.com/laminas/laminas.dev/blob/f872adb6eed0c0196d8026b78fea44c553f4c8fd/src/GitHub/ListenerProviderDelegatorFactory.php
Whenever listener provider is pulled from container it is already configured, no special bootstrap event is needed (and in case of mezzio does not exist).

In mvc a go to method to setup anything is on bootstrap and heavier it is loaded with setting up http handling logic the less suitable it is for cli usage. onBootstrap is in a way a remnant of ZF1 init system from one and half decade ago. A different design philosophy. Sure it was convenient for migrating back then but now DI container is expected to provide ready for use instances.

@Xerkus
Copy link
Member

Xerkus commented Oct 13, 2023

onBootstrap is not really that much of a problem, if it works for you you can use it. Up to you to ensure it is compatible for cli usage.

In context of this package we can not assume what is happening in onBootstrap. Eg quite a few applications out there outright require http request to perform logic and can not be invoked outside of http request handling context.

Moving this decision onto application with the config/container.php allows application developers to take responsibility and decide which way suits their case better.

What needs to be done here is documentation covering how to setup said file with or without calling bootstrap.

@boesing
Copy link
Member

boesing commented Oct 13, 2023

IMHO, the whole eventmanager documentation is kinda flawed.
There are a bunch of ways to register stuff to whatever event manager (both listeners keys are only handled by laminas-mvc), i.e.:

return [
    'listeners' => [], // whatever listener is listed here as class-string will be instantiated and attached to the `Application#getEventManager`
];

Then you can register listeners by adding that key to application.config.php for laminas-mvc.
Then you can register delegators listening to EventManager service, i.e. by using a config file like (which works for both laminas-mvc and mezzio):

return [
    'service_manager'  /* or 'dependencies' for mezzio */ => [
         'delegators' => [
              'EventManager' => [
                   ProjectSpecificEventsToEventManagerAttacher::class,
              ],
         ],
    ],
];

along with the delegator:

final class ProjectSpecificEventsToEventManagerAttacher
{
    public function __invoke(ContainerInterface $container, $name, callable $callback): EventManagerInterface
    {
         $notificationListener = $serviceManager->get( EventListener::class );
         $eventManager = $callback();
         $notificationListener->attach($eventManager);
         
         return $eventManager;
    }
}

Imho, none of the examples within the documentation of laminas-mvc nor mezzio do explain on how to attach events properly and thus, most people are kinda lost and/or have added this kind of stuff to onBootstrap as that is how it is recommended in the MVC documentation AFAIR.

This should be removed and instead replaced with the delegator approach: https://docs.laminas.dev/laminas-mvc/examples/#bootstrapping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

10 participants