Discuss LinkingService vs the NodeUriBuilder

[mhsdesign]

Should the LinkingService be deprecated or migrated to the NodeUriBuilder?

At the current point, the LinkingService is no longer heavily used in the Neos code-base.
And if it were to be used, it doest function like the old service. Absolute node paths or site relative node paths support was removed from createNodeUri.
Instead of the lInkingService we use the NodeUriBuilder and leverage its uribuilder for additional options.

The linking service should ideally use the NodeUriBuilder inside.

• plugins linking to nodes must not be namespaced but must use the main request: Discuss LinkingService vs the NodeUriBuilder #4552 (comment)
• All legacy path resolving like ~ should be moved to a dedicated Neos\Neos\Utility\LegacyNodePathNormalizer which is used by the linking service.

[kitsunet]

Just document that you should prefer NodeUriBuilder in the future.

[mhsdesign]

If we know that we cant get rid of the old linkingService right now, we can as well make it function mostly like in 8.3 and keep using it in the Neos.Neos:NodeUri and respektive fluid view helpers?

either way we need $someWhere logic to resolve this:

• /<Neos.ContentRepository:Root>/my/site
• some/relative/path
• ~/about/us will be transformed to /<Neos.Neos:Sites>/mysite/about/us

and the linking service should be able to do this as well as he has historically

#4524 (comment)

[mhsdesign]

As discussed here #4564 (comment) we want to introduce a new NodeIdentity dto. This will replace the usecase of the NodeAdress for the NodeUriBuilder.

To also decouple the nodeuribuilder from the uribuilder i thought about this implementation.

<?php

#[Flow\Scope('singleton')]
class NodeUriBuilder
{
    /**
     * Return human readable host relative uris if the cr of the current request matches the one of the specified node.
     * For cross-links to another cr the resulting uri be absolute and contain the host of the other site's domain.
     * 
     * As the human readable uris are only routed for nodes of the live workspace (see DocumentUriProjection)
     * This method requires the node to be passed to be in the live workspace and will throw otherwise.
     */
    public function uriFor(NodeUriSpecification $specification, NodeUriResolveContext $context): UriInterface;

    /**
     * Return human readable absolute uris with host, independent if the node is cross linked or of the current request.
     * For nodes of the current cr the passed base uri will be used as host. For cross-linked nodes the host will be derived by the site's domain.
     * 
     * As the human readable uris are only routed for nodes of the live workspace (see DocumentUriProjection)
     * This method requires the node to be passed to be in the live workspace and will throw otherwise.
     */
    public function absoluteUriFor(NodeUriSpecification $specification, NodeUriResolveContext $context): UriInterface;

    /**
     * Returns a host relative uri with fully qualified node as query parameter encoded.
     */
    public function previewUriFor(NodeUriSpecification $specification): UriInterface;
}

/**
 * This context can be inferred from the current request.
 *
 * For generating node uris in cli context, you can leverage `fromBaseUri` and pass in the desired base uri,
 * Wich will be used for when generating host absolute uris.
 * If the base uri does not contain a host, absolute uris which would contain the host of the current request
 * like from `absoluteUriFor`, will be generated without host.
 *
 * Flows base uri configuration is ignored if not specifically added via `mergeBaseUri`
 */
final readonly class NodeUriResolveContext // todo find better name
{
    public function __construct(
        UriInterface $baseUri,
        RouteParameters $routeParameters
    ) {
    }

    public static function fromActionRequest(ActionRequest $request): self;

    public static function fromBaseUri(UriInterface $baseUri): self;

    public function mergeBaseUri(UriInterface $baseUri): self;
}


final readonly class NodeUriSpecification
{
    public function __construct(
        public NodeIdentity $node,
        public string $format,
        public array $routingArguments,
        public array $queryParameters,
    ) {
    }
}

By using the fully qualified node identity we would also allow cross site link-inking #4441, which is nearly fully implemented but misses a NodeIdentity like value object.

[bwaidelich]

@mhsdesign Thanks for picking up on this one!
Just some really raw first thoughts of mine:
IMO node URL creation is such a common use case that it should be really simple for all the regular use cases. And while I like the clean interface of your NodeUriBuilder, I still stumble upon the whole "Specification" part.
Yes, it is a specification pattern somehow but IMO it's never a good idea to name software things according to their architecture pattern.
Also, "context" is a bit of a burned term (and you already mention this above).

Naming apart, with the introduction of named arguments I really love to use the following pattern for filter and option objects:

