!!! FEATURE: Introduce ActionUriBuilder

[bwaidelich]

Introduce ActionUriBuilder as more solid building block to create relative and absolute URLs for MVC actions.

This change should not be breaking, but it deprecates some classes that were previously part of the public API:

• Deprecate UriBuilder (in favor of the new ActionUriBuilder)
• Deprecate BaseUriProvider and the use of the Neos.Flow.http.baseUri setting
• Deprecate ControllerContext
• Deprecate Environment::isRewriteEnabled()

Usage of the new ActionUriBuilder:

Obtain an instance:

In Controllers (extending AbstractController) the new ActionUriBuilder is already available via $this->actionUriBuilder. For other usecases, the ActionUriBuilderFactory can be used to obtain an instance:

// inject the factory via constructor injection

public function __construct(private ActionUriBuilderFactory $uriBuilderFactory) {}

// ...or property injection:

/**
 * @var ActionUriBuilderFactory
 * @Flow\Inject(lazy=false)
 */
protected ActionUriBuilderFactory $uriBuilderFactory;

// obtain an instance from an action request

$uriBuilder = $uriBuilderFactory->createFromActionRequest($actionRequest);

// ...from an HTTP request

$uriBuilder = $uriBuilderFactory->createFromHttpRequest($httpRequest);

// ...from a custom base URI (e.g. for CLI usage):

$uriBuilder = $uriBuilderFactory->createFromBaseUri(new Uri('http://localhost'));

Build action URLs:

// relative URL to a specified action
$this->actionUriBuilder->uriFor(Action::create($packageKey, $controllerName, $actionName));

// absolute URL to a different action of the current controller
$this->actionUriBuilder->absoluteUriFor(Action::fromActionRequest($this->request)->withActionName('anotherAction'));

// relative URL to the current action with some additional arguments
$this->actionUriBuilder->uriFor(Action::fromActionRequest($this->request)->withAdditionalArguments(['order' => true]));

// custom format
$this->actionUriBuilder->uriFor(Action::fromActionRequest($this->request)->withFormat('xml'));

// custom query parameters / fragment (not part of the route)
$this->actionUriBuilder->uriFor(Action::fromActionRequest($this->request))->withQuery('foo=bar')->withFragment('baz');

Related: #2157

[mhsdesign]

Hi what is the state of this, ist this ready to be reviewed and ready for 8.2?

[mficzel]

Generally I think the uriFor method should have the distinct parameters “routingArguments” and “queryParameters“.

I know this can be done via -> withQuery but it could help to make the distinction clear. This should also include the merging of previous query parameters from appendExceedingArguments wich otherwise would have to be handled separately every time.

[bwaidelich]

Generally I think the uriFor method should have the distinct parameters “routingArguments” and “queryParameters“.

I agree. Query parameters are not part of the routing framework, but they are definitely a concern of the URI building. Also the UriInterface::withQuery() interface is cumbersome.

I'm just not sure whether it should be another argument to the uriFor method (I tried to introduce more single argument methods with this change) or a separate method on the builder, e.g.:

$this->actionUriBuilder
  ->uriFor(
      Action::fromActionRequest($this->request)->withArguments(['order' => true]),
      ['foo' => 'bar']
  );

vs

$this->actionUriBuilder
  ->uriFor(
    Action::fromActionRequest($this->request)->withArguments(['order' => true])
  )
  ->withQueryParameters(['foo' => 'bar']);

[mficzel]

@bwaidelich I think it is important to make clear that there are routingParameters and that you can also have queryParameters at the same time. Both routingParameters and queryParameter should be on the same api level so developers that see one also implicitly see the other.

So i would think both belong to the Action-DTO which could have withRouteParameters, withQueryParameters plus withAdditionalRouteParameters, withAdditionalQueryParameters. I personally would add both arguments also to the create method this is a complex task and justifies a more complex api.

[bwaidelich]

