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.

Sign up for GitHub

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

Fix mvc bootrapping #110

Draft
wants to merge 4 commits into
base: 1.9.x
Choose a base branch
from
Draft

Fix mvc bootrapping #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58cb9cc
enabled bootstrapping of MVC Application
FalkHe Mar 9, 2023
890a8b8
added documentation for config option 'bootstrap_mvc_application'
FalkHe Mar 9, 2023
141e188
PHPCS fixes
FalkHe Mar 9, 2023
794e80a
PHPCS fixes
FalkHe Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions docs/book/intro.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,30 @@ class Module
}
```

### MVC Application bootstrapping

Until 1.9.x laminas-cli did not bootrap the MVC Application and therefore left the Application in an unready state
wehen executing commands via laminas-cli. (read this [Issue](https://github.com/laminas/laminas-cli/issues/106))

To allow bootstrapping the MVC Application, without breaking backward compatibility, the option `'bootstrap_mvc_application'`
was introduced. Currently it's `false` by default to not break any Apps. This might change in the future.

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

```php
// File config/application.config.php

<?php

return [
/* ... */
'laminas-cli' => [
// execute Laminas\Mvc\Application::init(), including ::boostrap() during initialization of cli app
'bootstrap_mvc_application' => true,
]
];
```

## Integration in Other Applications

laminas-cli supports [Laminas MVC](https://github.com/laminas/laminas-mvc-skeleton)
Expand Down
35 changes: 26 additions & 9 deletions src/ContainerResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use InvalidArgumentException;
use Laminas\ModuleManager\ModuleManagerInterface;
use Laminas\Mvc\Application;
use Laminas\Mvc\Service\ServiceManagerConfig;
use Laminas\ServiceManager\ServiceManager;
use Laminas\Stdlib\ArrayUtils;
Expand All @@ -18,6 +19,9 @@
use function file_exists;
use function sprintf;
use function str_contains;
use function trigger_error;

use const E_USER_DEPRECATED;

/**
* @internal
Expand Down Expand Up @@ -98,18 +102,31 @@ private function resolveMvcContainer(string $path): ContainerInterface
Assert::isMap($appConfig);
}

$servicesConfig = $appConfig['service_manager'] ?? [];
Assert::isMap($servicesConfig);
Assert::classExists(Application::class);

if ($appConfig['laminas-cli']['bootstrap_mvc_application'] ?? false) {
// initialize & bootstrap MVC Application
$mvcApplication = Application::init($appConfig);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this quick fix…

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$serviceManager = $mvcApplication->getServiceManager();
} else {
/* @deprecated MVC Application is not bootstrapped */
trigger_error('Running laminas-cli in MVC Environment without bootstrapping '
. 'the MVC Application is deprecated. '
. '@see https://github.com/laminas/laminas-cli/issues/106', E_USER_DEPRECATED);

$smConfig = new ServiceManagerConfig($servicesConfig);
$servicesConfig = $appConfig['service_manager'] ?? [];
Assert::isMap($servicesConfig);

$serviceManager = new ServiceManager();
$smConfig->configureServiceManager($serviceManager);
$serviceManager->setService('ApplicationConfig', $appConfig);
$smConfig = new ServiceManagerConfig($servicesConfig);

$moduleManager = $serviceManager->get('ModuleManager');
Assert::isInstanceOf($moduleManager, ModuleManagerInterface::class);
$moduleManager->loadModules();
$serviceManager = new ServiceManager();
$smConfig->configureServiceManager($serviceManager);
$serviceManager->setService('ApplicationConfig', $appConfig);

$moduleManager = $serviceManager->get('ModuleManager');
Assert::isInstanceOf($moduleManager, ModuleManagerInterface::class);
$moduleManager->loadModules();
}

return $serviceManager;
}
Expand Down