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

Draft
wants to merge 38 commits into
base: 9.0
Choose a base branch
from

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Feb 6, 2024

This is the main PR.

Contains: #3232
Contains: #3294

Neos adjustments: neos/neos-development-collection#4738

Todo;

  • remove \Neos\Flow\Http\RequestHandler::getHttpResponse

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.
…ler.php

Co-authored-by: Marc Henry Schultz <85400359+mhsdesign@users.noreply.github.com>
…`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

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.

None yet

2 participants