I think it is important to make clear that there are routingParameters and that you can also have queryParameters

Yes, but for the same reason I would suggest to stick to routingArguments vs queryParameters (even though we mixed up the two in the past in a couple of places).

So i would think both belong to the Action-DTO

That was my first impression, too.. But the DTO in the current form describes a MVC action and that has nothing to do with query parameters. But maybe it's just a naming issue. I wasn't 100% happy with the name "Action" anyways

[mficzel]

But maybe it's just a naming issue. I wasn't 100% happy with the name "Action" anyways

Agree, if it is called "Action" i would expect it to separate routingArguments and queryParameters internally which is the problem we are currently stuck in. Maybe "UriBuildingRequest" or "ActionUriSpecification"

[albe]

But maybe it's just a naming issue. I wasn't 100% happy with the name "Action" anyways

Agree, if it is called "Action" i would expect it to separate routingArguments and queryParameters internally which is the problem we are currently stuck in. Maybe "UriBuildingRequest" or "ActionUriSpecification"

Agree that "Action" is not a good name here. What it actually does is take the description of an Action and turn it into routing values (or parameters?). So what about ActionRoute?

[mficzel]

One thing i am not totally sure about is how this would affect uriBuilding in Fluid/Fusion especially when subrequests are involved as is the case inside of backend modules or widgets.

[bwaidelich]

My comment from yesterday wasn't really well-thought-out and the code example didn't make sense.

