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: Render ResponseInterface directly in Fusion Views #4856

Merged
merged 31 commits into from Apr 24, 2024

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jan 28, 2024

Neos Adjustments for neos/flow-development-collection#3286

  • adjusts Views to return StreamInterface or an ResponseInterface
  • adjusts Fusion runtime to return StreamInterface or an ResponseInterface

The fusion runtime will not accept the controller context from outside anymore, but instead has its own legacy layer.
The legacy layer only works in combination with the new entry point in the fusion runtime:

$httpResponse = $fusionRuntime->renderEntryPathWithContext('root', ['node' => $node]);

Fusion's FusionView adjustments:

The current view behaves incorrect. The ViewInterface allows to return string|ActionResponse|ResponseInterface|StreamInterface|\Stringable
But we violate this by returning the raw runtime value instead ... which for all we now could be a string but might also be just as annotated anything else (mixed) not included in the allowed return types, like raw arrays or non string-able objects or integers.
Also previously when using the Neos.Fusion:Http.Message in combination with the view it would actually render the http response as string, so HTTP/ was part of the content and visible in the dom (This is currently broken since Neos 8 and before).

To streamline the behaviour and fix these specialities, the FusionView will return if applicable a ResponseInterface and for simple (string) cases a psr StreamInterface. Both cases have to be anticipated on call site.
The change is not expected to be super breaking if a StreamInterface will be returned as for classes with loose typing this will be converted via __toString.

This is also necessary for Neos.Neos:Plugin and Neos.Fusion.Form to work correctly.

Resolves: #4855

Upgrade instructions

Mutating the response of the Fusion Runtime ($this->getRuntime()->getControllerContext()->getResponse()) is highly internal and only allowed for Neos special cases as the fusion plugin implementation and fusion forms. This will be removed when we find a better concept internally!

Also other usages of the getControllerContext are deprecated like accessing the request the following way:

$this->runtime->getControllerContext()->getRequest();

Instead, the new fusion globals #4425 should be used (which will also guarantee that the request in EEL ${request} is the same as when fetched from a Fusion object:

$possibleRequest = $this->runtime->fusionGlobals->get('request');
if (!$possibleRequest instanceof ActionRequest) {
    // ... fallback
}

If you use the Fusion Runtime directly, which is internal, you should know that using Runtime::render as entry point will fail to render Neos.Neos:Plugin, Neos.Fusion.Form and also lead to weird behaviour when rendering a Neos.Fusion:Http.Message.
The Fusion view should be used for interacting with Fusion, but if you must, you can leverage Runtime::renderEntryPathWithContext to render a full page with all special cases.

Review instructions / WHY

The WHY of this PR might not seem obvious as a lot of pieces fall here into place.

  • We want to deprecate the controller context !!! 9.0 Deprecate controller context flow-development-collection#3152
    • -> But removing usages of the controller context is not easily doable as major parts of the system depend on it, like the fusion runtime
  • This pr is part of a larger movement and will make Fusion independent of the controller context
  • The controller context in Fusion is currently used for 2 major tasks
    1. accessing everything request related (the request itself, the uribuilder and flashmessages)
    2. accessing the current to be send response by object reference
  • the first (i) is easily replaceable by migrating to use the Fusion global "request"
  • the second (ii) is currently used for dark magic in Neos.Neos, we modify the to be send response by adding headers or changing the status code
    • in Neos.Neos:Plugin for sub-request link
    • in Neos.Fusion:Form to redirect on success to another page link
    • instead of creating a slightly prettier but still hacky api for these usecases (like $runtime->setHeader()) we will not change the syntax but deprecate it and internalise it.
    • to still support the usecases the Fusion runtime has to create a mutable object (a stub response) itself where it will extract the changes made to it and apply it to an actual reponse (merge it with the contents).
      • for that to archive we require to have a single entry point in Fusion to render a whole page (renderEntryPathWithContext)
      • and it requires this entrypoint to return something 'more' than the contents of the page, but also its headers: a full psr response
        • -> this is partially breaking
  • to support this change the ViewInterface will be adjusted in Flow to force returning either a StreamInterface or an ResponseInterface, and also the setControllerContext will not be necessary to implement / call any longer on views.

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

Neos.Fusion/Classes/View/FusionView.php Outdated Show resolved Hide resolved
/**
* @test
*/
public function fusionViewCanReturnHttpResponseFromHttpMessagePrototype()
Copy link
Member Author

Choose a reason for hiding this comment

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

this is actually a bugfix as the FusionView dindt handle Neos.Fusion:Http.Message before. Only the Neos FusionView did.

