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: Add createFromEnvironment factory method to ControllerContext #1502

Closed

Conversation

grebaldi
Copy link
Contributor

This introduces a factory method to
\Neos\Flow\Mvc\Controller\ControllerContext that has a similar purpose
as and works similarly to
\Neos\Flow\Http\Request::createFromEnvironment().

It allows to quickly create an ad-hoc controller context outside of the
usual http process chain.

Oftentimes, especially when doing extensive work with Fusion, it is necessary to spin up objects that require a ControllerContext. I've seen workarounds written for those ad-hoc contexts all over the place and have been in the situation of awkwardly writing them myself quite a few times. So I figured it might be worth providing a standard short-hand way of creating them.

Yet, I'm not entirely sure, whether this is a good idea, since it still feels like a workaround. Creating a FusionRuntime outside the MVC framework and providing it with an otherwise meaningless ControllerContext feels like I'm doing something I'm not supposed to be doing.

On the other hand, this pattern is quite common and currently it is pretty tedious to create a ControllerContext out of the blue.

So, let's discuss this. Maybe it is sufficient to provide the method and simply denying it an @api annotation. Maybe there's a good reason for not having anything like this, that I'm not seeing here.

This introduces a factory method to
\Neos\Flow\Mvc\Controller\ControllerContext that has a similar purpose
as and works similarly to
\Neos\Flow\Http\Request::createFromEnvironment().

It allows to quickly create an ad-hoc controller context outside of the
usual http process chain.
@grebaldi grebaldi force-pushed the feature/ad-hoc-controller-context branch from b36e289 to 600021d Compare January 25, 2019 10:24
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.

Looking good and works as expected.

@kitsunet
Copy link
Member

Given that I would rather want to get rid of this thing (and I think @bwaidelich too) I am not a big fan to make it easier to create one in user land code. ;)

@mficzel
Copy link
Member

mficzel commented Jan 25, 2019

@kitsunet killing it with fire is a good option as we already have plenty of context. However this has to be done in a major as i would consider it quite breaky.

I would be ok with adding this convenience now but mark it deprecated immediately, together with the whole ControllerContext thingy.

@bwaidelich
Copy link
Member

What @kitsunet says - ControllerContext is one of the most misused classes in Flow & Neos IMO. But I'm also fine with a work around that makes usage easier until this beast is gone :)

@albe
Copy link
Member

albe commented Apr 20, 2019

Just a notice, if this work-around should land, it will need to be adjusted once the PSR-7 change lands, since creating a Http Request and Response probably won't be that easy (and really shouldn't). Also, the ControllerContext should only depend on an ActionRequest and ActionResponse (see #1531). We should talk about what usage outside the MVC framework means exactly though. I do think there is some insight into how we should better organize the stack hidden.

@albe
Copy link
Member

albe commented May 19, 2019

@grebaldi Regarding the requirement to "Creating a FusionRuntime outside the MVC framework", I think for those cases you should really just use the RuntimeFactory, which will itself create a more or less meaningfull ControllerContext so the FusionRuntime works.

Do you have any other examples where you need to manually create a ControllerContext? The only other thing I can think of is for Fluid Views, e.g. to render an e-mail or other template outside the MVC. But for those the StandardView already is a good solution.

@albe
Copy link
Member

albe commented Mar 18, 2020

I'll close this for now to keep open PRs down - feel free to reopen with new information :)

@albe albe closed this Mar 18, 2020
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

6 participants