Thinking about it again and sticking to the current API (for example having $uriBuilder->withAddedRouteParameters(...) I would suggest to keep Action since it actually describes what MVC action to invoke and add an additional withAddedQueryParameters() and/or withAddedQueryParameter() to the builder:

$this->actionUriBuilder
  ->withAddedQueryParameter('foo', 'bar')
  ->uriFor(Action::fromActionRequest($this->request)->withArguments(['order' => true]));

One thing i am not totally sure about is how this would affect uriBuilding in Fluid/Fusion especially when subrequests are involved as is the case inside of backend modules or widgets.

Shouldn't that be possible with

$uriBuilderFactory->createFromActionRequest($subRequest)->uriFor(...);

?

[bwaidelich]

..and one additional thought: "Builder" is a bit quirky. especially with the combination of a "builder factory". Maybe it makes more sense to just call this ActionUri:

$actionUri = $uriFactory
    ->createFromActionRequest($actionRequest)
    ->for(Action::name('show'))
    ->withArguments(['id' => $entityId])
    ->withAddedQueryParameter('foo', 'bar');

// (string)$actionUri === '/entity/<id>?foo=bar'
// $actionUri->value === UriInterface of '/entity/<id>?foo=bar'
// $actionUri->absolute()->value === UriInterface of 'https://domain.tld/entity/<id>?foo=bar'

[mficzel]

Would prefer that

$actionUri = $uriFactory
    ->createFromActionRequest($actionRequest)
    ->for(Action::name('show'))
    ->withRoutingArguments(['id' => $entityId])
    ->withQueryArguments(['foo', 'bar']);

We should use the term parameter or argument consistently.

[bwaidelich]

We should use the term parameter or argument consistently.

it is always queryparameter and routeargument (or at least it should be IMO)

I'm fine with an array, but it should be clear that the query parameters are added to any existing parameters from the routing. That's why I named it "withAdded*"

[mficzel]

I'm fine with an array, but it should be clear that the query parameters are added to any existing parameters from the routing. That's why I named it "withAdded*"

I know. I only would prefer the nicer name that pretends we already removed "appendExceedingArguments". After that removal withQueryParameters is 100% correct.

[bwaidelich]

I pushed an update introducing the ActionUriSpecification DTO as discussed.

To be discussed is whether this object should also have a notion of queryParameters and fragment (and potentially other parts of the Uri). I left it out for now.

Example:

For the Neos preview route this would mean, changing

$uriBuilder
      ->reset()
      ->setSection($section)
      ->setFormat($format ?: $request->getFormat())
      ->setCreateAbsoluteUri($absolute)
      ->uriFor('preview', ['node' => $node], 'Frontend\Node', 'Neos.Neos');

to something like:

$actionSpecification = ActionUriSpecification::for(Action::create('Neos.Neos', 'Frontend\Node', 'preview'))
  ->withAdditionalArguments(['node' => $node])
  ->withFormat($format ?: $request->getFormat())
$uri = ($absolute ? $actionUriBuilder->uriFor($actionSpecification) : $actionUriBuilder->absoluteUriFor($actionSpecification))
  ->withFragment($section)

or (if we properly use query parameters):

$actionSpecification = ActionUriSpecification::for(Action::create('Neos.Neos', 'Frontend\Node', 'preview'))
  ->withFormat($format ?: $request->getFormat())
$uri = ($absolute ? $actionUriBuilder->uriFor($actionSpecification) : $actionUriBuilder->absoluteUriFor($actionSpecification))
  ->withQuery(Query::build(['node' => $node->getContextPath()])
  ->withFragment($section)

[mficzel]

@bwaidelich To me this looks still too complicated and i would be fine with an ActionUriBuilder that

1. Drops stuff like "addQueryString", "argumentsToBeExcludedFromQueryString"
2. Returns PSR Uri Objects
3. Has a simple interface that has a clear distinction between routingArguments and queryParameters

class ActionUriBuilder {
  public static function fromActionRequest() { ... }
  public static function fromHttpRequest() { ... }
  public static function fromBaseUri() { ... }

  public function uriFor(Action $action, array $routingArguments = null, array $queryParameters = null, string $section = null): UriInterface  { ... }
  public function absoluteUriFor(Action $action, array $routingArguments = null, array $queryParameters = null, string $section = null): UriInterface  { ... }
}

with Action beeing a value object encapsulating action, controller, package and subpackage

[bwaidelich]

A wise man once wrote

I like simple. Especially since I am quite bad at dealing with complexity

:)

I like the suggestion, with one suggestion: Instead of the many arguments what about just

class ActionUriBuilder {
  // ...
  public function uriFor(Action $action, array $routingArguments = []): UriInterface  { ... }
  public function absoluteUriFor(Action $action, array $routingArguments =[]): UriInterface  { ... }
}

IMO readability is quite similar:

$action = new Action(package: 'Neos.Neos', controller: 'Frontend\Node', action: 'preview');

// v1
$uriBuilder->uriFor($action, section: 'some-section'));
$uriBuilder->uriFor($action, queryParameters: ['foo' => 'bar']);

// v2
$uriBuilder->uriFor($action)->withSection('some-secton');
$uriBuilder->uriFor($action)->withQuery(http_build_query(['foo' => 'bar']));

[mficzel]

I think the extra queryParmeters argument helps to understand the distinction between those and routingArguments. We even could document this and explain the differences between them.

Also since routes may still use "appendExceedingArguments" it is not really safe to use withQuery since one would actually have to use:

parse_str($uri->getQuery(), $queryParametersFromRouting);
$mergedQueryParameters = Arrays::arrayMergeRecursiveOverrule($queryParametersFromRouting, $queryParameters);
$uri = $uri->withQuery(http_build_query($mergedQueryParameters, '', '&'));

Compared to that i like the separate queryParmeters much batter.

This leaves the section argument which i have not much opinions about. Would be fine to remove this and use $uri->withFragment() instead or call the parameter simply fragment

[bwaidelich]

@mficzel good point(s)!

[bwaidelich]

What about

final class ActionUriBuilder {
  public static function fromActionRequest(): self;
  public static function fromHttpRequest(): self;
  public static function fromBaseUri(): self;

  public function uriFor(Action $action, array $routingArguments = [], array $queryParameters = []): UriInterface;
  public function absoluteUriFor(Action $action, array $routingArguments = [], array $queryParameters =[]): UriInterface;
}

