zendframework / zend-expressive-hal

Hypertext Application Language implementation for PHP and PSR-7
BSD 3-Clause "New" or "Revised" License
37 stars 22 forks source link

MetadataMap does not honor optional route parameters #5

Closed ghost closed 6 years ago

ghost commented 7 years ago

In our expressive application we use one route definition for a whole endpoint. HAL's metadata map does not recognize optional parameters in this setup and complains about URIs it can't create for paginated list views.

Code to reproduce the issue

// PipelineAndRoutesDelegator.php
$app->route(
    '/book[/:id]',
    [
        BookAction::class
    ],
    [
        'GET', 'POST', 'PUT', 'DELETE', 'PATCH'
    ],
    'book'
);

// ConfigProvider.php
MetadataMap::class => [
    [
        '__class__' => RouteBasedResourceMetadata::class,
        'resource_class' => Book::class,
        'route' => 'book',
        'extractor' => ClassMethods::class,
    ],
    [
        '__class__' => RouteBasedCollectionMetadata::class,
        'collection_class' => BookCollection::class,
        'collection_relation' => 'book',
        'route' => 'book',
    ],
],

Expected results

{
  "_total_items": 245,
  "_page": 1,
  "_page_count": 25,
  "_links": {
    "self": {
      "href": "http://localhost:8080/book?page=1"
    }
  },

Actual results

{
  "code": 0,
  "message": "Missing parameter \"id\"",
  "trace": "#0 /…/vendor/zendframework/zend-router/src/Http/Segment.php(328): Zend\\Router\\Http\\Segment->buildPath(Array, Array, true, true, Array)\n#1
…
  {main}"
}

I temporarily resolved this by adding

'route_params' => [
    'id' => 0,
]

to the defintition of the collection. But then the generated link looks like this. It's functional, but not pretty:


"_links": {
  "self": {
    "href": "http://localhost:8080/book/0?page=1"
  }
},
Rockstar04 commented 6 years ago

I'm also running into this issue, and adding the route param to both config entries did allow it to run without errors, but in my setup the generated pagination lines did not work correctly.

commenting out the exception directly in Zend\Router generates correct links without having to specify a default route param. https://github.com/zendframework/zend-router/blob/999398f705459a3cf546f18b729975fefbc6a6a1/src/Http/Segment.php#L308-L310

Obviously that's not a real solution, but it shows that otherwise this will work as expected.

weierophinney commented 6 years ago

This is actually an issue with zend-router: when assembling a route that contains optional segments, it always renders the optional segments, which means that if any portions of that segment represent values to match, the router expects a matching value in the parameters passed to it.

This may be true of FastRoute and/or Aura.Router as well; I haven't tested.

What this means for you as users is that you have one of two paths to take:

The second is the approach I would suggest. From your original example:

$app->route(
    '/book',
    BookAction::class,
    [ 'GET', 'POST'],
    'books'
);

$app->route(
    '/book/:id',
    BookAction::class,
    ['GET', 'PUT', 'DELETE', 'PATCH'],
    'book'
);

Then update the metadata such that the collection pins to the books route, and the resource pins to the book route.

Ideally, I'd split these into at least two separate middleware as well (one for collections, one for resources), and, better, separate middleware for each method; however, the above changes will make it work properly, and give you proper links.

ghost commented 6 years ago

@weierophinney Thanks for pointing out the workaround. Splitting routes is a feasible way of fixing this. Would it still make sense to open a separate issue for the Zend Router component? Or maybe this should be included into the MetadataMap documentation as a known mis-behaviour.

weierophinney commented 6 years ago

@sleitz You could report it to zend-router, though it's a known limitation, and, IIRC, too complex to address (too many edge cases that can go wrong).

I'd definitely welcome an FAQ entry or similar against the documentation of zend-expressive-hal, however, if you're willing to submit a pull request.