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

!!! FEATURE: Dispatcher psr overhaul #3311

Merged
merged 41 commits into from
Sep 12, 2024

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Feb 6, 2024

To ease the implementation of a custom controller now use a direct pattern of f(ActionRequest) => PsrResponseInterface.
This is breaking if you implemented your own ControllerInterface or overwrote low level parts and methods of the ActionController.

class Dispatcher
{
    public function dispatch(ActionRequest $request): ResponseInterface;
}

interface ControllerInterface
{
    public function processRequest(ActionRequest $request): ResponseInterface;
}

Also, it's discouraged to manipulate $this->response in controllers (the ActionResponse is deprecated), although it's still supported in actions for now, please consider returning a new response instead.

Explanation of the legacy MVC response object.

Previously Flows MVC needed a single mutable response which was passed from dispatcher to controllers
and even further to the view and other places via the controller context: ControllerContext::getResponse().
This allowed to manipulate the response at every place.
With the dispatcher and controllers now directly returning a response, the mutability is no longer required.
Additionally, the abstraction offers naturally nothing, that cant be archived by the psr response,
as it directly translates to one: ActionResponse::buildHttpResponse()
So you can and should use the immutable psr {@see ResponseInterface} instead where-ever possible.
For backwards compatibility, each controller will might now manage an own instance of the action response
via $this->response AbstractController::$response and pass it along to places.
But this behaviour is deprecated!
Instead, you can directly return a PSR repose \GuzzleHttp\Psr7\Response from a controller:

public function myAction()
{
    return (new Response(status: 200, body: $output))
        ->withAddedHeader('X-My-Header', 'foo');
}

Further the ForwardException does not extend the StopActionException anymore, meaning a try catch must be adjusted.

This is the main PR and it contains

Followup: #3301
Resolves: #3289

This change needs an accompanying adjustment to Neos to adjust the
PluginImplementation as well as Modules: neos/neos-development-collection#4738

Upgrade instructions

WIP Upgrade notes: #3232 (comment)

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

kitsunet and others added 24 commits November 11, 2023 22:14
WIP

This change needs an accompanying adjustment to Neos to adjust the
PluginImplementation as well as Modules.

The new `SimpleActionController` gives you a direct and simple way to
route an ActionRequest and return an ActionReponse with nothing in
between. Routing should work just like with other ActionControllers.

This is breaking if you implemented your own ControllerInterface
or overwrote or expect some of the api methods of the ActionController.
We now use a direct pattern of f(ActionRequest) => ActionResponse
in more places. Adjusting existing controllers should be easy though.

We discourage to manipulate `$this->reponse` in controllers,
although it should still work fine in actions for now, please consider
other options.
Original this method and logic was introduced via: b780118

The previous try catch would still work but mutating `$this->response` AFTER throwing an exception is pretty ugly.
…`afterControllerInvocation`

The parameter `$response` was removed from the `beforeControllerInvocation` signal, as it would be just always empty at that point and misleading and any mutations would be ignored.

The parameter `$response` was made nullable for the `afterControllerInvocation` signal, as it's not set after a `forwardToRequest`.

Also, it's to be noted, that modifying the response in an `afterControllerInvocation` is not always going to take effect, for example if the dispatcher is still in the loop.
Explain that the details/exception message dont matter, and set a default.
…stionmark

!!! TASK: Separate `ForwardException` from `StopActionException`
…econtroller

!!! FEATURE: WIP Dispatcher and controller return `ActionResponse` (simpler controller pattern)
@github-actions github-actions bot added the 9.0 label Feb 6, 2024
@mhsdesign mhsdesign changed the title !!! FEATURE: Dispatcher returns psr responses !!! FEATURE: Dispatcher psr overhaul Feb 22, 2024
This will not open a memory stream
`replaceHttpResponse` is unsafe to use and has hard to tell behaviour.
Instead, we up-merge the headers explicitly
!!! TASK: Deprecate and replace `ActionResponse` in dispatcher
Neos passes an object for the node frontend routing:

expects array<string, string>, array<string, Neos\Neos\FrontendRouting\NodeAddress> given.
@mhsdesign mhsdesign force-pushed the feature/dispatcher-returns-psr-responses branch from 29694c2 to f4c5479 Compare April 24, 2024 18:56
}
return $parentResponse;
// TODO $response is never _null_ at this point, except a `forwardToRequest` and the `nextRequest` is already dispatched == true, which seems illegal af
Copy link
Member Author

Choose a reason for hiding this comment

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

will be fixed via #3301

This method threw an exception since Flow 6.

> The HTTP response only exists after the innermost middleware (dispatch) is done. For that stage use a middleware instead.
@mhsdesign mhsdesign marked this pull request as ready for review June 5, 2024 10:11
Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

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

I like this by reading and it makes a lots of sense. Would lite to test this before finally approving. Will take some tome. Maybe count this as +0.5 in the meantime

@kitsunet kitsunet merged commit dc19cfc into 9.0 Sep 12, 2024
7 of 8 checks passed
@kitsunet kitsunet deleted the feature/dispatcher-returns-psr-responses branch September 12, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DISCUSSION: Replace ActionResponse with ResponseInterface?
3 participants