[mficzel]

@bwaidelich i really like this.

One thing to check while implementing would be wether those fromActionRequest ... methods best belong to the ActionUriBuilder or the Action. Beeing on both ends would be a little weird but also an option.

$builder = ActionUriBuilder::fromActionRequest($request) 

vs:

$action = Action::fromActionRequest(request)

[bwaidelich]

One thing to check while implementing would be wether those fromActionRequest ... methods best belong to the ActionUriBuilder or the Action

Ah yes, right. I kinda like the Action VO to be a simple DTO like:

final class Action {
    private function __construct(
        public readonly string $actionName,
        public readonly string $controllerName,
        public readonly string $packageKey,
    ) {}

    public static function create(string $packageKey, string $controllerName, string $actionName): self
    {
        return new self($actionName, $controllerName, $packageKey);
    }

    public static function fromActionRequest(ActionRequest $request): self
    {
        return new self($request->getControllerActionName(), $request->getControllerName(), $request->getControllerPackageKey());
    }

    // ...
}

Alternatively we could make $controllerName and $packageKey optional and store them in the ActionUriBuilder (if created from ActionRequest) but IMO that increases complexity and chances of runtime exceptions.

BTW: We'll need the factory IMO because the ActionUriBuilder needs access to the router and base uri. So instead of

$builder = ActionUriBuilder::from...(...)

it would be

$builder = $this->actionUriBuilderFactory->createFrom...(...);

(according to the current implementation)

[mficzel]

Maybe ActionUriGenerator and ActionUriGeneratorFactory would be better names in that case. A BuilderFactory seems a little weird to me but it may still be the correct name.

[mhsdesign]

Hi ;) i remember we discussed a lot in november ^^ how is the status and do we want it into 8.3 or is it too breaky and needs to wait for 9.0?

[bwaidelich]

I don't think, it's too breaky since we could introduce it in addition to the existing (marked deprecated) UriBuilderand it would even smooth migration to 9.0. But unfortunately I don't really find the time to work on this one right now

[mhsdesign]

maybe @mficzel and me can finish this pr on the sprint?

[mhsdesign]

The current uri builder does also allow non rewritten uris via:

        $uriPathPrefix = $this->environment->isRewriteEnabled() ? '' : 'index.php/';
        $uriPathPrefix = RequestInformationHelper::getScriptRequestPath($httpRequest) . $uriPathPrefix;
        $uriPathPrefix = ltrim($uriPathPrefix, '/');

should (must?) we support this aswell?

[bwaidelich]

That ENV Variable was deprecated with 8.0 IIRC and I think it's totally fine to drop support for it. But we should adjust the docs (and the htaccess file) and communicate that in the release notes IMO

[mhsdesign]

And do we maybe want some validation while constructing the ActionUriSpecification ? I can imagine checking if the controller exists?

Or we can make a factory by controller class name like:

// option1
ActionUriSpecification::createForController("Foo\Bar\Controller\MyController::indexAction");
// option2
ActionUriSpecification::createForController(Foo\Bar\Controller\MyController::class, "indexAction");
// option3 (phpstorm ide support as this is infact a callable)
ActionUriSpecification::createForController([Foo\Bar\Controller\MyController::class, "indexAction"]);
# option 4
ActionUriSpecification::forController(Controller::class)->withActionName(‘index’)

what do you say? @bwaidelich

[bwaidelich]

@mhsdesign I would definitely avoid some actual validation as that would introduce a dependency to the ObjectManager and it might break cases where the controller doesn't actually exists or a different naming scheme is used. I think, "option 4" is a neat idea – but it also means that we duplicate the naming resolution logic from \Neos\Flow\Mvc\ActionRequest::getControllerObjectName() – I'd be OK with it, but we could also embrace the uncertainty of connecting two systems in this point

[mhsdesign]

my end goal would be to allow a fully qualified class name in the controller option of the routes.yaml (as discussed with @sorenmalling)