Comment on lines -138 to -146
protected function parsePotentialRawHttpResponse($output)
{
if ($this->isRawHttpResponse($output)) {
return Message::parseResponse($output);
}

return $output;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this hack could be removed from the Neos FusionView and the view will use the new Runtime::renderResponse which will handle this.

Neos.Fusion/Classes/Core/Runtime.php Outdated Show resolved Hide resolved
Neos.Neos/Tests/Unit/Fusion/PluginImplementationTest.php Outdated Show resolved Hide resolved
* @deprecated {@see self::getControllerContext()}
* @internal
*/
public function setControllerContext(ControllerContext $controllerContext): void
Copy link
Member Author

Choose a reason for hiding this comment

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

this method was only introduced in 9.0 dev thats why we can so easily remove it: #4425

@@ -169,23 +167,10 @@ public function setControllerContext(ControllerContext $controllerContext): void
*/
public function getControllerContext(): ControllerContext
{
if (isset($this->controllerContext)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this logic was only introduced in 9.0 dev thats why we can so easily change it: #4425

@mhsdesign mhsdesign changed the title Feature/fusion runtime render http response FEATURE: Render http response in fusion runtime Jan 30, 2024
@mhsdesign mhsdesign marked this pull request as ready for review January 30, 2024 14:36
@mhsdesign mhsdesign force-pushed the feature/fusionRuntimRenderHttpResponse branch from 428afca to a64bd6c Compare January 30, 2024 14:36
@mhsdesign mhsdesign changed the title FEATURE: Render http response in fusion runtime FEATURE: Render ResponseInterface directly in Fusion Runtime Jan 30, 2024
@mhsdesign mhsdesign force-pushed the feature/fusionRuntimRenderHttpResponse branch from a64bd6c to 5d4eb41 Compare January 30, 2024 14:40
@kitsunet kitsunet self-requested a review February 2, 2024 12:37
@mhsdesign mhsdesign force-pushed the feature/fusionRuntimRenderHttpResponse branch 2 times, most recently from 5fbe56b to 27313f9 Compare February 3, 2024 21:44
@mhsdesign mhsdesign force-pushed the feature/fusionRuntimRenderHttpResponse branch from 27313f9 to 12bf4c7 Compare February 14, 2024 21:31
@mhsdesign mhsdesign force-pushed the feature/fusionRuntimRenderHttpResponse branch from 12bf4c7 to b737c01 Compare February 16, 2024 21:54
@mhsdesign mhsdesign changed the title FEATURE: Render ResponseInterface directly in Fusion Runtime !!! FEATURE: Render ResponseInterface directly in Fusion Runtime Feb 18, 2024
@mhsdesign
Copy link
Member Author

mhsdesign commented Feb 22, 2024

I can confirm that a Neos.Neos:Plugin works as described here neos/Neos.Demo#190

Though it needs an additional patch as plugins are currently broken on 9.0-dev anyways: #4909

Things i tested in the controller:

  • forward (content of other action)
  • throwStatus (500 will be upmerged and plugin will show text inside: 500 Internal Server Error
  • redirect (uri will change)
  • set arbitrary header x-mhs: foobar
  • build uri to another action
  • render fluid view
  • render string

@mhsdesign mhsdesign changed the title !!! FEATURE: Render ResponseInterface directly in Fusion Runtime !!! FEATURE: Render ResponseInterface directly in Fusion Views Feb 23, 2024
@mhsdesign mhsdesign force-pushed the feature/fusionRuntimRenderHttpResponse branch 3 times, most recently from bf09d77 to e2f7562 Compare February 23, 2024 18:42
@mhsdesign mhsdesign force-pushed the feature/fusionRuntimRenderHttpResponse branch from 198c755 to 17caac0 Compare February 25, 2024 12:59
…o allow setting headers dynamically

Instead, the legacy layer

> $this->runtime->getControllerContext()->getResponse();

should be continued to be used by fusion forms and fusion plugin impl.
Previously the pattern was that the utmost caller of the runtime would unwrap the exception.
To simplify this, as the runtime now has a single entry point, we add this behaviour into the runtime.
This will be discussed separately and not part of the change of the Neos Node `FusionView`
…`ControllerContext`

`getArguments` removed without replacement.
…ead of `mixed`

This will make the `Neos.Fusion:Http.Message` prototype work out of the box.
This was also done to return a value allowed by the view:
`string|ActionResponse|ResponseInterface|StreamInterface|\Stringable`

Adjusts AbstractFusionObjectTest::buildView to the runtime directly.
We still use a `ViewInterface` here as this allows us to not rewrite all our tests :D
We will rewrite them either way in behat some time.
…ot a string

After a discussion with Christian we found it to be a more workable behaviour than throwing an exception.
Initially introduced the option expected an actual instance of `FusionGlobals`. This is a flawed design as only primitives can be set for example via `Views.yaml`

Additionally, the `FusionView` is forward compatible for this change neos/flow-development-collection#3286
The `fusionGlobals` must not be used for setting the request but rather

```
$view->assign('request"', $this->request)
```
This magic behaviour is not expected and will rather lead to bugs instead of help the integrator to use fusion.

The action controllers `renderView` would previously just silently ignore this case and render a blank page.

Instead, to force to write correct functioning entry points, we throw an exception like:

> Fusion entry path "root" is expected to render a compatible http response body: string|\Stringable|null. Got array instead.
- this will make the change less breaking due to allowing an implicit string cast
- adjust to fluidViews will now return a stream
@mhsdesign mhsdesign force-pushed the feature/fusionRuntimRenderHttpResponse branch 2 times, most recently from 361f2d0 to 13077e6 Compare April 22, 2024 08:08
mhsdesign and others added 2 commits April 22, 2024 10:08
Co-authored-by: Wilhelm Behncke <2522299+grebaldi@users.noreply.github.com>
@mhsdesign mhsdesign force-pushed the feature/fusionRuntimRenderHttpResponse branch from 13077e6 to 2cf100f Compare April 22, 2024 08:09
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Yes!

@mhsdesign mhsdesign merged commit 7a74b72 into neos:9.0 Apr 24, 2024
3 of 9 checks passed
@mhsdesign mhsdesign deleted the feature/fusionRuntimRenderHttpResponse branch April 24, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DISCUSSION: How to avoid in fusion access to $this->runtime->getControllerContext()->getResponse()?
3 participants