REST Views override existing REST GET routes

This is not remotely a critical bug. See https://www.drupal.org/core/issue-priority#critical-bug.

Please post your rest.settings.yml.

Also, for now it's not yet clear that this needs to be a separate issue compared to #2772413: REST GET fails on entity/taxonomy_vocabulary/{id} 403 Forbidden with error.

Oh, sorry, I didn't notice that difference! My bad!

 'entity:taxonomy_term':
  …

Okay, great. You have enabled the taxonomy term REST resource. Looking at \Drupal\taxonomy\TermAccessControlHandler::checkAccess(), you need the access content permission to be allowed to GET Taxonomy Terms entities. So, does the user that's doing REST requests have that permission?

When requesting GET /taxonomy/term/1?_format=hal_json, this is what we see in \Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher::matchRequest():

Then, after having applied route filters, this is what you see (this was tested with #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response applied, which adds a HTTP method filter to the routing system, in HEAD you'll still see the above screenshot):

So, we end up with multiple candidates for this route… the problem is that Symfony just picks the first one.

But wait! Turns out there's a reason why we end up with so many routes. Views. Specifically, the /taxonomy/term/% view is altering all GET routes with the /taxonomy/term/% path in \Drupal\views\Plugin\views\display\PathPluginBase::alterRoutes(). Even despite #2449143: REST views specify HTML as a possible request format, so if there is a "regular" HTML view on the same path, it will serve JSON!

Hence the multiple matching routes you end up with while this view is enabled all have the same fit, and Symfony just picks the first one.

So, the problem really is still Views' \Drupal\views\Plugin\views\display\PathPluginBase::alterRoutes(). Like I wrote at #2730497-9: REST Views override existing REST routes:

This actually was an incredibly incomplete fix. This was a small step in the right direction, but me saying in #5 that we could defer the _format stuff was actually a mistake now. (Although it did make the situation better in some cases.) We really need to fix that.

Hence repurposing this issue to fix that very important bug in Views.

Here's a patch proving this is broken in HEAD. (Switching to 8.2, because #2730497: REST Views override existing REST routes was not committed to 8.1, even though it should have been.)

We should always ensure to have the same kind of fix in page_manager as well. It inherits all that kind of problems.

I agree altering existing routes is by far not an easy job, and hard to get right, given that you potentially even would like to override REST provided routes? Maybe we need to be able to tell, these are potential routes you would override with that or so?

#13: see the IS changes I made in #11 for the solution I proposed.

#12 had exactly one fail, in exactly the one expected test. Excellent.

Just bumped into this. It would already be useful if the error message showed that it was coming from the view. The only thing it shows now is " { "message": "Not acceptable format: json" } " which is confusing.

dawehner told me to disable the view and that worked and solves our usecase so we can work around it, but would be great to make sure it doesn't overlap and create a vague conflict. Our usecase iterates over all the rest routes and automatically adds another serializer support to it. This issue prevents us from doing it for all the entity types with the default path so If I need to review and test, please let me know!

+++ b/core/modules/views/tests/src/Unit/Plugin/display/PathPluginBaseTest.php
@@ -266,7 +266,7 @@ public function testAlterRoute() {
-    $route->setMethods(['POST']);
+    $route->setMethods(['GET', 'POST']);

This cannot be the only way to distinct this behaviour. In this case its a HTML route, and for that one, we want to override.

Here is a patch which solves the problem better, but still, its entirely not obvious for me at least, how the logic of \Drupal\rest\Plugin\views\display\RestExport::overrideApplies should exactly look like. Potentially a rest view route might want to override an existing normal rest route, right?

Let's see ...

@Nick_vh: to be clear, we can still fix this after today in Drupal 8.2 (and even in 8.1), because this is a pure bug. It's not adding a new feature, nor an optional refactoring of some kind.

I pinged tim on IRC for a review.

Pinged tim.plunkett again.

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -348,6 +350,26 @@ public function collectRoutes(RouteCollection $collection) {
   +    return $this->overrideAppliesPathAndMethod($view_path, $view_route, $route)
   +      && (!$route->hasRequirement('_format') || array_intersect($route_formats, $view_route_formats) != []);
   
   +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -227,18 +227,61 @@ public function collectRoutes(RouteCollection $collection) {
   +    return $this->overrideAppliesPathAndMethod($view_path, $view_route, $route)
   +      && (!$route->hasRequirement('_format') || $route->getRequirement('_format') === 'html');
   ...
   +    return !$route->hasDefault('view_id')
   +    && ('/' . $view_path == $route_path)
   +    // Also ensure that we don't override for example REST routes.
   +    && (!$route->getMethods() || in_array('GET', $route->getMethods()));
   

   Extreme nit: On some of these multiline statements you indent, others you don't.

2. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -227,18 +227,61 @@ public function collectRoutes(RouteCollection $collection) {
   +    $route_path = RouteCompiler::getPathWithoutDefaults($route);
   +    $route_path = RouteCompiler::getPatternOutline($route_path);
   

   In the route override code of Page Manager, we also used to use getPathWithoutDefaults and getPatternOutline, but switched to $route_path = RouteCompiler::getPatternOutline($route->getPath()); instead, as we wanted to keep the default arguments from existing routes. I don't think we want those here, so this is fine.

+1, nice work.

Committed f8f3983 and pushed to 8.3.x. Thanks!

I think we should consider fixing this in 8.2.x once 8.2.0 is released - this seems are very annoying bug.

Leaving #25.1 alone - we can wait for a coding standard to fix this :)

diff --git a/core/modules/rest/src/Plugin/views/display/RestExport.php b/core/modules/rest/src/Plugin/views/display/RestExport.php
index 15942d1..74a2762 100644
--- a/core/modules/rest/src/Plugin/views/display/RestExport.php
+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -7,7 +7,6 @@
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Render\RenderContext;
 use Drupal\Core\Render\RendererInterface;
-use Drupal\Core\Routing\RouteCompiler;
 use Drupal\Core\Routing\RouteProviderInterface;
 use Drupal\Core\State\StateInterface;
 use Drupal\views\Plugin\views\display\ResponseDisplayPluginInterface;
@@ -359,8 +358,8 @@ public function collectRoutes(RouteCollection $collection) {
    * @param \Symfony\Component\Routing\Route $route
    *   The route itself.
    *
-   * @return bool TRUE, when the view should override the given route.
-   * TRUE, when the view should override the given route.
+   * @return bool
+   *   TRUE, when the view should override the given route.
    */
   protected function overrideApplies($view_path, Route $view_route, Route $route) {
     $route_formats = explode('|', $route->getRequirement('_format'));

Fixed on commit - yay for PHPCS.

I think we should consider fixing this in 8.2.x once 8.2.0 is released - this seems are very annoying bug.

+1

Hello,

I currently use version 8.2.1 and I must use GET REST routes but it's not possible.

Is the patch will soon be released please?

Let's try to get this into 8.2.x

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -227,18 +227,61 @@ public function collectRoutes(RouteCollection $collection) {
+  protected function overrideApplies($view_path, Route $view_route, Route $route) {
...
+  protected function overrideAppliesPathAndMethod($view_path, Route $view_route, Route $route) {

So the risk of backporting this patch in its current form is that a path plugin extension which already defines a method with one of these names will break. There is also a similar risk for something subclassing the REST export display, I think. The questions are:

1. Does the value of this bugfix outweigh the risk of theoretical breakage? Probably.
2. Should we use an underscore prefix for the backport to eliminate the risk at the cost of a slight divergence between patch and minor branches?Maybe.
3. Should we add a change record? Yes.

One unrelated question I have after reading this is, why are there identically named methods on two different plugins, without any parent API I can see?

Should we use an underscore prefix for the backport to eliminate the risk at the cost of a slight divergence between patch and minor branches?Maybe.

Well, I guess there is no reason not to. I'm still wondering whether we should go straight to private for those.

Should we add a change record? Yes.

So the change record would be: In case you already worked around this problem, now you can continue to do so?

Closed #2797267: GET taxonomy/term returns error [406:Not acceptable format: json] as yet another duplicate of this. Lots of people are running into this problem. Let's get this fixed!

I strongly believe in order to keep the minor vs. patch fight sane, we have to always ask: is there any chance that this might break sites. In case it doesn't, its obvious, we don't do it.
After thinking about that for a while, here is an approach thought, which could work:

• Move the methods onto a trait
• Check in PathPluginbase whether static uses this trait, using instanceof
• Use that trait in each individual implementation, Page, RestExport etc.

By that there is no risk involved of breaking any side, because its always an explicit decision to use that new functionality.

Let's get this going again.

So the risk of backporting this patch in its current form is that a path plugin extension which already defines a method with one of these names will break. There is also a similar risk for something subclassing the REST export display, I think. The questions are:

1. Does the value of this bugfix outweigh the risk of theoretical breakage? Probably.
2. Should we use an underscore prefix for the backport to eliminate the risk at the cost of a slight divergence between patch and minor branches?Maybe.
3. Should we add a change record? Yes.

1. Requires no further work. I'd replace "probably" with "definitely". This bug makes it impossible to GET taxonomy terms via REST.
2. I think this is a fair compromise! The risk is currently tiny. This reduces that risk again 100-fold.
3. I don't think a CR is necessary; this is simply a bugfix. What should the CR say? That a bug is fixed and extremely crazy edge cases may no longer work? That's the case for almost every bugfix. @dawehner also doesn't seem to see the value of a CR in this case (see #33).

Rerolled with @xjm's suggestion of adding an underscore prefix to eliminate the risk at the cost of a slight divergence between 8.2.x and 8.3.x.

Since this implements the core committers' suggestion directly, moving to RTBC immediately. I'll write a CR once a core committer explains what the CR should communicate.

Is there a reason why we don't experiment with #36, as that has the potential to solve it properly without further risks?

I thought #36 was talking more about a general new system to approach BC-sensitive bugfixes like this one. I don't think we need to delay this bugfix any further — this is good enough is it not?

Its tough, because this introduces a bad API for 8.3.x and 8.2.x or it requires module authors to update between 8.2.x and 8.3.x

How does this require module authors to make changes between 8.2.x and 8.3.x?

Well, I don't consider _overrideApplies as the API we want to keep around.

The underscore approach works really fine, when you deal with methods which are not meant to be overridden. In this case the method is actually meant to be overridden.

Ah, I see now. That is a very good point!

(Thanks again for explaining!)

So, applying the approach of #36 requires:

1. a new patch for 8.2.x
2. a patch that updates the fix for 8.3.x

Correct?

I guess? Yeah I mean this is a bug since over a year, it is not the end of the world when it doesn't land immediately.

Well, people keep running into it. And keep getting the perception Drupal 8's REST is atrociously broken. I was pinged by colleagues about this bug still being a big problem in Drupal 8.2.

#36 is not perfect, but here is an implementation of it.

OMG I totally missed #48. Great work @dawhener. That's not how I understood #36, but it makes sense.

Sorry for not making this more clear.

No worries. This sort of thing is very hard to explain with just words.

Random testbot fail (could not create directories etc). Retesting.

I was getting { "message": "Not acceptable format: hal_json" }

I've just applied patch in #48 to an D8 v8.2.4 site, and I can now GET taxonomy terms via REST.
Thanks,

Sigh random testbot fail. It's already green again.

8.2.x is closed. This won't be committed there. This was committed to 8.3.x. So, done!