Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- Currently, "page" views override all GET routes with the same path.
- Consequence: REST GET routes are overridden too.
- Consequence: the default Taxonomy Term view makes it literally impossible to GET a Taxonomy Term via REST.
Proposed resolution
\Drupal\views\Plugin\views\display\Page
should only override routes that are render array-based, i.e. that have no_format
- (although perhaps those routes should get a default
_format
route requirement, just like\Drupal\Core\EventSubscriber\RouteMethodSubscriber::onRouteBuilding()
already sets a default_method
requirement?) \Drupal\rest\Plugin\views\display\RestExport
shouldn't alter any routes
Remaining tasks
TBD
User interface changes
None.
API changes
TBD
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#48 | interdiff.txt | 3.45 KB | dawehner |
#48 | 2772537-48.patch | 10.61 KB | dawehner |
#37 | interdiff.txt | 3.21 KB | Wim Leers |
#37 | 2772537-37.patch | 9.43 KB | Wim Leers |
#20 | interdiff.txt | 1021 bytes | dawehner |
Comments
Comment #2
jacov CreditAttribution: jacov as a volunteer commentedit looks like the request is being routed to a canonical route defined in core/modules/taxonomy/taxonomy.routing.yml
based on this, it looks like the controller is expecting termTitle, where the REST endpoint is expecting the term_id....
this will never work till the routing issue is corrected and the proper handling is defined in the controller in: core/modules/taxonomy/src/Controller/TaxonomyController.php
Comment #3
jacov CreditAttribution: jacov as a volunteer commentedComment #4
Wim LeersThis is not remotely a critical bug. See https://www.drupal.org/core/issue-priority#critical-bug.
Comment #5
Wim LeersPlease post your
rest.settings.yml
.Comment #6
Wim LeersAlso, 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.
Comment #7
jacov CreditAttribution: jacov as a volunteer commentedthis is not a duplicate, this is a separate issue on a completely separate endpoint that produces a different error then #2772413: REST GET fails on entity/taxonomy_vocabulary/{id} 403 Forbidden with error
Comment #8
jacov CreditAttribution: jacov as a volunteer commentedrest.settings.yml
Comment #9
Wim LeersOh, sorry, I didn't notice that difference! My bad!
Okay, great. You have enabled the taxonomy term REST resource. Looking at
\Drupal\taxonomy\TermAccessControlHandler::checkAccess()
, you need theaccess
content permission to be allowed to GET Taxonomy Terms entities. So, does the user that's doing REST requests have that permission?Comment #10
jacov CreditAttribution: jacov as a volunteer commentedyes, i checked permissions, and i have enabled both:
Access GET on Taxonomy term resource
Access GET on Taxonomy vocabulary resource
Comment #11
Wim LeersWhen 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:Hence repurposing this issue to fix that very important bug in Views.
Comment #12
Wim LeersHere'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.)
Comment #13
dawehnerWe 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?
Comment #14
Wim Leers#13: see the IS changes I made in #11 for the solution I proposed.
Comment #16
Wim Leers#12 had exactly one fail, in exactly the one expected test. Excellent.
Comment #17
Nick_vhJust 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!
Comment #18
dawehnerThis 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?Comment #20
dawehnerLet's see ...
Comment #21
Wim Leers@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.
Comment #23
dawehnerI pinged tim on IRC for a review.
Comment #24
Wim LeersPinged tim.plunkett again.
Comment #25
tim.plunkettExtreme nit: On some of these multiline statements you indent, others you don't.
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.
Comment #26
alexpottCommitted 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 :)
Fixed on commit - yay for PHPCS.
Comment #28
Wim Leers+1
Comment #29
tveron CreditAttribution: tveron commentedHello,
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?
Comment #30
dawehnerLet's try to get this into 8.2.x
Comment #31
xjmSo 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:
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?
Comment #32
dawehnerWell, I guess there is no reason not to. I'm still wondering whether we should go straight to private for those.
Comment #33
dawehnerSo the change record would be: In case you already worked around this problem, now you can continue to do so?
Comment #34
Wim LeersClosed #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!
Comment #35
Wim LeersComment #36
dawehnerI 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:
instanceof
By that there is no risk involved of breaking any side, because its always an explicit decision to use that new functionality.
Comment #37
Wim LeersLet's get this going again.
GET
taxonomy terms via REST.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.
Comment #38
dawehnerIs there a reason why we don't experiment with #36, as that has the potential to solve it properly without further risks?
Comment #39
Wim LeersI 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?
Comment #40
dawehnerIts 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
Comment #41
Wim LeersHow does this require module authors to make changes between 8.2.x and 8.3.x?
Comment #42
dawehnerWell, I don't consider
_overrideApplies
as the API we want to keep around.Comment #43
dawehnerThe 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.
Comment #44
Wim LeersAh, I see now. That is a very good point!
Comment #45
Wim Leers(Thanks again for explaining!)
So, applying the approach of #36 requires:
Correct?
Comment #46
dawehnerI 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.
Comment #47
Wim LeersWell, 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.
Comment #48
dawehner#36 is not perfect, but here is an implementation of it.
Comment #49
Wim LeersOMG I totally missed #48. Great work @dawhener. That's not how I understood #36, but it makes sense.
Comment #50
dawehnerSorry for not making this more clear.
Comment #51
Wim LeersNo worries. This sort of thing is very hard to explain with just words.
Comment #53
Wim LeersRandom testbot fail (could not create directories etc). Retesting.
Comment #54
darrenmothersele CreditAttribution: darrenmothersele commentedI 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,
Comment #56
Wim LeersSigh random testbot fail. It's already green again.
Comment #58
Wim LeersComment #60
Novitsh CreditAttribution: Novitsh as a volunteer and commentedComment #64
Wim Leers8.2.x is closed. This won't be committed there. This was committed to 8.3.x. So, done!
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedIt is still not very clear , which rest method to be used for fetching taxonomy terms. It also leads to NULL data sometimes.