Problem/Motivation

Let's assume we have a rest route on /node similar to what #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available does.
Currently views overrides this route on /node with its own internal controller no matter what

Proposed resolution

Just add it, if needed.

Remaining tasks

Figure out whether we want to filter by more than request verb. What about _format etc.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 interdiff.txt911 bytesdawehner
#2 2730497-2.patch3.03 KBdawehner

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2730497-2.patch3.03 KB
new interdiff.txt911 bytes
dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review

.

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Views overrides existing REST routes » REST Views override existing REST routes
Version: 8.2.x-dev » 8.1.x-dev
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +VDC, +DrupalWTF, +DX (Developer Experience), +API-First Initiative
Related issues: +#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

Figure out whether we want to filter by more than request verb. What about _format etc.

I think that can happen in a follow-up. This solves a big frustration/WTF.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 9b650e9 and pushed to 8.2.x. Thanks!

  • catch committed 9b650e9 on 8.2.x 
    Issue #2730497 by dawehner: REST Views override existing REST routes

Status: Fixed » Closed (fixed)

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Closed (fixed) » Active

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.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Also, this was not pushed to 8.1.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2772537: REST Views override existing REST GET routes

So, that follow-up mentioned in #5 and #9 now is #2772537-11: REST Views override existing REST GET routes.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Reviewed & tested by the community

I'd reclose this, but this really should be committed cherry-picked to 8.1 also.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2730497-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

I agree to be honest. Its quite a clusterfuck for actual sites.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -237,8 +237,8 @@ public function alterRoutes(RouteCollection $collection) {
+      if (!$route->hasDefault('view_id') && ('/' . $view_path == $route_path) && (!$route->getMethods() || in_array('GET', $route->getMethods()))) {

    Let's do a strict comparison here ... 

  2. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
index 90d2ae7..9241a35 100644
--- a/core/modules/views/tests/src/Unit/Plugin/display/PathPluginBaseTest.php

--- a/core/modules/views/tests/src/Unit/Plugin/display/PathPluginBaseTest.php
+++ b/core/modules/views/tests/src/Unit/Plugin/display/PathPluginBaseTest.php

    I still like that we have a unit test here :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#16: we just need to cherry-pick the commit from 8.2 to 8.1, we don't want to change it here I think?

dawehner’s picture

dawehner
Primary language German
commented

#16: we just need to cherry-pick the commit from 8.2 to 8.1, we don't want to change it here I think?

Well, I guess i'm a bit stricter with my own patches, but sure, we should just cherry pick, I haven't seen that.

  • catch committed 5552cb8 on 8.1.x 
    Issue #2730497 by dawehner: REST Views override existing REST routes

(...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Cherry picked to 8.1.x.

Agreed on the strict comparison - I'd have asked for that if I'd spotted it. Let's do that in a follow-up though.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yes! And we now have that follow-up: #2772537: REST Views override existing REST GET routes.

  • catch committed 9b650e9 on 8.3.x 
    Issue #2730497 by dawehner: REST Views override existing REST routes

  • catch committed 9b650e9 on 8.3.x 
    Issue #2730497 by dawehner: REST Views override existing REST routes

Status: Fixed » Closed (fixed)

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