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

RFC CLI compatible context for uri building #3354

Open
mhsdesign opened this issue May 23, 2024 · 1 comment
Open

RFC CLI compatible context for uri building #3354

mhsdesign opened this issue May 23, 2024 · 1 comment

Comments

@mhsdesign
Copy link
Member

Goals:

  • discourage using global request state because this won't for in CLI
    • ServerRequest::fromGlobals()
    • $bootstrap->getActiveRequestHandler()->getHttpRequest()
      • BaseUriProvider::generateBaseUriFromHttpRequest
  • get rid of the ControllerContext
  • render fusion in CLI
  • simple uri building in CLI (without the need for a fake PSR HttpRequest)
  • we still want to support sub request (plugins)

1.) Requirements for uri-building

Many parts of the application need to build uris.
Christian Alexander and me determined the current requirements for building an uri:

use Neos\Flow\Mvc\Routing\Dto\RouteParameters;

final class RoutingContextThingy
{
    public function __construct(
        UriInterface $baseUri,
        RouteParameters $parameters,
        array $routeValues
    ) {
    }
}
  • $baseUri
    -> required for building absolute uris
  • $parameters
    -> for special route part handlers like Neos frontend routing where the RequestUriHostMiddleware is used to set a requestUriHost parameter to make the handler host-aware

    The routing should really have access to the host without this, but this is required for historic reasons ~ comment

  • $routeValues
    -> for templates to only have to specify the next action name and the current controller and package will be kept (as routing default values)
    -> for sub requests (plugin and widget framework)
    -> must also be passed around, to resolve a template by controller and package (or a fusion entry path)

So with the above information at hand one could pass this around and create for example an uri-builder satisfy parts requiring the $routeValues or the $baseUri.

2.) Idea ActionRequest vs new context DTO

As it's always a hassle to think of new context DTOs and actually pass them through the whole application, we could consider using the abstraction we already have around request.
The ActionRequest can answer provide the above specified information:

  • $baseUri -> RequestInformationHelper::generateBaseUri($this->getHttpRequest()), though without respecting Neos.Flow.http.baseUri
  • $parameters -> $this->getHttpRequest()->getAttribute(ServerRequestAttributes::ROUTING_PARAMETERS)
  • $routeValues -> $this->getArguments()

The ActionRequest is the non-perfect but still better replacement for the ControllerContext.
In the future we might come across a perfect name and value object structure to encapsulate all that.
But users are currently passing the action requests around (and use new UriBuilder and $uriBuilder->setRequest) thus we will make it just more official and stream lined.
Regarding CLI use currently people have to go to great lengths to create a request:

$httpRequest = new ServerRequest('GET', $baseUri);
$httpRequest = $httpRequest->withAttribute(
    ServerRequestAttributes::ROUTING_PARAMETERS,
    RouteParameters::createEmpty()->withParameter('requestUriHost', $httpRequest->getUri()->getHost())
);
$actionRequest = ActionRequest::fromHttpRequest($httpRequest);

We could think of setting the $baseUri and $parameters state at construction time to make it simple to fake such a request for cli or testing via say ActionRequest::createDecoupled:

class ActionRequest
{
    /** Cached pointer to the (real) http request, if in web context */
    private ?HttpRequestInterface $httpRequest = null;

    /** The parent request – either another sub ActionRequest a main ActionRequest or null */
    private ?ActionRequest $parentRequest = null;

    /** For absolute uri building */
    private UriInterface $baseUri;

    /** for special route part handlers (alternatively all http attributes?) */
    private RouteParameters $parameters;

    public static function createDecoupled( // wip name, create for "easy use" in cli context
        UriInterface $baseUri,
        RouteParameters $parameters,
        array $routingArguments
    ): ActionRequest;

    public static function createForParameterizedHttpRequest(
        HttpRequestInterface $request,
        array $routingArguments
    ): ActionRequest;

    public function createParameterizedSubRequest(
        string $argumentNamespace,
        array $pluginRoutingArguments
    ): ActionRequest;

    public function getBaseUri();
    
    /** Is not safe to rely on, will return null if no HTTP request is at hand */
    public function getHttpRequest(): ?HttpRequestInterface;
}

In the same turn, the following things would be deprecated because we don't accept parameters for the construction and don't want to be super mutable:

  • fromHttpRequest, createSubRequest, setControllerPackageKey, setControllerSubpackageKey, setControllerName, setControllerActionName, setFormat

The "fake" method would mean that getHttpRequest now can return null in CLI context.
As alternative, we could introduce getters such as ActionRequest::getRoutingContext() or ActionRequest::getUriBuilder() to retrieve information on how to build an uri.

3.) Uri-building use-cases

3.1) Special case | Passing the base uri to resource targets (#3334)

Some targets of the resource manager require the current base uri for building resource uris consistently or in to build uris for CLI use.
The context for uri building should expose its base-uri, so it can be extracted from the outside and passed to TargetInterface::getPublicStaticResourceUri,
which will be possible with this in depth proposal: RFC: Dynamic resource uri host (baseUri) for CLI

$resourceManager->getPublicPackageResourceUri('Vendor.Site', 'Foo.js', $context->getBaseUri())

3.2) Passing uri building context to views

