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

Prepare Routing for multi-domains / add extension points #1120

Closed
bwaidelich opened this issue Nov 16, 2017 · 2 comments · Fixed by #1126
Closed

Prepare Routing for multi-domains / add extension points #1120

bwaidelich opened this issue Nov 16, 2017 · 2 comments · Fixed by #1126
Assignees

Comments

@bwaidelich
Copy link
Member

bwaidelich commented Nov 16, 2017

Extend the Router as follows:

1. Extend RouterInterface

As a first step we should change the RouterInterface from:

interface RouterInterface
{
    public function route(Request $httpRequest);

    public function resolve(array $routeValues): string
}

to sth like:

interface RouterInterface
{
    public function route(RouteContext $context): RouteResult

    public function resolve(ResolveContext $context): ResolveResult
}
  • RouteContext contains some optional routeValues in addition to the current
  • Http\Request (so that we can pre-set route values before the routing kicks in).
  • RouteResult wraps the resolved routeValues and/or contains a flag whether a route matched at all.
  • ResolveContext contains the routeValues in addition to the current Http\Request and (in v2) some flags like forceAbsoluteUrl, forceScheme
  • ResolveResult contains the resolved Http\Uri and/or a flag whether a route resolved

2. Adjust Router Cache

The cache identifiers must be built from the RouteContext / ResolveContext (i.e. they should probably implement the CacheAwareInterface)

3. Allow hooking into RouteContext

I.e. it should be possible to write a Http\Component that can set routeValues for the current RouteContext (example: extract language from the Accept-Language HTTP header and set a routeValue correspondingly)

4. Allow hooking into ResolveResult

Introduce a signal that allows 3rd party code to hook into the ResolveResult just before it is put into the Cache. It should receive the ResolveContext and the current ResolveResult and might return a new result (or modify the existing one somehow)

To be discussed

  • Do we need a b/c layer for the breaking Router(Interface) signature change?
  • Are there better names for the new classes (see above)?

Background:

This will allow 3rd party packages (and Neos) to hook into the routing process and is a first step for a fully fledged multi-domain routing in Flow as described in #614

Neos: 3.3 LTS
Flow: 4.3 LTS

@bwaidelich bwaidelich self-assigned this Nov 16, 2017
bwaidelich added a commit to bwaidelich/flow-development-collection that referenced this issue Nov 17, 2017
@bwaidelich
Copy link
Member Author

bwaidelich commented Nov 17, 2017

Intermediate status

I managed to get a first version running (without adjusted tests & caching) that allows for custom RoutePart implementations to react to/influence parts of the URI "outside" of their respective route part.

I mostly followed the steps outlined above, with the following deviations:

2. Adjust Router Cache

.. I skipped that step (yet)

3. Allow hooking into RouteContext

Instead of hooking into the context via Http\Component, I created a signal Router::beforeRoute that allows to modify the context like this:

$dispatcher->connect(Router::class, 'beforeRoute', function(RouteContext &$routeContext) {
  // some check based on the current context, e.g. $routeContext->getHttpRequest()->getPort() === 8080
  $routeContext = $routeContext->withParameter('Some.Namespace', new Parameter('parameterName', 'parameterValue'));
});

There is no major advantage over a HTTP component, but I felt it was more in sync with the signal for the ResolveResult

Considerations

The impact of the change is not too bad, but the implementation feels a bit hacky and I think it make sense to abandon the Signal/Slot approach in favor of a more straight-forward implementation.

Recap

We need a way to set "route context parameters" before Router::route() is called because they affect the cache entry identifier (for example if the routing should react to some HTTP header of the request)

But we could maybe skip the whole "Url post-processing" by extending the RoutePart implementation to return some request.
Currently RoutePartInterface::resolves() should return a boolean (note: some implementations, Neos I'm looking at you, return a negative integer instead of false but we can cater for that).
I suggest to extend that so that RouteParts can return some object as well (instead of true) that contains "instructions" to apply on the final URL (for example forceScheme: https).

That way we can get rid of the need to post-process the ResolveResult and Router::resolve() can just return an instance of UriInterface that can be easily cached

@mficzel
Copy link
Member

mficzel commented Nov 29, 2017

I removed this from the 3.3 project-board in favor of pr #1126 that fixes this issue.

bwaidelich pushed a commit that referenced this issue Dec 18, 2017
An overhauled Router implementation that allows for more flexible
routing.

## Features:

### Routing Parameters

Routing `Parameters` can be defined globally (via HTTP component) in order
to allow custom RoutePart handler to react to influences that are outside of
the incoming URI path (example: The requested host name or scheme)

For a RoutePart handler to access the parameters they have to implement
the new `ParameterAwareRoutePartInterface`.
The `DynamicRoutePart` already implements the interface. For custom implementations
extending `DynamicRoutePart` the parameters will be accessible via `$this->parameters`.

### Extended URI matching

RoutePart handlers can now return an instance of `MatchResult` when mapping
incoming requests.
This allows the handler to specify *Tags*  to be associated with the route.

#### Example:

*Before:*

```php
protected function matchValue($value)
{
     // custom logic, returning false if the $value doesn't match
     $this->value = $matchedValue;
     return true;
}
```

*Now:*

```php
protected function matchValue($value)
{
     // custom logic, returning false if the $value doesn't match
     return new MatchResult($matchedValue, RouteTags::createFromTag('some-tag'));
}
```
*Note:* The RouteTags argument is optional
*Note:* For backwards compatibility the above still works as before

### Extended URI resolving

Similar to the above, RoutePart handlers can now specify tags for *resolved* routes
by returning an instance of `ResolveResult`.
This also allows RouteParts to specify `UriConstraints` that will affect the resolved
URI.
UriConstraints allow to pre-set the following attributes of the resulting URI:

* Scheme  (for example "https")
* Host (for example "www.somedomain.tld")
* Host prefix (for example "en.")
* Host suffix (for example "co.uk")
* Port (for example 443)
* Path (for example "some/path")
* Path prefix (for example "en/")
* Path suffix (for example ".html")

#### Example:

*Before:*

```php
protected function resolveValue($value)
{
     // custom logic, returning false if the $value doesn't resolved
     $this->value = $resolvedPathSegment;
     return true;
}
```

*Now:*

```php
protected function resolveValue($value)
{
     // custom logic, returning false if the $value doesn't resolve
     return new ResolveResult($resolvedPathSegment, UriConstraints::create()->withHost('some-domain.tld'), RouteTags::createFromTag('some-tag'));
}
```

*Note:* The UriConstraints and RouteTags arguments are optional
*Note:* For backwards compatibility the above still works as before

## Breaking Change

This isn't a breaking feature in the strict sense because it doesn't affect the public API.
But it introduces some profound changes that potentially requires 3rd party code to be
adjusted:

#### UriBuilder return type

`UriBuilder::build()` (and thus also `UriBuilder::uriFor()`) no longer returns a `string` but instead
an instance of `UriInterface`.
In most places this won't be a problem, because it can be casted to a string but sometimes you
need to manually convert the result: `$uri = (string)$uri;`

#### RouterInterface signature

In case you implemented a custom router:
Congratulations for being brave!
You'll need to slightly adjust the code due to the modified signature of the two methods `route`
and `resolve`:

*Previously:*
```php
/**
 * @return array|null
 **/
public function route(Request $httpRequest);

/**
 * @return string
 **/
public function resolve(array $routeValues);
```

*Now:*
```php
public function route(RouteContext $routeContext): array;

public function resolve(ResolveContext $resolveContext): UriInterface;
```

#### RouterCachingService

Some public methods of the `RouterCachingService` have been changed and you might need
to adjust code that accesses those (directly or via AOP).

Resolves: #1120
bwaidelich added a commit to bwaidelich/flow-development-collection that referenced this issue Dec 18, 2017
This partly reverts neos#1126 by casting the resolved URI so that
`UriBuilder::uriFor()` and `UriBuilder::build()` returns a
`string` again rather than an instance of `UriInterface`.

Background:
Even though `UriInterface` is mostly casted to a string implicitly,
this change was considered "too dangerous" for a minor release
because it breaks code that relies on the previous return type
(i.e. when serializing the resolved URI).

For the next major release we can re-consider this change.

Related: neos#1120
bwaidelich added a commit to bwaidelich/flow-development-collection that referenced this issue Dec 18, 2017
This is a (cosmetic) follow-up to neos#1126 reverting some
doc comment changes that were introduced by accident.

Related: neos#1120
bwaidelich added a commit to bwaidelich/flow-development-collection that referenced this issue Dec 19, 2017
This is a follow up to neos#1126 fixing the automatic tagging of
routes in `RouterCachingService::storeResolvedUriConstraints()` that
no longer included the UUIDs of entities in route values.

Related: neos#1120
ComiR pushed a commit to ComiR/flow-development-collection that referenced this issue Aug 17, 2018
An overhauled Router implementation that allows for more flexible
routing.

## Features:

### Routing Parameters

Routing `Parameters` can be defined globally (via HTTP component) in order
to allow custom RoutePart handler to react to influences that are outside of
the incoming URI path (example: The requested host name or scheme)

For a RoutePart handler to access the parameters they have to implement
the new `ParameterAwareRoutePartInterface`.
The `DynamicRoutePart` already implements the interface. For custom implementations
extending `DynamicRoutePart` the parameters will be accessible via `$this->parameters`.

### Extended URI matching

RoutePart handlers can now return an instance of `MatchResult` when mapping
incoming requests.
This allows the handler to specify *Tags*  to be associated with the route.

#### Example:

*Before:*

```php
protected function matchValue($value)
{
     // custom logic, returning false if the $value doesn't match
     $this->value = $matchedValue;
     return true;
}
```

*Now:*

```php
protected function matchValue($value)
{
     // custom logic, returning false if the $value doesn't match
     return new MatchResult($matchedValue, RouteTags::createFromTag('some-tag'));
}
```
*Note:* The RouteTags argument is optional
*Note:* For backwards compatibility the above still works as before

### Extended URI resolving

Similar to the above, RoutePart handlers can now specify tags for *resolved* routes
by returning an instance of `ResolveResult`.
This also allows RouteParts to specify `UriConstraints` that will affect the resolved
URI.
UriConstraints allow to pre-set the following attributes of the resulting URI:

* Scheme  (for example "https")
* Host (for example "www.somedomain.tld")
* Host prefix (for example "en.")
* Host suffix (for example "co.uk")
* Port (for example 443)
* Path (for example "some/path")
* Path prefix (for example "en/")
* Path suffix (for example ".html")

#### Example:

*Before:*

```php
protected function resolveValue($value)
{
     // custom logic, returning false if the $value doesn't resolved
     $this->value = $resolvedPathSegment;
     return true;
}
```

*Now:*

```php
protected function resolveValue($value)
{
     // custom logic, returning false if the $value doesn't resolve
     return new ResolveResult($resolvedPathSegment, UriConstraints::create()->withHost('some-domain.tld'), RouteTags::createFromTag('some-tag'));
}
```

*Note:* The UriConstraints and RouteTags arguments are optional
*Note:* For backwards compatibility the above still works as before

## Breaking Change

This isn't a breaking feature in the strict sense because it doesn't affect the public API.
But it introduces some profound changes that potentially requires 3rd party code to be
adjusted:

#### UriBuilder return type

`UriBuilder::build()` (and thus also `UriBuilder::uriFor()`) no longer returns a `string` but instead
an instance of `UriInterface`.
In most places this won't be a problem, because it can be casted to a string but sometimes you
need to manually convert the result: `$uri = (string)$uri;`

#### RouterInterface signature

In case you implemented a custom router:
Congratulations for being brave!
You'll need to slightly adjust the code due to the modified signature of the two methods `route`
and `resolve`:

*Previously:*
```php
/**
 * @return array|null
 **/
public function route(Request $httpRequest);

/**
 * @return string
 **/
public function resolve(array $routeValues);
```

*Now:*
```php
public function route(RouteContext $routeContext): array;

public function resolve(ResolveContext $resolveContext): UriInterface;
```

#### RouterCachingService

Some public methods of the `RouterCachingService` have been changed and you might need
to adjust code that accesses those (directly or via AOP).

Resolves: neos#1120
ComiR pushed a commit to ComiR/flow-development-collection that referenced this issue Aug 17, 2018
This partly reverts neos#1126 by casting the resolved URI so that
`UriBuilder::uriFor()` and `UriBuilder::build()` returns a
`string` again rather than an instance of `UriInterface`.

Background:
Even though `UriInterface` is mostly casted to a string implicitly,
this change was considered "too dangerous" for a minor release
because it breaks code that relies on the previous return type
(i.e. when serializing the resolved URI).

For the next major release we can re-consider this change.

Related: neos#1120
ComiR pushed a commit to ComiR/flow-development-collection that referenced this issue Aug 17, 2018
This is a (cosmetic) follow-up to neos#1126 reverting some
doc comment changes that were introduced by accident.

Related: neos#1120
ComiR pushed a commit to ComiR/flow-development-collection that referenced this issue Aug 17, 2018
This is a follow up to neos#1126 fixing the automatic tagging of
routes in `RouterCachingService::storeResolvedUriConstraints()` that
no longer included the UUIDs of entities in route values.

Related: neos#1120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants