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: ViewInterface returns PSR StreamInterface #3286

Merged

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jan 27, 2024

Neos adjustments neos/neos-development-collection#4856

  • the views are now independent of the ControllerContext
    • ViewInterface::setControllerContext is not part of the interface anymore and will only be called on demand
  • the ActionRequest if necessary can be accessed via the variable "request" (like "settings")
  • ViewInterface::canRender was required for fallbackviews which have been removed long ago, and so this artefact will be removed as well.
  • !!! the return type is now forced to be either a ResponseInterface or a StreamInterface. Simple strings must be wrapped in a psr stream! (see StreamFactoryTrait::createStream)

Related to #3232

@github-actions github-actions bot added the 9.0 label Jan 27, 2024
@mhsdesign
Copy link
Member Author

mhsdesign commented Jan 27, 2024

@kitsunet That might be in conjunction with #3232 an actual step to make the controllerContext obsolete?

But to still support martins use case and have the response always available #3232 (comment) i introduced a new ActionMessages dto, wich i pass secretly to the view under the key actionMessages like we do it with settings:

 final readonly class ActionMessages
 {
     private function __construct(
         public ActionRequest $request,
         public ActionResponse $response
     ) {
     }

     public static function create(ActionRequest $request, ActionResponse $response)
     {
         return new self($request, $response);
     }
 }

This is the secret passing logic where we make the setControllerContext optional and only call it if existent:

$this->view->assign('actionMessages', ActionMessages::create($this->request, $this->response));
if (method_exists($this->view, 'setControllerContext')) {
    $this->view->setControllerContext($this->controllerContext);
}

One could say that nothing changed but i think the ActionMessages is an improvement over the controller context as it offers less weird things.

Of course it would be ideal to not have to pass the pre-response anywhere but just work with the request - like my first commit shows, but that wouldnt allow many complex usecases. Edit no see my next comment...

@mhsdesign
Copy link
Member Author

Of course it would be ideal to not have to pass the pre-response anywhere but just work with the request - like my first commit shows, but that wouldnt allow many complex usecases.

yes that would be the goal. To only pass the request but not the Response outside of the view and its possible!!!

In this pr i found a way to make the neos fusion view work with plugins and fusion forms despite having no controller context at hand. neos/neos-development-collection#4856

It works by creating a dummy controller context in the view and later rendering out that response from the view. That gives us the possibility to actually decouple views and controllers a lot. Simply by using the feature to render a psr response from a view.

The same i did in a much smaller scope for the json view, so that the content type is set automatically.

My prior ActionMessages gedöns has been reverted.

@mhsdesign mhsdesign changed the title Feature/view interface without controller context FEATURE: ViewInterface independent of ControllerContext Jan 30, 2024
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Feb 25, 2024
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)
```
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Feb 25, 2024
The Neos `FusionView` is forward compatible for this change neos/flow-development-collection#3286
Assign should be used for setting the request:
```
$view->assign('request"', $this->request)
```
@mhsdesign mhsdesign changed the title FEATURE: ViewInterface independent of ControllerContext !!! FEATURE: Overhaul ViewInterface Mar 13, 2024
@mhsdesign
Copy link
Member Author

mhsdesign commented Mar 13, 2024

About ViewInterface::canRender

Initially canRender was introduced to allow special logic in a controller to determine the correct view out of multiple options. cb8fa24

But with d69bcfe the canRender() checks were removed and leaves the method with zero usages and no real implementation.

@mhsdesign mhsdesign changed the title !!! FEATURE: Overhaul ViewInterface !!! FEATURE: PSR ViewInterface without controller context Mar 15, 2024
@mhsdesign mhsdesign force-pushed the feature/viewInterfaceWithoutControllerContext branch from 3ff7973 to 7b568f6 Compare March 15, 2024 10:02
@mhsdesign mhsdesign force-pushed the feature/viewInterfaceWithoutControllerContext branch from 676ea9a to 7b04e7b Compare March 16, 2024 19:15
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Mar 17, 2024
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)
```
@mhsdesign mhsdesign marked this pull request as ready for review March 17, 2024 15:33
@@ -26,53 +25,45 @@ interface ViewInterface
/**
* Sets the current controller context
*
* @deprecated if you absolutely need access to the current request please assign a variable.
Copy link
Member

Choose a reason for hiding this comment

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

Will this even show in IDEs with the function commented out? bit nasty I guess 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

documenting this via

@method setControllerContext(ControllerContext $controllerContext) @deprecated
on the view-interface will not have the desired effect of annotating it as deprecated but would still allow goto clicking.

In the end all views extend the AbstractView which still contains a definition for setControllerContext (but deprecated) so i guess its fine?

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.

I think the more important headline this PR should have is View returns Stream, because that will hit many peoples feet updating to this. I think it's a good direction, minor comments, probably overlooked osmething, probably something needs fixing in the future, but we should do it.

@mhsdesign mhsdesign changed the title !!! FEATURE: PSR ViewInterface without controller context !!! FEATURE: ViewInterface returns PSR StreamInterface Mar 21, 2024
@mhsdesign mhsdesign requested a review from mficzel March 21, 2024 09:05
{
$this->actionController = $this->getAccessibleMock(ActionController::class, ['resolveActionMethodName', 'initializeActionMethodArguments', 'initializeActionMethodValidators', 'resolveView', 'callActionMethod']);
$this->actionController->method('resolveActionMethodName')->willReturn('indexAction');

$this->inject($this->actionController, 'objectManager', $this->mockObjectManager);
$this->inject($this->actionController, 'controllerContext', $this->mockControllerContext);
$this->inject($this->actionController, 'request', $this->mockRequest);
Copy link
Member Author

Choose a reason for hiding this comment

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

One last question, do we want to use request as naming here or preserve that slot for a pretty psr request, despite that in Fusion request is an ActionRequest (which will not change anytime?)

Do we all agree to keep the ActionRequest for some time and have it hold all the context, like matched controller, base uri and routing parameters?

Initially `canRender` was introduced to allow special logic in a controller to determine the correct view out of multiple options.
neos@cb8fa24

But with neos@d69bcfe the canRender() checks were removed and leaves the method with zero usages and no _real_ implementation.
…face`

Please adjust your view to return a `StreamInterface` instead of a stream.
If you need a string at call site, please use `->getContent()` instead.
@mhsdesign mhsdesign force-pushed the feature/viewInterfaceWithoutControllerContext branch from aa7a41e to 524f61e Compare April 22, 2024 07:51
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Apr 22, 2024
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)
```
@mhsdesign mhsdesign merged commit 35386a2 into neos:9.0 Apr 24, 2024
7 checks passed
@mhsdesign mhsdesign deleted the feature/viewInterfaceWithoutControllerContext branch April 24, 2024 18:30
neos-bot pushed a commit to neos/fusion that referenced this pull request Apr 24, 2024
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)
```
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.

None yet

3 participants