We decided to not wanting to have to pass the controller context around any longer and thus deprecated and partially removed ViewInterface::setControllerContext.
In its current implementation via !!! FEATURE: ViewInterface returns PSR StreamInterface, we decided to pass the request as "magic" view variable as an ActionRequest

$this->view->assign('request', $this->request);
if (method_exists($this->view, 'setControllerContext')) {
    $this->view->setControllerContext($this->controllerContext);
}

In combination with keeping the ActionRequest abstraction for the long run this makes perfectly sense.
But for the case we decide that instead of the ActionRequest we want to either operate fully on PSR, or have a separate context for passing around request would be a rather short-lived concept, and we should rethink its naming before releasing 9.0.

3.3) Passing uri building context to EEL helpers or other PHP services

In Neos & Flow 8.3 it was the necessary evil, or rather the pattern we used to pass the ControllerContext around.
Thus also in the Neos.Link.resolveNodeUri helper:

public function resolveNodeUri(string|UriInterface $uri, Node $contextNode, ControllerContext $controllerContext): ?string;

Instead, I propose to deprecate the use of the ControllerContext here and pass the new context thing (as proposed above, the ActionRequest):

public function resolveNodeUri(string|UriInterface $uri, Node $contextNode, ActionRequest $request): ?string;

The request is A) always at hand (in Fusion via ${request}) and B) the ControllerContext contains too much information and is even harder to mock on CLI.
Also, the controller context is not exposed in fusion and thus the helper Neos.Link.resolveNodeUri is actually unusable.

Other examples:

// the neos menu structure contain uris to be build (taken from code in the Neos.Ui)
public function getMenu(ActionRequest $actionRequest): array;

// render the node uri (anywhere really)
public function getNodeUri(Node $node, ActionRequest $actionRequest): UriInterface;

4.) Idea no context. Pass immutable new preconfigured ActionUriBuilder around vs context to create such uri builder

Bastian suggested the following in his comment

Maybe the ActionUriBuilder could become the replacement for the annoying ControllerContext. For example in the Fusion RuntimeFactory

public function create(array $fusionConfiguration, ActionUriBuilder $actionUriBuilder)

If consistently done every place would use the one uri-builder (which must be immutable!) which is constructed once on the boundary.
But Point 3.1 would demand that the uri-builder would need to expose its base uri. And further the logic to determine the template by the current arguments could as well be answered by the state the uri builder upholds but its not its responsibility.
That means for most use-cases we would need to pass the request as well along, and thus not simpling CLI use.

5.) Idea PSR request with attached attributes insteadof ActionRequest wrapper

There also exists the idea to operate at some point purely on PSR requests and attach additional information form the routing match as an API way to the server request attributes.
That is actually currently even done, see ServerRequestAttributes::ROUTING_RESULTS and due to a hack the whole ActionRequest is even attached as instance to ServerRequestAttributes::ACTION_REQUEST.
These attributes does allow some kind of extensibility but under the assumption that we want to keep supporting sub-requests (and thus a request chain) we need an abstraction on top like the current ActionRequest

6.) Discussion, promote using ActionRequeset vs raw PSR in APIs ($request->getHttpRequest())

Having to unwrap the request constantly seems like other parts of Neos and Flow moved already past the ActionRequest wrapper and want to go pure PSR which is a conflicting state and without clear decision we might never move past that (see 5. that we will always need a wrapper to support sub requests).

Keeping the request and rather improving its api and discouraging getHttpRequest makes especially sense when looking from a users' perspective:
Writing this ${request.arguments.myQueryParameter} vs typing request twice ${request.httpRequest.queryParams.myQueryParameter} is much simple and makes more sense.

As it seems like we are about to promote getHttpRequest calls extensively in Neos 9 we should be sure that we want that and not go back later :)

Link collection

Strongly related:

@bwaidelich
Copy link
Member

As discussed, we suggest to add some new DTO that can be used from userland to initialize a Fusion runtime or potentially other code with some routing defaults:

final readonly class RoutingDefaults {
  private function __construct(
    public UriInterface $baseUri,
    public string $pathPrefix,
    public RouteParameters $parameters,
    public string|null $packageKey,
    public string|null $controllerName,
    public string $format,
  ) {
  }

  // and some static constructors like

  public static function fromActionRequest(ActionRequest $actionRequest): self
  {
    // ...
  }

  public static function create(
    UriInterface $baseUri,
    string|null $packageKey = null,
    string|null $controllerName = null,
  ): self
  {
    // ...
  }
}

Internally we would still create a mock ActionRequest instance from those defaults, because that's already being used everywhere.
But for new classes we could refer to that new DTO instead so that we can slowly fade out the ActionRequest dependency over time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

2 participants