Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Rewrite for PSR-7 #259

Closed
wants to merge 51 commits into from
Closed

Rewrite for PSR-7 #259

wants to merge 51 commits into from

Conversation

Xerkus
Copy link
Member

@Xerkus Xerkus commented Nov 17, 2017

Work in progress

Goal of the rewrite is to switch the component to the native support of PSR-7.

PSR-7 Request and Response

PSR-7 is a way into the future. But what does it mean for zend-mvc? Sadly, immutable nature is incompatible with zend-http and switch to PSR-7 constitutes immense backward compatibility break.
In versions 2 and 3, zend-mvc relies heavily on zend-http request and response mutability, where single instance is expected to be shared between many parts, starting from Zend\Mvc\Application to various listeners, controllers, helpers.

To keep zend-mvc architecture changes to a minimum, mutability is shifted to MvcEvent instance, which then becomes a single source of truth throughout the dispatch. If request or response need to be changed, they should be obtained from MvcEvent, changed and then set back.

Intended changes

  • Drop zend-mvc-console support
  • Remove Request from bootstrap stage and make it available only when application is run.
  • Remove Request from Container and Application, make it available only as an execution context, ie obtainable from MvcEvent or passed as a call time parameter.
  • Remove Response from Container and Application.
  • Investigate ResponseFactory vs Response prototype provided by MvcEvent.
  • Make Application act as a PSR-15 Http Server Request Handler
  • Move sending response from finish event to dedicated Emitter, same way as in zend-expressive
  • Drop ModuleRouteListener and related MODULE_NAMESPACE constant usage.
  • Drop RouteMatch from MvcEvent
  • Make use of new RouteResult set as a Request attribute
  • Drop module manager from direct dependencies. By the time application is created modules should already be loaded and config merged.

Console integration removal

Console and HTTP handling needs are different. Listeners and controllers are targeting one or the other most of the time, not both. In practice, many implementations do not expect to receive console requests at all. Besides, as stated above, PSR-7 request and response are not compatible with zend-stdlib request and response.

Considering all, support for console handling will be dropped from zend-mvc in version 4. That will simplify zend-mvc and allow to better focus on HTTP request handling.

Recommended way forward for framework users is to implement console as a separate application, utilizing shared container. It should be trivial with configuration merging and container setup moved out of zend-mvc.
zf-console is recommended for simple needs and Symfony console for more complex cases.

Depending on Request or Response outside of dispatching

Some zend-mvc users are depending on Request availability during bootstrap or even module loading.
This is flawed approach. Request and Response are runtime values and they are not guaranteed to exist or be the same during bootstrap. With container, service creation can happen outside of application runtime as well. To remove a possibility for their abuse, Request and Response are removed from container and application, and provided only during Zend\Mvc\Application::run() as MvcEvent parameters.
Should request specific initialization be needed, it should happen early on mvc routing event.
Dedicated request setup event is likely to be introduced later specifically for that purpose.

ResponseSender replaced with Diactoros ResponseEmitter

Console removal allows zend-mvc to make use of Diactoros reusable PSR-7 response emitters and drop Zend\Mvc\ResponseSender.
Zend\Mvc\Application now composes emitter as a constructor dependency and invokes it in run(). SendResponseListener is removed from finish event and dropped.

Mvc Application as a PSR-15 RequestHandler

Zend\Mvc\Application implements PSR RequestHandler so it could be used as a final handler, for example in middleware pipelines.
Zend\Mvc\Application::run() delegates to Zend\Mvc\Application::handle() and emits returned Response with the help of ResponseEmitter.
Since handle() must return response and not emit it, SendResponseListener is removed from 'finish` mvc event.

ModuleManager is... gone?

Back in the time, module manager was introduced to solve basic packaging support, autoloading, config loading and merging. Number of important things happened since then.
Composer emerged and became a standard package manager in php world. It solved all our autoloading needs. No longer constrained by autoloading, ConfigAggregator was introduced to handle config loading and merging in a simple and clean way.

Service configuration is available before container is created, Zend\ModuleManager\Listener\ServiceListener for various plugin managers could be replaced by regular factories. That fits well with zend-servicemanager v3 push for immutability.

Module::onBootstrap() is just a convenience method for providing listener for zend-mvc bootstrap event. Zend\Mvc\Application factory is now looks for ListenerAggregates in config which then attached to Application's EventManager.

Dropping module manager from zend-mvc greatly reduces Application complexity, makes zend-mvc more consistent with zend-expressive and ConfigProvider use in zend framework components.

New mvc application setup would be immediately familiar for zend-expressive users:

// config/config.php
$aggregator = new ConfigAggregator([
    // ...
    Zend\Mvc\ConfigProvider::class,
    Zend\Router\ConfigProvider::class,

    // Default App module config
    Application\ConfigProvider::class,

    new PhpFileProvider(realpath(__DIR__) . '/autoload/{{,*.}global,{,*.}local}.php'),
    // ...
], $cacheConfig['config_cache_path']);

return $aggregator->getMergedConfig();
// public/index.php

// ...
chdir(dirname(__DIR__));
require 'vendor/autoload.php';
 
(function () {
    $container = require 'config/container.php';
    $app = $container->get(\Zend\Mvc\Application::class);
    $app->run();
})();

Related changes

zendframework/zend-router#40


Note: this PR is a subject for rebases and commit squashes. Ask before using as a basis for your work.

@Xerkus Xerkus added this to the 4.0.0 milestone Nov 17, 2017
@Xerkus Xerkus self-assigned this Nov 17, 2017
@zendframework zendframework deleted a comment from Xerkus Nov 28, 2017
@thomasvargiu
Copy link

The idea looks good for me, I would like a stateless flow with immutable request and response. I don’t like to have state of MvcEvent, request and response in the application. A stateless flow would help to allow never-end process to serve requests without bootstrapping application at every request. I know that the framework isn’t the best suitable for this, but... why not? Right? :)

@@ -120,8 +119,8 @@ public function isEnabled()
/**
* @param bool $enabled
*/
public function setEnabled($enabled)
public function setEnabled(bool $enabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we need this method if it can be configurable by constructor. Or are we need enable/disable flag, maybe detach functionality in event manager is better option?

}

/**
* Get application instance
*
* @return ApplicationInterface
*/
public function getApplication()
public function getApplication() : ApplicationInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Application is not required by constructor, so can be null. I think that application is required to mvc event existent.

*/
public function __construct(
ServiceManager $serviceManager,
ContainerInterface $container,
Copy link
Contributor

Choose a reason for hiding this comment

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

Require all parameters and avoid injecting container here will simplify implementation IMO. Getting stuff (or choose default instances) we can move to ApplicationFactory

@fezfez
Copy link

fezfez commented Jun 1, 2018

@Xerkus : Very interesting PR, do you need help on that ?

@michalbundyra
Copy link
Member

@Xerkus what we are doing with this PR? We need to do something before migrating to Laminas.

@Xerkus
Copy link
Member Author

Xerkus commented Dec 12, 2019

@michalbundyra all PSR changes will happen under Laminas, all WIP to be closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants