Updated: Comment #47

Problem/Motivation

Currently, REST module creates all entity routes at /entity/{type}/{id}. That rarely matches the HTML-based route. They should match, to be properly RESTful. Example: JSON representations of nodes should be available at /node/1, not /entity/node/1.

Proposed resolution

Now that NG entities can define their own link relations per #1970360: Entities should define URI templates and standard links, we can just leverage that. To wit:

1) The URI for an entity should be the "canonical" link the entity defines.
2) We have to create a new link relation to specify the URL path where to create new entities. The current proposal for the name is "http:/drupal.org/link-relations/create", a fully qualified domain name. Example: /node would be the link for POSTing a new node. This is the most reasonable thing we can do for now until we settle on a solution how we deal with link relations in general. This is discussed in #2113345: Define a mechanism for custom link relationships and does not block this issue.
3) We have to split up MimeTypeMatcher into AcceptHeaderMatcher and ContentTypeHeaderMatcher in order to be able to differentiate properly between POST requests to the same path that carry form data vs. JSON or other formats. Example: POST to /node with Content-type: application/hal+json should not end up on some node/view HTML route.
4) We have to initialize the _method restriction on routes to GET|POST so that other routes with different HTTP methods have a chance to be matched. Example: a DELETE request to /node/1 should not end up on the usual node view HTML route.

Remaining tasks

* Commit the RTBC patch.

User interface changes

None.

API changes

Entity URIs for web services change to what they should be anyway. Example: /entity/user/1 changes to /user/1.

#1970360: Entities should define URI templates and standard links
#2113345: Define a mechanism for custom link relationships

Steps to make this manually testable

There is lots of documentation on all the REST stuff here: https://drupal.org/documentation/modules/rest

Here's the quick way to get you started on making REST requests:

  • Enable modules rest and hal
  • Set permission "Access GET on Content resource" for anonymous users.
  • Get a REST Client (there's for example a firefox called RESTClient)
  • Create a node for testing
  • Make /node/$nid GET request with the "Accept: application/hal+json" set

Change Record

[#2096019]

CommentFileSizeAuthor
#122 interdiff.txt955 bytescatch
#119 2019123.patch57.18 KBklausi
#109 2019123.patch56.95 KBklausi
#107 2019123.patch56.58 KBklausi
#105 2019123.patch56.59 KBklausi
#98 2019123.patch56.37 KBklausi
#88 2019123.patch55.62 KBklausi
#45 rest-url-patterns-2019123-45-interdiff.txt3.58 KBklausi
#45 rest-url-patterns-2019123-45.patch48.56 KBklausi
#41 rest-url-patterns-2019123-41.patch49.05 KBklausi
#81 2019123.patch56.5 KBGithub sync
#74 2019123.patch58.14 KBGithub sync
#72 2019123.patch57.13 KBGithub sync
#59 rest-url-patterns-2019123-59-interdiff.txt8.37 KBklausi
#59 rest-url-patterns-2019123-59.patch49.29 KBklausi
#55 profiling.ods21.18 KBklausi
#55 profile.php_.txt1.42 KBklausi
#41 rest-url-patterns-2019123-41-interdiff.txt2.73 KBklausi
#39 rest-url-patterns-2019123-39.patch48.9 KBklausi
#39 rest-url-patterns-2019123-39-interdiff.txt21 KBklausi
#35 rest-url-patterns-2019123-35.patch43.95 KBklausi
#35 rest-url-patterns-2019123-35-interdiff.txt10.51 KBklausi
#34 rest-url-patterns-2019123-34.patch37.69 KBklausi
#34 rest-url-patterns-2019123-34-interdiff.txt9.73 KBklausi
#32 rest-url-patterns-2019123-32.patch35.47 KBklausi
#32 rest-url-patterns-2019123-32-interdiff.txt17.7 KBklausi
#30 rest-url-patterns-2019123-30.patch31.34 KBklausi
#30 rest-url-patterns-2019123-30-interdiff.txt6.81 KBklausi
#26 rest-url-patterns-2019123-26.patch19.51 KBklausi
#26 rest-url-patterns-2019123-26-interdiff.txt6.54 KBklausi
#24 rest-url-patterns-2019123-23.patch15.97 KBklausi
#24 rest-url-patterns-2019123-23-interdiff.txt2.83 KBklausi
#22 rest-url-patterns-2019123-22.patch14.04 KBklausi
#22 rest-url-patterns-2019123-22-interdiff.txt10.93 KBklausi
#12 rest-url-patterns-2019123-12.patch11.41 KBygerasimov
#12 rest-url-patterns-2019123-12-interdiff.txt2.38 KBygerasimov
#8 rest-url-patterns-2019123-8.patch8.82 KBklausi
#8 rest-url-patterns-2019123-8-interdiff.txt5.76 KBklausi
#6 rest-url-patterns-2019123-6.patch6.28 KBklausi
#3 2019123-rest-paths.patch7.65 KBCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Assigned: Unassigned » klausi
Crell’s picture

Priority: Normal » Major
Crell’s picture

Status: Active » Needs review
FileSize
7.65 KB

Here's a first stab. It's nowhere close to done, and tests are failing locally without giving me any useful direction, but let's see what testbot says.

I'm not convinced that create-form is the right link to use here, but I couldn't find anything better.

Status: Needs review » Needs work

The last submitted patch, 2019123-rest-paths.patch, failed testing.

Crell’s picture

Discussed a bit on the REST call today. create-form is not the right link. IANA doesn't have a right link yet.

Crell suggested that "drupal:create" might be technically a valid complete URI per
http://www.ietf.org/rfc/rfc2396.txt

But we should verify that. We want something that is "semantically correct, standards correct, and quick to implement." Whatever that is.

klausi’s picture

Here is an attempt that is nowhere near ready, but shows the approach I want to take:

* "link" can be defined on the REST resource annotation
* the entity derivative copies the link patterns from the entity info to the resource plugin info.
* the generic routes() method checks if such links pattern exist on the plugin and uses that if available, with a fallback to just using the plugin name in the route.
* I'm using "drupal:create" as a link name for now until a better suggestion is made.

Problems:
* This completely breaks node paths for example because our content negotiation sucks (visiting /node/1 in firefox returns REST module's XML response instead of the node page). :-(
* I don't know if this extraction of named request parameters is correct:

    // Determine the request parameters that should be passed to the resource
    // plugin.
    $route_parameters = $request->attributes->get('_route_params');
    $parameters = array();
    // Filter out all internal parameters starting with "_".
    foreach ($route_parameters as $key => $parameter) {
      if (strpos($key, '_') !== 0) {
        $parameters[] = $parameter;
      }
    }
ygerasimov’s picture

Lets have example of resource annotations to have "create" link.

I would propose to have "rest:create" instead of "drupal:create" as a link name as "rest:create" more intuitive (at least for me).

klausi’s picture

Tried to experiment with MimeTypeMatcher, because I thought we can filter out less fitting routes there so that requests to /node/1 work again in web browsers. Turns out that the filter() method only retrieves 3 REST routes, so the usual HTML page route is missing. Maybe the /node/{node} route is still in the old menu system?

Have to give up for today.

ygerasimov’s picture

Looking at node.module node_menu() I see that '/node/%node' is still in old menu system.

linclark’s picture

I would propose to have "rest:create" instead of "drupal:create" as a link name as "rest:create" more intuitive (at least for me).

As noted on the REST call yesterday, using a CURIE style (e.g. drupal:create) is probably not what we want here.

As the CURIE spec says:

CURIEs and SafeCURIEs map to IRIs, but neither a CURIE nor a Safe_CURIE is an IRI or URI.

A CURIE is only interpreted as a URI if the host language supports CURIEs. We don't currently define a namespace mapping for the prefix "drupal" anywhere, so even if it were being output in a host language that does support CURIEs, it would be an invalid CURIE.

Someone on the call yesterday suggested that this is still a valid URI even if we don't consider it a CURIE. Here is the structure of URIs.

<scheme name> : <hierarchical part> [ ? <query> ] [ # <fragment> ]

In this case, we would be defining a whole new scheme, either called "drupal" or "rest". This means we are not using the HTTP scheme. I can't see a reason why we would want to do this.

klausi’s picture

Ok, so working with nodes is blocked by #1987768: Convert node_page_view() to a new style controller. I guess there a lot of other WSCCI conversion issues affecting entity paths that are not yet resolved.

Even entity test still uses hook_menu(). So we have no way to make any progress here unless we have at least one entity type that is NG and uses the new routing system.

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
2.38 KB
11.41 KB

I have played with idea of adding a legacy route in RouteProvider before we get to MimeTypeMatcher and it gets into endless recursion (I have added static flag to prevent it). Now if you call node/1 with browser you get html, and when you call with header Accept: application/xml you will get xml. But I am not a big fan of this solution. Hope we can get it less ugly.

Status: Needs review » Needs work

The last submitted patch, rest-url-patterns-2019123-12.patch, failed testing.

klausi’s picture

Assigned: klausi » Unassigned

@ygerasimov: that hack is not so bad, but it breaks all other pages with endless loops.

I have no idea how the current legacy menu system integration into the routing system works. Where is LegacyUrlMatcher called from? If we have Symfony routes for a path does that mean that the legacy routes can never be called? Where is the distinction between legacy and new routing system done? In DynamicRouter, NestedMatcher, ChainRouter, some event subscriber or in the kernel?

Is the approach of inventing an artificial route object that we generate from the old menu system feasible?

At this point I'm a bit frustrated that I could not answer that questions myself after 2 hours of interactive debugging. Drupal 8 is just a bit too complex for me now.

Unassigning from myself, I'm stuck at developing this further. Feedback welcome!

klausi’s picture

Issue tags: +API addition

So this is probably not going to happen for code freeze, but there might be another way out: we keep the /entity/{entity_type}/{entity} paths and just add the canonical entity paths as well later. So we could deprecate the old paths but still keep them for client compatibility.

Technically this would be an API addition, which is also not supposed to happen after code freeze (I think). Maybe we can get an exception here, since it does not break anything but rather solves a big WTF.

Going with that approach we can avoid any struggle with the old menu system and do this later once most of the HTML entity paths are converted.

linclark’s picture

scor tells me that API additions are still possible after API freeze, since it is no longer a code freeze. I think that they are being careful about what gets in, though.

I think it will be a real shame if we can't get the paths to align between HTML and other serializations. Just yesterday, someone here at DevDays was testing REST and set the path for node/1 to entity/node/1 in the form because they thought that's what they were supposed to do.

linclark’s picture

EDIT: posted twice.

Crell’s picture

Category: task » bug

API change or not, I'd argue that not having consistent paths is a straight up bug. We need to fix that bug before launch, ASAP.

Klausi: The routing system entry point is ChainRouter. That calls the legacy system and then DynamicRouter in turn. LegacyMatcher is part of the old system. If LegacyMatcher doesn't find a route then DynamicRouter is called. DynamicRouter contains the NestedMatcher, which is what looks up routes from the DB, filters them, etc.

It's probably not feasible to create route objects from legacy routes, especially since we're planning to remove legacy route support entirely. However, if a route comes from the legacy matcher then there will be a _legacy attribute set on the route, set to true. We if-else on that in a couple of places for now; that's the easiest way to deal with that problem for the moment.

webchick’s picture

The only BC break here seems to be just that URIs are changing; I don't think this needs approval, go for it. But if there are other module developer-facing changes required to get this done, we need that info in the issue summary.

Crell’s picture

Issue tags: +Approved API change

I will take that as approval. :-)

This is still blocked by #1987778: Convert node_show() and node_page_view() to a new style controller at the moment.

Crell’s picture

That issue is no longer blocking, thanks to #2089635: Convert non-test non-form page callbacks to routes and controllers. So we can move forward here.

klausi’s picture

Status: Needs work » Needs review
FileSize
10.93 KB
14.04 KB

The good news: this now basically works for me for nodes during manual testing.

The bad news:
* lot's of REST tests need to be fixed.
* we return now 200 with HTML for invalid formats on nodes instead of 406, is that ok with us?
* i'm not sure about the algorithm in the MIME type matcher and whether we should do some optimization for HTML routes and routes that don't specify any format requirement.
* Will still haven't settled on the naming of the link relation for creating entities with POST, still using drupal:create in the patch.

Status: Needs review » Needs work

The last submitted patch, rest-url-patterns-2019123-22.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
2.83 KB
15.97 KB

The problem is that nodes and entity_test behave differerntly on non existing entities. A non existing node path returns an Exception from upcasting which results in an obscure plain text response body. On the other hand entity_test is NOT upcasted and the error is handled by the resource plugin.

Not much progress.

Status: Needs review » Needs work

The last submitted patch, rest-url-patterns-2019123-23.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI
FileSize
6.54 KB
19.51 KB

Not much luck with this.

* Drupal AJAX is weird. I tried to mark ajax route as "_format: drupal_ajax" so that they match with the Accept headers of AJAX requests, but that did not work because then the request object suddenly does not know about the drupal_ajax format anymore. Anyway, I added a similar workaround as in the content negotiation class to just list those formats as default.

* PATCH requests for nodes do not work. The problem is that the two routes node.view and rest.entity.node.PATCH match the request. The MIME type matcher only acts on Accept headers (a PATCH request carries the format in the Content-type header), so both routes survive the matching. Then the first route is picked randomly, which is the node.view route and the usual node GET controller is executed.

I see two possibilities to solve this:
* Make MimeTypeMatcher also look for the Content-Type header on write requests or create a new matcher that targets this. Not sure if we should also use the _format requirement on routes or a second _content_type_format?
* Make UrlMatcher better at filtering out routes per HTTP method. We have explicitly specified PATCH as required method on the REST route, should the UrlMatcher throw out other routes that do not specify that exact HTTP method? Actually it should not throw out the route but rather adjust the fitness of the routes.

Status: Needs review » Needs work

The last submitted patch, rest-url-patterns-2019123-26.patch, failed testing.

andypost’s picture

klausi’s picture

The route selection problem even more applies to POST requests. Let's assume I have a standard HTML route with a form on it on /node:

mymodule.node:
  path: '/node'
  defaults:
    _content: '\Drupal\mymodule\Controller\NodeController::form'

Then this will collide with the REST route:

rest.node.POST
  path: '/node'
  defaults:
    _controller => 'Drupal\rest\RequestHandler::handle',
  requirements:
    _method: POST

So the routing system will never be able to select the second REST route, because it does not look at the Content-type HTTP header. The only way to differentiate between two POST requests is to examine the incoming data: one request uses application/x-www-form-urlencoded to submit a HTML form and should be mapped to the first route, the other uses application/hal+json and should be mapped to the second route.

Looks like we need a HTTP Content-type MIME type matcher and a new requirements property "_content_type_format" for routes. Should we mix that into MimeTypeMatcher, so that it matches based on Accept and Content-Type headers? Or should we create a new ContentTypeMatcher class?

I'm going to experiment with an advanced MimeTypeMatcher class.

klausi’s picture

Status: Needs work » Needs review
FileSize
6.81 KB
31.34 KB

What we decided on the last REST call:

* DELETE routing collision with existing node paths that return HTML. Conclusion: default all default HTML routes to GET|POST in their _method requirement, use a route alter event for that.
* Same for POST requests. Conclusion: add a HTTP content type matcher and rename MimeTypeMatcher.

So this patch now fixes the problem with DELETE requests on the /node/1 path never matching the REST routes by adding a RouteMethodSubscriber.

The interdiff is not fully complete and this will still fail on the testbot. Coming up next: Creating ContentTypeMatcher that will act on non GET requests.

Status: Needs review » Needs work

The last submitted patch, rest-url-patterns-2019123-30.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
17.7 KB
35.47 KB

Updated patch, including the split-up of MimeTypeMatcher to AcceptHeaderMatcher and ContentTypeMatcher.

This should get us pretty far I hope, let's see what test fails remain.

TODO:
* expand unit test for AcceptHeaderMatcher
* create unit test for ContentTypeMatcher
* create REST test for GET JSON /node/1

Status: Needs review » Needs work

The last submitted patch, rest-url-patterns-2019123-32.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
9.73 KB
37.69 KB

So we simply cannot remove routes in the matcher if they don't have any restrictions, because then all the things break. New approach: prioritize the returned route collection with better fitting routes on top of the list and generic routes moved down. This has the disadvantage that we rely on internal implementation details of the RouteCollection class (it never says anything about order in the collection), but it should cover our use case.

klausi’s picture

Yay, so the tests pass now!

This is now ready for review. I added phpunit test cases for both new matcher classes.

We still have a drupal:create link relation in there, which is not a standards-compliant name. Other suggestions?

klausi’s picture

Issue summary: View changes

Adding docs to test this manually to the summary.

klausi’s picture

Updated the issue summary.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteMethodSubscriber.php
    @@ -0,0 +1,48 @@
    +/**
    + * Provides a default value for the HTTP method restriction on routes.
    + *
    + * Most routes will only deal with GET and POST requests, so we restrict them to
    + * those two if nothing else is specified. This is necessary to give other
    + * routes a chance during the route matching process when they are listening
    + * for example to DELETE requests on the same path. A typical use case are REST
    + * web service routes that use the full spectrum of HTTP methods.
    + */
    +class RouteMethodSubscriber implements EventSubscriberInterface {
    +
    

    To be honest it is a bit odd to add default values, as this increases the site of every route, even we actually could just apply the default logic on the RouteFilter level.

  2. +++ b/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php
    @@ -186,7 +186,10 @@ public function enhance(array $defaults, Request $request) {
    +        throw new NotFoundHttpException(format_string('Item "@name" with ID @id not found', array(
    

    We could just use String::format

  3. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php
    @@ -124,19 +129,21 @@ public function post($id, EntityInterface $entity = NULL) {
    +    if (is_scalar($original_entity)) {
    +      $original_entity = entity_load($definition['entity_type'], $original_entity);
    +    }
    
    @@ -176,17 +183,20 @@ public function patch($id, EntityInterface $entity = NULL) {
    +    if (is_scalar($entity)) {
    +      $definition = $this->getPluginDefinition();
    +      $entity = entity_load($definition['entity_type'], $id);
    +    }
    

    Just wondering how it is possible that the entity is not upcasted yet. Can't we fix that on the param converter level? The amount of workarounds seems too much.

  4. +++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php
    @@ -69,13 +68,24 @@ public function handle(Request $request, $id = NULL) {
    +    $route_parameters = $request->attributes->get('_route_params');
    

    I try to understand the different of _route_params and _raw_variables which should have the same information.

  5. +++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php
    @@ -69,13 +68,24 @@ public function handle(Request $request, $id = NULL) {
    +      if (strpos($key, '_') !== 0) {
    

    Can we just go with $key[0] !== '_' this describes it a bit easier what is going on.

  6. +++ b/core/modules/rest/lib/Drupal/rest/Tests/DeleteTest.php
    @@ -59,17 +59,22 @@ public function testDelete() {
    +        $decoded = drupal_json_decode($response);
    

    We could use the new object instead but this is out of scope;

  7. +++ b/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php
    @@ -2,21 +2,22 @@
    +class AcceptHeaderMatcherTest extends UnitTestCase {
    
    @@ -44,39 +45,51 @@ function __construct($test_id = NULL) {
        */
       public function testNoRouteFound() {
    

    We could use @expectedException with this change.

  8. +++ b/core/tests/Drupal/Tests/Core/Routing/ContentTypeHeaderMatcherTest.php
    @@ -0,0 +1,96 @@
    +  public function testFilterRoutes() {
    +
    +    $matcher = new ContentTypeHeaderMatcher();
    +    $collection = $this->fixtures->sampleRouteCollection();
    +    $collection->addCollection($this->fixtures->contentRouteCollection());
    +
    +    // Tests that routes are not filtered on GET requests.
    +    $request = Request::create('path/two', 'GET');
    +    $routes = $matcher->filter($collection, $request);
    +    $this->assertEquals(count($routes), 7, 'The correct number of routes was found.');
    +
    +    // Test that XML-restricted routes get filtered out on JSON requests.
    +    $request = Request::create('path/two', 'POST');
    +    $request->headers->set('Content-type', 'application/json');
    +    $routes = $matcher->filter($collection, $request);
    +    $this->assertEquals(count($routes), 6, 'The correct number of routes was found.');
    +    $this->assertNotNull($routes->get('route_f'), 'The json route was found.');
    +    $this->assertNull($routes->get('route_g'), 'The xml route was not found.');
    +    foreach ($routes as $name => $route) {
    +      $this->assertEquals($name, 'route_f', 'The json route is the first one in the collection.');
    +      break;
    +    }
    +
    +    // Test all XML and JSON restricted routes get filtered out on a POST form
    +    // submission.
    +    $request = Request::create('path/two', 'POST');
    +    $request->headers->set('Content-type', 'application/www-form-urlencoded');
    +    $routes = $matcher->filter($collection, $request);
    +    $this->assertEquals(count($routes), 5, 'The correct number of routes was found.');
    +    $this->assertNull($routes->get('route_f'), 'The json route was found.');
    +    $this->assertNull($routes->get('route_g'), 'The xml route was not found.');
    

    If we create such separate tests just use multiple descriptive test methods.

  9. +++ b/core/tests/Drupal/Tests/Core/Routing/ContentTypeHeaderMatcherTest.php
    @@ -0,0 +1,96 @@
    +  /**
    +   * Confirms that the matcher throws an exception for no-route.
    +   */
    +  public function testNoRouteFound() {
    

    Just use @expectedException as annotation. I don't care whether the other example does it different.

Crell’s picture

To be honest it is a bit odd to add default values, as this increases the site of every route, even we actually could just apply the default logic on the RouteFilter level.

That introduces more overhead at runtime along the critical path. This is a compile-time operation. I am really not concerned about the size of the serialized route object; certainly not for adding, what, 8 bytes?

klausi’s picture

Excellent review dawehner, thanks!

Now that I think about it we can just use the entity upcaster/enhancer for all our routes, then we don't have to deal with two cases which simplifies the code a lot.

I agree with Crell that the compile time approach looks better to me.

I tried to address the rest of your comments in this patch. No idea about _route_params vs. _raw_variables, so I left that unchanged.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Routing/ContentTypeHeaderMatcherTest.php
@@ -77,20 +96,18 @@ public function testFilterRoutes() {
+   * @expectedExceptionMessage No route found that matches the Content-Type header.

Nice! I did not know what you can also check the actual message.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
@@ -78,13 +78,17 @@ public function permissions() {
+    $definition = $this->getPluginDefinition();
+    $canonical_path = isset($definition['links']['canonical']) ? $definition['links']['canonical'] : '/' . strtr($this->pluginId, ':', '/') . '/{id}';
+    $create_path = isset($definition['links']['drupal:create']) ? $definition['links']['drupal:create'] : '/' . strtr($this->pluginId, ':', '/');

I am wondering whether the concept of canonical links etc. really belongs into the ResourceBase or rather is coupled to entities.

klausi’s picture

Yes, it belongs into resource base because we not only want to support link definitions on the entity derivative plugin but also on other resource plugins. So people could use a links annotation on their resource plugin class (which basically looks the same as on an entity class), which is cool.

This updated patch now replaces the drupal:create link relation name with the fully qualified name http://drupal.org/link-relations/create . A page on drupal.org does not exist yet, but that is not a requirement. It could be used later for documentation purposes (explaining the meaning of this link relation in Drupal land).

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php
    @@ -0,0 +1,54 @@
    +    // We do not throw a
    +    // \Symfony\Component\Routing\Exception\ResourceNotFoundException here
    +    // because we don't want to return a 404 status code, but rather a 400.
    +    throw new BadRequestHttpException('No route found that matches the Content-Type header.');
    

    Isn't there a more specific 4xx error in this case? This looks like a 415 to me:

    http://httpstatus.es/415

    (I think there is a Symfony exception for that, which we were previously using in place of 406, because those two are confusingly similar.)

  2. +++ b/core/modules/hal/lib/Drupal/hal/HalSubscriber.php
    @@ -34,7 +34,8 @@ public function onKernelRequest(GetResponseEvent $event) {
    +    // The format must be available before routing, so we make the priority big.
    +    $events[KernelEvents::REQUEST][] = array('onKernelRequest', 4000);
    

    Routing is priority 32. We don't need to have priorities of a bajillion. Anything larger than 32 is fine. (Why is it 32? I have no idea. I borrowed that from fullstack.)

  3. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
    @@ -98,9 +102,14 @@ public function routes() {
    +          // Do not break here, fall through to PATCH additions which also apply
    +          // to POST.
    +
    

    This is a micro-op. It's also a sloppy one. It's 2 lines of code. Duplicate it. :-)

linclark’s picture

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/Derivative/EntityDerivative.phpundefined
@@ -74,6 +74,18 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
+        $this->derivatives[$entity_type]['links']['http://drupal.org/link-relations/create'] = isset($entity_info['links']['http://drupal.org/link-relations/create']) ? $entity_info['links']['http://drupal.org/link-relations/create'] : "/entity/$entity_type";

This might change based on #2113345: Define a mechanism for custom link relationships. If we don't use link relations for the keys, then we would likely change this key to something a little more readable.

star-szr’s picture

Title: Use definied entity URIs for routes » Use defined entity URIs for routes
klausi’s picture

How could I miss 415? Yep, that is exactly what we need. Also fixed the other stuff.

Ignoring the link relation discussion, we can change that later if we want.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Change interdiff looks good. The patch adds good testcoverage etc.

dawehner’s picture

Issue summary: View changes

Expanded on proposed solution.

klausi’s picture

Issue summary: View changes

clarified link relation usage, remove part about rendering links in entities

klausi’s picture

Updated the issue summary.

pwolanin’s picture

Does this leave in place the 2x definition of the path in the route and the annotation? we need to come up with a plan to fix that if it's not fixed here.

klausi’s picture

This is not fixed here, that would be a separate issue.

catch’s picture

Sorry it took me a while to get to this. Could we do some profiling of the route negotiation on nodes? This looks like it could impact that and it'd be good to know if it does.

Crell’s picture

What sort of profiling do you see needed?

catch’s picture

The inbound processing for node/n. For example we'll be doing that if a node is in a breadcrumb - so say there were six breadcrumbs all pointing to nodes (via aliases), does this add any overhead or not?

catch’s picture

Issue summary: View changes

fixed numbering

catch’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

CNR for the profiling.

klausi’s picture

So I tried to do some profiling with 8 nodes and 8 breadcrumbs looking at the PathBasedBreadcrumbBuilder.

I had a look at it with xhprof at the most outer node page that has 8 breadcrumbs on it.

Drupal 8.x:

main():	1 call, 539ms = 100.0% incl. wall time
Drupal\system\PathBasedBreadcrumbBuilder::build(): 1 call, 78ms = 14.4% incl. wall time
Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher::matchRequest(): 28 calls, 67ms = 12.5% incl. wall time

This patch:

main():	1 call, 550ms = 100.0% 	
Drupal\system\PathBasedBreadcrumbBuilder::build(): 1 call, 75ms =	13.6%
Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher::matchRequest(): 28 calls, 71ms = 12.9%

So the whole page request was a bit slower with this patch, which was expected. 4 milli seconds more have been spent in the nested routing matcher where the new matchers from this patch are invoked. If we only look at this part the performance impact is about 6% (from 67ms to 71ms). If we look at the whole page request then it is roughly 2% (from 539ms to 550ms).

It might also be interesting to look at the case when more routes are available at the node paths with REST module:

Drupal 8.x with basic_auth, HAL, REST and serialization enabled:

main(): 1 call, 564ms = 100.0%
Drupal\system\PathBasedBreadcrumbBuilder::build(): 1 call, 75ms =	13.3%
Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher::matchRequest(): 28 calls, 69ms = 12.3%

This patch with basic_auth, HAL, REST and serialization enabled:

main():	1 call, 565ms =	100.0%
Drupal\system\PathBasedBreadcrumbBuilder::build(): 1 call, 87ms =	15.4%
Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher::matchRequest(): 28 calls, 91ms =	16.1%

So this has a bigger impact, because we have one route at /node/{node} in D8 right now and six routes with this patch. Interestingly the overall page request time only increases 1ms, but that number is volatile anyway as the whole page request time varies +-50ms on my computer. The time spent in the nested matcher increases 31% from 69ms to 91ms.

So these numbers look like they cannot be trusted because they are volatile. One xhprof run just does not cut it.

I wrote a script to generate more numbers to get to a useful median value. It invokes the system.breadcrumb.default service 100 times and measures the first run (more expensive building all the objects) and the subsequent runs average. I ran that script more than 20 times per test case and collected the numbers in a spreadsheet.

Drupal 8.x: first run: 157ms, average: 32ms
This patch: first run: 171ms (+9%), average: 34ms (+6%)
Drupal 8.x + REST: first run: 172ms, average: 34ms
This patch + REST: first run: 169ms (-2%), average: 37ms (+9%)

So we have some notable performance impacts here when we benchmark directly at the breadcrumb build service. If you look at the raw data you can see that the data points vary a lot, so there is still some fuzzy margin of error around this test. Please also note that we are testing an edge case here with many breadcrumbs that needs to run request matching many times.

I think we can conclude that we are making Drupal a bit slower with this patch, ranging from 0 to 14ms on my computer. That translates into 0-2.5% more in overall page request time, depending how often the whole request matching pipeline is executed.

Given that we adopted the new routing system because of its design and flexibility we should also make proper use of it, which we try to accomplish in this patch. We buy that with a small performance penalty, but my testing did not show any serious slow down.

More testing or do we want to go back to RTBC?

klausi’s picture

FileSize
1.42 KB
21.18 KB

Added profiling data and script.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Catch, is that acceptable? If not, we'd need suggestions on how else to handle it because HEAD is currently incorrect in this regard by not using /node/{node} for REST module.

catch’s picture

Status: Reviewed & tested by the community » Needs work

0-2.5% of total request time is only such a small percentage because overall request time is so bad at the moment.

If it's around 14ms then that's closer to 10% of Drupal 7 request time for at least some pages, which is a significant regression.

Would be good to see a bit more from inside matchRequest() to see what's taking the extra time. An xhprof diff report would probably be most useful. If we can optimize the lookup that'd be the best thing - this is really the first time it's been applied to anything that's used a lot.

Crell’s picture

Issue tags: +beta blocker

This is only an API change for REST clients, of which there are none at present. However, I do consider fixing this to be a necessary, release-blocking API change and thus beta blocking.

klausi’s picture

Status: Needs work » Needs review
FileSize
49.29 KB
8.37 KB

Rerolled patch.
Added 2 performance optimizations:
1) Don't create new RouteCollection objects in AcceptHeaderMatcher and rather manipulate the incoming object.
2) Restrict the fake request object in PathBasedBreadcrumbBuilder to only one accepted format (text/html) to reduce matching costs in AcceptHeaderMatcher.

I also did some XHprof profiling for Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher::matchRequest(), will follow-up with that later.

The last submitted patch, 59: rest-url-patterns-2019123-59.patch, failed testing.

klausi’s picture

Edit: Moved profiling table to http://klau.si/sites/default/files/profiling.html

So this is pretty bad. Drupal has to match 18 requests on the frontpage (logged in as admin), WTF? Of course that cumulates to a loss of 4ms overall (up from 39ms to 43ms on the invocation of NestedMatcher::matchRequest()).

Honestly I don't see any hope for this patch to keep up with performance while we have to match so many fake sub-requests.

So should we close this as won't fix or has anyone ideas how to reduce the number of matched fake requests?

andypost’s picture

effulgentsia’s picture

has anyone ideas how to reduce the number of matched fake requests?

I do, but too tired to write it up right now. Will do so tomorrow.

effulgentsia’s picture

Well, turns out the direction I was thinking is made obsolete by #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit, which fixes it a different way. With that patch, the number of calls to matchRequest() for an admin viewing the front page goes down to 3. That's still 2 too many, but that's a separate issue (#2150833: Performance regression in menu access checks). I suspect if you benchmark the impact of this patch after #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit lands, it'll turn out to be acceptable, and will get even better as the remaining occurrences of #2150833: Performance regression in menu access checks are fixed.

Given that it's insane for us to call matchRequest() for anything other than the one actual HTTP request being handled (not counting non-performance-critical steps, like saving a menu link or rebuilding menu caches) my recommendation is to allow this in, and fix those needless invocations in those other issues. But, that's a call for catch to make, not me.

From catch in #52:

The inbound processing for node/n. For example we'll be doing that if a node is in a breadcrumb - so say there were six breadcrumbs all pointing to nodes (via aliases), does this add any overhead or not?

#2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit decouples breadcrumbs / menu parents from paths (or if it doesn't fully, then there should be a follow up to finish that), so no longer an issue, right?

andypost’s picture

The inbound processing for node/n

the same applies for taxonomy terms and other entities that could have limited access on per user basis

klausi’s picture

Status: Needs review » Postponed
Related issues: +#2150747: Switch back to URI templates in entity annotations

Looks like we can continue here, since the performance regression will be dealt with in other issues.

This approach will not work until we have URI templates back in entity link annotations, postponing this on #2150747: Switch back to URI templates in entity annotations.

xjm’s picture

Priority: Major » Critical

This is only an API change for REST clients, of which there are none at present. However, I do consider fixing this to be a necessary, release-blocking API change and thus beta blocking.

Release blockers are by definition critical, so bumping accordingly.

@catch pointed that if this is only a change to the REST module API, it's not a beta blocker. Can we either move this issue to the entity or routing system as appropriate, or remove the beta blocker tag?

Crell’s picture

To clarify, it's an API change for "people talking to a Drupal 8 site over non-HTML", which just so happens to be provided by rest.module. It's not an API change for developers interacting with rest.module. (It may be, but that's less of an issue, I agree.) Changing the REST API of Drupal is rather the definition of "API break". :-)

catch’s picture

Yes but changing that doesn't block a beta IMO - people creating a REST client for 8.x are not the target of beta, module developers are.

Crell’s picture

Issue tags: -beta blocker +beta target

I find the idea that those are necessarily different people odd, but I'll compromise here. (Critical = 8.0.0 blocking, but not beta blocking. Changing the path after 8.0 would definitely be a problem.)

catch’s picture

It's a quantitative vs. qualitative difference.

Github sync’s picture

Issue summary: View changes
Status: Postponed » Needs review
FileSize
57.13 KB

klausi opened a new pull request for this issue.

klausi’s picture

Un-postponing this: maybe we can make this work with route names in entity annotations.

So what I'm doing now is to extract the path patterns from the routes when we build the entity REST plugins in EntityDerivative.php.

This is now rather inconsistent, because in entity annotations we have route names for links and in REST plugin annotations we have path patterns for links, but I want to see how many test cases fail first.

Github sync’s picture

FileSize
58.14 KB

klausi pushed some commits to the pull request.

klausi’s picture

So I renamed "links" to "uri_paths" in the REST plugin definition to better distinguish it from "links" in entity annotations. That should make it more clear that we are dealing with URI patterns here nd not with route names as in the entity links annotation.

Status: Needs review » Needs work

The last submitted patch, 74: 2019123.patch, failed testing.

Xano’s picture

Can you please provide interdiffs?

klausi’s picture

klausi’s picture

Status: Needs work » Needs review

74: 2019123.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 74: 2019123.patch, failed testing.

Github sync’s picture

Status: Needs work » Needs review
FileSize
56.5 KB

klausi pushed some commits to the pull request.

Status: Needs review » Needs work

The last submitted patch, 81: 2019123.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review

81: 2019123.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 81: 2019123.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review

81: 2019123.patch queued for re-testing.

klausi’s picture

Those test fails look very random; the first was an out of memory error, now something unrelated UserCancelTest. Retesting one more time.

dawehner’s picture

Great work! The issue title is a bit of vague and confused me.

  1. +++ b/core/core.services.yml
    @@ -412,6 +417,10 @@ services:
    diff --git a/core/lib/Drupal/Core/Controller/ExceptionController.php b/core/lib/Drupal/Core/Controller/ExceptionController.php
    
    diff --git a/core/lib/Drupal/Core/Controller/ExceptionController.php b/core/lib/Drupal/Core/Controller/ExceptionController.php
    index df5eb07..915d819 100644
    
    index df5eb07..915d819 100644
    --- a/core/lib/Drupal/Core/Controller/ExceptionController.php
    
    --- a/core/lib/Drupal/Core/Controller/ExceptionController.php
    +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
    
    +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
    +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
    @@ -105,7 +105,7 @@ public function execute(FlattenException $exception, Request $request) {
    
    @@ -105,7 +105,7 @@ public function execute(FlattenException $exception, Request $request) {
           return $this->$method($exception, $request);
         }
     
    -    return new Response('A fatal error occurred: ' . $exception->getMessage(), $exception->getStatusCode(), $exception->getHeaders());
    +    return new Response('An error occurred: ' . $exception->getMessage(), $exception->getStatusCode(), $exception->getHeaders());
       }
     
    

    Out of scope of this issue.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteMethodSubscriber.php
    @@ -0,0 +1,48 @@
    +    $events[RoutingEvents::ALTER][] = 'onRouteBuilding';
    

    For now we maybe should set a higher priority.

  3. +++ b/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php
    @@ -0,0 +1,53 @@
    +    // The Content-type header does not make sense on GET requests, so nothing
    

    It would be great to also explain why it does not make sense

  4. +++ b/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php
    @@ -0,0 +1,53 @@
    +    if ($request->getMethod() == 'GET') {
    

    Note: We can also use $request->isMethod('GET') instead.

  5. +++ b/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php
    @@ -0,0 +1,53 @@
    +    // We do not throw a
    +    // \Symfony\Component\Routing\Exception\ResourceNotFoundException here
    +    // because we don't want to return a 404 status code, but rather a 415.
    +    throw new UnsupportedMediaTypeHttpException('No route found that matches the Content-Type header.');
    

    It feels a bit odd to throw an exception here, but i cannot think of a better place.

  6. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php
    @@ -31,36 +34,29 @@ class EntityResource extends ResourceBase {
    +    if (!$entity->access('view')) {
    +      throw new AccessDeniedHttpException();
    +    }
    

    As general new issue we could think about having a separate access method. Not a problem at all for this issue.

  7. +++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php
    @@ -69,13 +68,24 @@ public function handle(Request $request, $id = NULL) {
    +    // Determine the request parameters that should be passed to the resource
    +    // plugin.
    +    $route_parameters = $request->attributes->get('_route_params');
    +    $parameters = array();
    +    // Filter out all internal parameters starting with "_".
    +    foreach ($route_parameters as $key => $parameter) {
    +      if ($key{0} !== '_') {
    +        $parameters[] = $parameter;
    +      }
    +    }
    +
    ...
    +      $response = call_user_func_array(array($resource, $method), array_merge($parameters, array($unserialized, $request)));
    

    Could we use the controller resolver instead to figure out the needed parameters?

  8. +++ b/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php
    @@ -0,0 +1,125 @@
    +/**
    + * Basic tests for the AcceptHeaderMatcher class.
    + */
    +class AcceptHeaderMatcherTest extends UnitTestCase {
    

    One thing we do is using @coversClassDefault \Drupal\... to help phpunit code coverage.

  9. +++ b/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php
    @@ -0,0 +1,125 @@
    +  public static function getInfo() {
    ...
    +  public function setUp() {
    

    Let's interhitdoc

  10. +++ b/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php
    @@ -0,0 +1,125 @@
    +  /**
    +   * Check that JSON routes get filtered and prioritized correctly.
    +   */
    +  public function testJsonFilterRoutes() {
    +    $collection = $this->fixtures->sampleRouteCollection();
    +
    +    // Tests basic JSON request.
    +    $request = Request::create('path/two', 'GET');
    +    $request->headers->set('Accept', 'application/json, text/xml;q=0.9');
    +    $routes = $this->matcher->filter($collection, $request);
    +    $this->assertEquals(count($routes), 4, 'The correct number of routes was found.');
    +    $this->assertNotNull($routes->get('route_c'), 'The json route was found.');
    +    $this->assertNull($routes->get('route_e'), 'The html route was not found.');
    +    foreach ($routes as $name => $route) {
    +      $this->assertEquals($name, 'route_c', 'The json route is the first one in the collection.');
    +      break;
    +    }
    +  }
    +
    +  /**
    +   * Tests a JSON request with alternative JSON MIME type Accept header.
    +   */
    +  public function testAlternativeJson() {
    +    $collection = $this->fixtures->sampleRouteCollection();
    +
    +    $request = Request::create('path/two', 'GET');
    +    $request->headers->set('Accept', 'application/x-json, text/xml;q=0.9');
    +    $routes = $this->matcher->filter($collection, $request);
    +    $this->assertEquals(count($routes), 4, 'The correct number of routes was found.');
    +    $this->assertNotNull($routes->get('route_c'), 'The json route was found.');
    +    $this->assertNull($routes->get('route_e'), 'The html route was not found.');
    +    foreach ($routes as $name => $route) {
    +      $this->assertEquals($name, 'route_c', 'The json route is the first one in the collection.');
    +      break;
    +    }
    +  }
    +
    +  /**
    +   * Tests a standard HTML request.
    +   */
    +  public function teststandardHtml() {
    +    $collection = $this->fixtures->sampleRouteCollection();
    +
    +    $request = Request::create('path/two', 'GET');
    +    $request->headers->set('Accept', 'text/html, text/xml;q=0.9');
    +    $routes = $this->matcher->filter($collection, $request);
    +    $this->assertEquals(count($routes), 4, 'The correct number of routes was found.');
    +    $this->assertNull($routes->get('route_c'), 'The json route was not found.');
    +    $this->assertNotNull($routes->get('route_e'), 'The html route was found.');
    +    foreach ($routes as $name => $route) {
    +      $this->assertEquals($name, 'route_e', 'The html route is the first one in the collection.');
    +      break;
    +    }
    +  }
    

    These 3 ones have really similar structure so we could convert them to use a @dataProvider

klausi’s picture

FileSize
55.62 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Title: Use defined entity URIs for routes » Use the same canonical URI paths as for HTML routes

@dawehner: RouteFilterInterface specifies that an exception should be thrown when all routes are filtered out by a matcher, that's why we throw an exception.

I don't think we can use ControllerResolver, because the needed parameters are dynamic. The resource plugins will handle the parameters, so maybe we could get parameter information from them with reflection, but I would like to keep it simple for this patch for now.

Addressed the other comments, thanks!

dawehner’s picture

Sadly we need to create a change notice now.

klausi’s picture

Change record draft created: https://drupal.org/node/2199185

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I am fine with this now, though klausi, seriously, please don't make the life of reviewers harder than it has to be and upload an easy to access interdiff manually.

klausi’s picture

88: 2019123.patch queued for re-testing.

webchick’s picture

Assigned: Unassigned » Crell

Ok, this has sat here at RTBC for quite awhile, so I took a peek. I wasn't expecting an issue with such a nice, simple change notice to involve 55K worth of changes. :) Though a lot of them are in tests.

I don't follow everything that's going on in here exactly, but the changes are salient and well-commented and tested. When I pinged Crell about it though he said he had some earlier architectural concerns and wanted to take an eyeball first. Assigning to him.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Moving out of the RTBC queue to reflect that.

Crell’s picture

This looks reasonable to me overall. I only have one comment worth mentioning, and it's for the tests:

+++ b/core/modules/rest/lib/Drupal/rest/Tests/AuthTest.php
@@ -46,7 +46,7 @@ public function testRead() {
-    $response = $this->httpRequest('entity/' . $entity_type . '/' . $entity->id(), 'GET', NULL, $this->defaultMimeType);
+    $this->httpRequest($entity_type . '/' . $entity->id(), 'GET', NULL, $this->defaultMimeType);

If we have the $entity, why are we forming the URI manually? Just ask the $entity for it, no?

(That appears a few dozen times in the tests.)

Not RTBCing yet pending answer to the above.

tim.plunkett’s picture

$entity->url() ftw

klausi’s picture

FileSize
56.37 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Now using the entity url() method in the tests as proposed!

Status: Needs review » Needs work

The last submitted patch, 98: 2019123.patch, failed testing.

tim.plunkett’s picture

Please upload an interdiff so we can see what broke between this and #88.

klausi’s picture

klausi’s picture

Status: Needs work » Needs review

98: 2019123.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 98: 2019123.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
56.59 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Status: Needs review » Needs work

The last submitted patch, 105: 2019123.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
56.58 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Status: Needs review » Needs work

The last submitted patch, 107: 2019123.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
56.95 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Very good, this now finally passes the testbot again.

1) We have to use $entit->getSystemPath() instead of ->url() because we are generating the full URL later. Interdiff: https://github.com/klausi/drupal/commit/306aaf32677c52da405a647b4b904348...

2) InstallUninstallTest catches some interesting corner cases when entity routes might not be available during plugin derivative building. That's why I'm catching RouteNotFoundException and just exclude the incomplete entity type during module install/uninstall phases. It is ugly and you won't like it, but it is robust. Better suggestions welcome: Interdiff: https://github.com/klausi/drupal/commit/84e40f8e87900ec22e60af88f1add739...

Crell’s picture

Status: Needs review » Reviewed & tested by the community
dawehner’s picture

+1

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/rest/lib/Drupal/rest/Plugin/Derivative/EntityDerivative.php
@@ -74,6 +87,34 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
+              // If the route does not exist it means we are in a brittle state
+              // of module enabling/disabling, so we simply exclude this entity
+              // type.
+              unset($this->derivatives[$entity_type_id]);
+              // Continue with the next entity type;

Doesn't this mean the plugin derivates are built before the routes? If it's consistent, then changing the order of those ought to be enough no?

klausi’s picture

Status: Needs work » Needs review

The derivatives have to be built before routes, because during route building those derivatives are invoked. So they will have to deal with outdated route information, because the new routing information is just being built. It's a cyclic dependency: we need existing routes while we are building routes, so that circle will break somewhere and we are catching that here.

I think we cannot change that order, what would you suggest?

dawehner’s picture

@klausi
This really reminds me of the discussion on #2158571: Routes added in RouteSubscribers cannot be altered

Crell’s picture

Status: Needs review » Reviewed & tested by the community

If #2150747: Switch back to URI templates in entity annotations happens (which I still want it to) that code will become irrelevant anyway. Let's not block this issue on #2158571: Routes added in RouteSubscribers cannot be altered since it's not moving right now. If we can do better on that bit of code later it's not an API change, whereas moving URLs around IS an API change and needs to happen.

webchick’s picture

Assigned: Crell » catch

Back to catch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 109: 2019123.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
57.18 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Simple reroll, fixed the merge conflict, back to RTBC.

Interdiff: https://github.com/klausi/drupal/commit/64d0afdabf437bbd49813fbead9cf343...

  • Commit 5f61e26 on 8.x by catch:
    Issue #2019123 by klausi, ygerasimov, Crell: Use the same canonical URI...
catch’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
955 bytes

OK I added a @todo for that issue locally and committed/pushed this one. Interdiff attached.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

yanniboi’s picture

Title: Use the same canonical URI paths as for HTML routes » Change record: Use the same canonical URI paths as for HTML routes
Status: Closed (fixed) » Active

Updating documentation for this change.

yanniboi’s picture

Issue summary: View changes
Status: Active » Needs review

Rather than creating a change record, I updated the documentation page here: https://drupal.org/node/2096019

klausi’s picture

Title: Change record: Use the same canonical URI paths as for HTML routes » Use the same canonical URI paths as for HTML routes
Status: Needs review » Closed (fixed)

Thanks! Change record already exists, linked above.