final readonly class SomeObject {
    private __construct(
        public NodeIdentity $node,
        public ?array $routingArguments,
        public ?array $queryParameters,
        public ?string $format,
    ) {}

    public static function create(NodeIdentity|string $node): self {
        if (is_string($node)) {
            $node = NodeIdentity::fromString($node);
        }
        return new self($node, null, null, null);
    }

    public function with(
        public ?array $routingArguments = null,
        public ?array $queryParameters = null,
        public ?string $format,
   ): self {
       return new self(
            $this->node,
            $routingArguments ?? $this->routingArguments,
            $queryParameters ?? $this->queryParameters,
            $format ?? $this->format,
       );
   }
}

Just sketched out here, the idea being: Type safety on the consuming side but easy to create.

$spec = new NodeUriSpecification(NodeIdentity::fromString($node), $this->request->format, [], ['q' => 'search term']);

vs

$spec = NodeUriSpecification::create($node)->with(queryParameters: ['q' => 'search term']));

I would even suggest to leave out routingArguments of that with() constructor and create a dedicated method that makes clear that you probably didn't want to use this :)

Last but not least I'm thinking about custom DTOs for the return types.
UriInterface is universal but it has some flaws: there is no explicit string conversion for example, no notion of absolute/relative and no built-in way to set query params.

If we had

final class NodeUriBuilder
{
    public function uriFor(NodeUriSpecification $specification, NodeUriResolveContext $context): RelativeUri;

    public function absoluteUriFor(NodeUriSpecification $specification, NodeUriResolveContext $context): AbsoluteUri;
    // ...
}

we could even omit the queryParameters from the specification and write sth like this instead:

$this->nodeUriBuilder->uriFor(...)->withQueryParameters(['q' => 'search term']);

[mhsdesign]

About our beloved $additionalQueryParameters. When i wrote down my proposal, i knew that i would heat up the discussion once more. I decided to include them in the specification based on our discussions we made one year ago in this closely related topic of introducing a flow ActionUriSpecification.

Originally i was on your side, but after the big extensive discussion on the Dresden Sprint last year where @mficzel made these points (and also mentioned that adding query parameters should be an obvious supported usecase) we agreed to still add it to the ActionUriSpecification.

My compromise was something like returning a thing that implements the psr uri interface, but has additional capabilities like withQueryParameters, but that was at that time not well perceived. I still hold it as a viable option.

The agreement to include the $queryParams in the spec were later again questioned by @kitsunet

I would probably remove ActionUriSpecification::queryParameters as IMHO that is not integral nor necessary for the ActionUri

To which you responded:

If I get you right, this is something we went back and forth with. I also thought that it makes sense to separate query parameters from the routing but @mficzel was in favor of solving it in the builder. And he has a point, setting and merging hierarchical query parameters is a pain with the UriInterface and we would need to add yet another utility class.

Coming back to the discussion and seeing others share the same concerns (that its not part of the spec but some later processing), it think we should look into alternate ways like withQueryParameters, that still make it easy to manipulate and merge recursively the query params. Sorry martin for this betrayal :D

[mhsdesign]

My compromise was something like returning a thing that implements the psr uri interface, but has additional capabilities like withQueryParameters

so i do like the idea of returning a more capable object than just a psr response interface. But i think its not that important whether the uri is host relative or not. That knowledge is already provided by getHost.

I could imagine flow providing this thing (which could even include a isHostRelative method but rather not :P):

class ActionUri implements \Psr\Http\Message\UriInterface
{
    public static function fromUri(UriInterface $uri): self;
    public function withAdditionalQueryParameters(array $queryParameters): self;

    // alias ...
    public function getHost() { $this->uri->getHost(); }
}

[bwaidelich]

+1 but lets not call it ActionUri – at this point it's just a Uri (more specifically a URL, but we already went for the URI nomenclature)

[mhsdesign]

Implemented here neos/flow-development-collection#3316
And as i already thought we already had once a Neos\Flow\Http\Uri ... will need to read up on that :D

Yes as i thought. We had it first, then it implemented a psr interface, then dead.

[bwaidelich]

A few more thoughts after talking to @mhsdesign:
We agree that it makes sense to remove the NodeUriResolveContext parameters and make it part of the NodeUriBuilder state because it is rather internal and won't change over the lifetime of a single request.

So my suggestion:

#[Flow\Scope('singleton')]
final readonly class NodeUriBuilderFactory {
  // ...
  public function create(UriInterface $baseUri): NodeUriBuilder {
    // ..
  }
}

final readonly class NodeUriBuilder
{
    public function uriFor(NodeUriSpecification $specification): Uri;

    public function absoluteUriFor(NodeUriSpecification $specification): Uri;

    public function previewUriFor(NodeUriSpecification $specification): Uri;
}


final readonly class NodeUriSpecification
{
  private function __construct(
    public NodeIdentity $node,
    public ?string $format,
    public array $routingArguments,
  ) {}
    
  public static function create(NodeIdentity $node): self {
    return new self($node, null, []);
  }

  public function withFormat(string $format): self {
    return new self($this->node, $format, $this->routingArguments);
  }

  /**
   * Warning about implications – note about `NodeUriBuilder::uriFor(...)->withAdditionalQueryParameters(...)`
   */
  public function withRoutingArguments(array $routingArguments): self {
    return new self($this->node, $this->format, $routingArguments);
  } 
}

Additionally we could provide some Objects.yaml configuration that allows simple injection of the NodeUriBuilder via the magic BaseUriProvider:

'Some\Namespace\NodeUriBuilder':
  factoryObjectName: Some\Namespace\NodeUriBuilderFactory
  arguments:
    1:
      object:
        factoryObjectName: Neos\Flow\Http\BaseUriProvider
        factoryMethodName: getConfiguredBaseUriOrFallbackToCurrentRequest

[mhsdesign]

thanks so much @bwaidelich for the discussion. We now came up with this pattern:

# objects yaml to make it possible to inject the `NodeUriBuilder` for the happy web path.
# will use the BaseUriProvider which uses the bootstraps active request handler

Neos\Neos\FrontendRouting\NodeUriBuilder:
  factoryObjectName: Neos\Neos\FrontendRouting\NodeUriBuilderFactory
  factoryMethodName: forBaseUri
    arguments:
      1:
        object:
          factoryObjectName: Neos\Flow\Http\BaseUriProvider
          factoryMethodName: getConfiguredBaseUriOrFallbackToCurrentRequest

#[Flow\Scope('singleton')]
class NodeUriBuilderFactory
{
    public function __construct(
        private RouterInterface $router
    ) {
    }

    public function forRequest(ServerRequestInterface $request)
    {
        $baseUri = RequestInformationHelper::generateBaseUri($request);
        $routeParameters = $request->getAttribute(ServerRequestAttributes::ROUTING_PARAMETERS)
            ?? RouteParameters::createEmpty();
        return new NodeUriBuilder($this->router, $baseUri, $routeParameters);
    }

    public function forBaseUri(UriInterface $baseUri)
    {
        $siteDetectionResult = SiteDetectionResult::fromRequest(new ServerRequest(method: 'GET', uri: $baseUri));
        $routeParameters = $siteDetectionResult->storeInRouteParameters(RouteParameters::createEmpty());

        return new NodeUriBuilder($this->router, $baseUri, $routeParameters);
    }
}

class NodeUriBuilder
{
    /**
     * @internal
     */
    public function __construct(
        RouterInterface $router,
        UriInterface $baseUri,
        RouteParameters $routeParameters
    ) {
    }

    /**
     * Return human readable host relative uris if the cr of the current request matches the one of the specified node.
     * For cross-links to another cr the resulting uri be absolute and contain the host of the other site's domain.
     *
     * As the human readable uris are only routed for nodes of the live workspace (see DocumentUriProjection)
     * This method requires the node to be passed to be in the live workspace and will throw otherwise.
     */
    public function uriFor(NodeUriSpecification $specification): \Neos\Flow\Http\Uri;

    /**
     * Return human readable absolute uris with host, independent if the node is cross linked or of the current request.
     * For nodes of the current cr the passed base uri will be used as host. For cross-linked nodes the host will be derived by the site's domain.
     *
     * As the human readable uris are only routed for nodes of the live workspace (see DocumentUriProjection)
     * This method requires the node to be passed to be in the live workspace and will throw otherwise.
     */
    // should throw if baseUri has no host??
    public function absoluteUriFor(NodeUriSpecification $specification): \Neos\Flow\Http\Uri;

    /**
     * Returns a host relative uri with fully qualified node as query parameter encoded.
     */
    public function previewUriFor(NodeUriSpecification $specification): \Neos\Flow\Http\Uri;
}

final readonly class NodeUriSpecification
{
    private function __construct(
        public NodeIdentity $node,
        public string $format,
        public array $routingArguments,
    ) {
    }

    public static function create(NodeIdentity $node): self;

    public function withFormat(): self;

    /** @deprecated if you meant to append query parameters, please use withAdditionalQueryParameters instead */
    public function withRoutingArguments(): self;
}

i

[mhsdesign]

Fixed by #4892 and via #4886 the linking service will be deprecated