-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
Comments
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. |
Hey Steffen, yes, you are right. I am not 100% aware of why we decided to avoid calling As a workaround, you can create a <?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. |
@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 I find this is kind of difficult to explain. Replacing the current 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 |
@boesing regarding the use of the Also, Bottomline is: If this is not considered a bug, then I would like to have a pointer to the "idiomatic" |
@boesing @steffendietz |
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. |
Every pull request is welcome. Go for it! 👍🏻 |
is something open or done? |
@nusphere not AFAIK |
To give a use case: I usually set up EventListeners within |
Because request and response are in container, a lot MVC applications in the wild do http request specific initialization in |
I gave it a shot. Let me know your thoughts. |
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. |
What RFC are you refferring to? Could you please explain the reason/source for this decision?
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. |
Changes now were made to MVC skeleton application to include I suggest what happens here is we update documentation detailing how to apply same changes to existing MVC applications to utilize laminas-cli. |
Maybe I had this in mind and just did not remembered correct: @Xerkus we should deprecate listener stuff in init and remove it in v4. listeners should get registered with delegator rather than manual config extraction. |
@steffendietz @nusphere Please see the related changes in the skeleton application:
Can you give feedback on this? Thanks in advance! 👍🏻 |
@froschdesign Concerning this component ( 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 |
TBH, we made bad decisions in the past - or at least it feels to me as that is the case. So you will never have those routes or middlewares available in 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. 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 wonder if we should provide some kind of decorator to allow projects to actually have BC feeling when migrating from Thoughts @laminas/technical-steering-committee ? |
@steffendietz
It may be frustrating, but we have to deal with the problem: #106 (comment)
I understand the desire to solve this quickly and easily in the You may call the
(Positive side effect: a harmonisation of laminas-mvc and Mezzio.) |
It is a part of MVC application lifecycle. It should not be used for anything else. Legacy Just recently a user looking for support had their services initialized in listener on 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. |
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 The idea behind |
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 |
Correct, that cannot be the goal, that a user has to create this himself. |
Hello, in
i have read the whole discussing, but not clearly understood what is the suggested way to use logic here adds code inside Questionwhat is the workaround? Thanks! |
The important part is not the change in the laminas-cli/src/ContainerResolver.php Lines 55 to 60 in 97930db
This calls the And then the Which includes the |
@froschdesign, maybe it should be some big red note 'DANGER-DANGER' in docs for this important edge case? |
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
|
This comment was marked as abuse.
This comment was marked as abuse.
Can be done in the configuration: https://docs.laminas.dev/laminas-eventmanager/application-integration/usage-in-a-laminas-mvc-application/#register-listener
Correct, the documentation needs to be expanded, but a warning about a danger is a bit too much. |
only
ok,
so what is the correct place (without Thanks! UPD. froschdesign answered already, what is your point of view? |
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 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. |
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 What needs to be done here is documentation covering how to setup said file with or without calling bootstrap. |
IMHO, the whole return [
'listeners' => [], // whatever listener is listed here as class-string will be instantiated and attached to the `Application#getEventManager`
]; Then you can register 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 This should be removed and instead replaced with the delegator approach: https://docs.laminas.dev/laminas-mvc/examples/#bootstrapping |
I know there was no activity on this issue in 8 months, but I wanted to know if anyone found a solution? I think, like most people here, that the application should be bootstraps and have all the MVC listener present when calling commands. It is so convoluted to duplicate the code so it works in commands. Using the delegator approach describe above is very complicated since any objects that receives the EventManager in a factory now create infinite loops. Hopefully, a real working solution will be provided soon. Thanks and keep up the good work! |
Already there! Use the same file structure as in the skeleton application and you're done: |
Bonjour @froschdesign , I copied the container.php and the line from the index.php. I pasted those lines in my CommandFactory file since this is the first thing that is executed in my code when I run a command (using the vendor/bin/laminas executable) and it seems to be working. It did output my layout so I have to use ob_start and ob_clean to prevent the output in the terminal. But this still feels like a hack and not something permanent. Unless I don't know where to put the code from the index.php. From what I have done right now, every time I create a new command I will have to put the index.php lines in that new command as well. Anyway, it seems to be working for now, my logs shows the activity from my cronjob. |
Those two files in skeleton are exactly the two files in your application that you need to modify. As I stated prior, bootstrapping MVC application for container usage in other contexts such as cli is wrong. It is required for legacy reasons in older applications but decision to invoke bootstrap event can not be made by this component and must be deferred to the specific app. Application can then do anything needed to get container ready for use in |
Bug Report
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 thebootstrap
lifecycle stage.Based on the Documentation, the
onBootstrap
method should/could be used for:How to reproduce
Use a service somehow modified or configured in the
onBootstrap
method in aCommand
executed throughlaminas/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:
SampleCommand
returns1
.Expected behavior
From my perspective, using commands through
laminas/laminas-cli
in the context of an MvcApplication, should guarantee the same initialisation level as withApplication::init
.In the above example,
SampleCommand
should return0
, as it has access to a bootstrapped version of theSampleService
.The text was updated successfully, but these errors were encountered: