I think we need a new test for getting taxonomy terms via the REST API.
When you try to get a taxonomy term with an "Accept" header of application/json
. you get the following error:
{"message":"Not Acceptable.","supported_mime_types":["text\/html","application\/vnd.drupal-ajax","application\/vnd.drupal-dialog","application\/vnd.drupal-modal"]}
This has been tested on beta 7. Of course rest has been enabled for taxonomy terms and access has been granted. One must clear the cache after enabling rest for entities. taxonomy/term/{tid}
produces the error but node/{nid}
works with application/json
.
Suggested commit message:
Issue #2449143 by damiankloip, Wim Leers, dawehner, Tybor: 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
Comment | File | Size | Author |
---|---|---|---|
#179 | interdiff-2449143-179.txt | 734 bytes | damiankloip |
#179 | 2449143-179.patch | 23.97 KB | damiankloip |
#176 | 2449143-176.patch | 24.02 KB | tedbow |
#176 | interdiff-172-176.txt | 1.26 KB | tedbow |
#172 | 2449143-172.patch | 23.74 KB | tedbow |
Comments
Comment #1
Jaesin CreditAttribution: Jaesin commentedComment #2
larowlanTry application/hal+json
Comment #3
Jaesin CreditAttribution: Jaesin commentedWith Hal + appropriate permissions enabled on beta6 and beta7:
Status Code: 406 Not Acceptable
Comment #4
brstdev5 CreditAttribution: brstdev5 commentedI am also getting same error
Status Code: 406 Not Acceptable
Resposne:-
{"message":"Not Acceptable.","supported_mime_types":["text\/html","application\/vnd.drupal-ajax",null
,"application\/vnd.drupal-dialog","application\/vnd.drupal-modal"]}
Any solutions will be helpful
Best regards,
dev
Comment #5
Jaesin CreditAttribution: Jaesin commentedContent Negotiation is still in flux as per #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use. Using the header for content negotiation will likely be eliminated in core in favor of link generation (url) for specific content output formats.
Comment #6
mgiffordComment #7
rcaracaus CreditAttribution: rcaracaus as a volunteer commentedStill getting
{
message: "Not acceptable"
}
for
http://dev-mysite.pantheon.io/taxonomy/term/32?_format=hal_json
Comment #8
ra. CreditAttribution: ra. commentedI've resolved this issue when I've disabled default taxonomy term views and recreate similar views page with REST export
Comment #9
jfrederick CreditAttribution: jfrederick at NBCUniversal commentedI am still seeing this problem in 8.0.1.
When I expose taxonomy terms via the REST API, visiting
http://dev-mysite.pantheon.io/taxonomy/term/32?_format=json
results inMessage: Not Acceptable.
If I access the URL via curl, I get 403 Access Denied.
Comment #10
jfrederick CreditAttribution: jfrederick at NBCUniversal commentedSimilar to #8, I worked around this problem by disabling the default View taxonomy_term. That View was taking precedence when I visited the path I listed above in #9. Thankfully I don't need that View, so I just disabled it, and the REST response took its place as the prioritized route.
In a clean core installation, if I enabled taxonomy terms as a REST Resource, it wouldn't work without first disabling the default View, which is not obvious at all. So I think this is a bug.
Comment #11
omarlopesinoI had the same problem. I debbuged this and i found the problem is, because the route is the same as taxonomy term view, there are a conflict and taxonomy term view reacts first than rest module.This doesn't happens in all cases, like node.
I created another issue because it affects all entity types: Entity resource urls shouldn't be the same as entity view urls.
Comment #12
swentel CreditAttribution: swentel commentedCan be fixed in 8.0.x (hopefully)
Comment #13
dawehnerSo the actual underlying issue is "interesting". Its caused that overrides in views works on the path level without taking into account formats.
Comment #14
Wim Leers#13: that very much reminds me of the routing problems uncovered by/described in #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response.
Comment #15
Wim LeersShould we move this to
?Comment #16
Wim Leers#2658926: Entity resource urls shouldn't be the same as entity view urls was a duplicate of this. It was marked ,.
Comment #17
Wim LeersComment #18
Wim LeersMarked #2605906: Frontpage shows json instead of HTML after adding a rest export display as a duplicate of this because it AFAICT has the same underlying root cause wrt Views' routing.
Quoting @dawehner in #2605906-7: Frontpage shows json instead of HTML after adding a rest export display there:
Comment #20
Wim LeersPer #18.
Comment #21
Wim LeersI think this is a clearer title.
Comment #22
dawehnerAt this point this basically reverts #1969870: REST export view should default to JSON, so yeah we have to ask whether we want that.
Comment #23
Wim LeersNice, thanks for the link.
And, as is often the case, @effulgentsia predicted exactly this problem:
I guess an alternative solution is to make our route matcher smarter: if a particular route only supports HTML, prefer that over others.
\Symfony\Component\Routing\RouteCollection
is a list, not a set, so this is possible AFAICT. In fact,\Drupal\accept_header_routing_test\Routing\AcceptHeaderMatcher::filter()
does exactly that.Comment #25
dawehnerRight, accept header negotation is tricky and we are aware of that. The problem is that we basically need to introduce again accept header based routing, if we want to distinct the two routes ... Maybe an alternative idea, in the views UI render the link "view page" with
?_format=json
?Comment #26
Wim Leers+1
Well, no, it would just be smarter matching?
_format: 'html'
vs._format: 'json|xml|hal_json|html'
would result in the first one being picked.Comment #27
Wim LeersNote: the failing test coverage was introduced at #1891202: REST Export Views format should be configurable with @todos specifically for this problem. So, that's another related issue.
Comment #28
dawehnerSo yeah the logic in
\Drupal\Core\Routing\RequestFormatRouteFilter::filter
is a bit different. When there is no format provided we move it to the end. I think you basically propose to special case HTML here, or assume no format === HTML ?Comment #29
dawehnerComment #31
dawehnerI think this was just a random failure.
Comment #32
Wim LeersThe comment needs to be updated.
This should be testing the
rest_export
display plugin?Usually, routes don't have a method specified. Therefore this test shouldn't specify one either, otherwise we're testing an edge case instead of the common case.
I think the real problem is this code in
RestExport
:The reason this was added in #1969870: REST export view should default to JSON: "I want to get JSON and not have to specify the necessary ?_format=json or Accept:json because that's easier if I'm a dumb user". This does not make sense. Developers using REST surely know how to specify the format they want. So, let's omit
'html'
there.Comment #33
dawehnerGood question ... but well, actually the case layed out in the issue summary is a page display overriding a REST route, not the other way round.
Well, there are two problems we have here.
a) the rest display takes over the HTML route as well.
b) the views HTML route replaces the REST resource route.
Comment #34
Wim LeersRight, so:
html
as a format on REST Export displayshtml
as a format on path displaysAFAIK we need both?
Comment #35
dawehnerI would be in favour of doing that, but we would first have to ask webchick, whether its okay to drop that feature again. I don't feel to be in any position to disagree with other people's opinion.
Well, its not just that. What happens on the /node POST issue is that both the /node POST route and the /node GET route contains the view at the end. Feel free to debug it again if you don't believe me :)
We could not also take into account the METHOD but also use the _content_type_format requirement see:
$route->addRequirements(array('_content_type_format' => implode('|', $this->serializerFormats)));
Comment #36
webchickThe original issue summary at #1969870: REST export view should default to JSON was just a "feature" I just accidentally blundered into, so it's possible to revert it. I would say that it is super convenient though to be able to view JSON in the browser (as opposed to having to use Postman or cURL), so I wonder if it's possible to retain that functionality, but only with adding ?_format=html add-on or similar, and having the "View page" link in Views automatically set that, as you described. Is that doable at all?
Comment #37
Wim LeersYes, that's what I said at the bottom of #32.
#35/@dawehner: so we now have a +1 from webchick.
Comment #38
Wim LeersThis also fixes the test fail in #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available's patch (which is otherwise ready), so this blocks that issue.
Comment #39
dawehner@wim
So I think we have multiple issues here, so we should split up the issue into one which includes the current patch and one for the 'html' route problem.
Comment #40
Wim LeersWorks for me.
Comment #41
dawehnerAdded #2730497: REST Views override existing REST routes for the other problem
Comment #42
Wim LeersComment #43
Wim LeersHurray, #2730497: REST Views override existing REST routes landed!
Comment #45
xjmThe title and the summary are inconsisten here, so it would be good to update the summary with the latest direction for the issue.
Comment #46
damiankloip CreditAttribution: damiankloip at Acquia commentedSo I think we ideally want to do something like this?
Comment #47
dawehner+1 for me
Comment #49
Wim LeersThis method should really be renamed to
setFormat()
, but that's probably out of scope.YES!
This will also still work for
?_wrapper_format=drupal_dialog
et cetera, because those are wrapper formats for thehtml
format.This is also exactly what I proposed in #32, in May 2016, about 8 months ago.
Comment #50
damiankloip CreditAttribution: damiankloip at Acquia commentedLet's see if this improves things at all.
Comment #51
dawehnerThis looks super good for me.
<3
Mh, that just shows how well the routing system is designed ;) It makes it hard to link to it.
Comment #53
damiankloip CreditAttribution: damiankloip at Acquia commentedYeah, exactly, Not sure how else we can handle this without special casing the rest export display explicitly, which I don't think we want to do :/
Comment #54
damiankloip CreditAttribution: damiankloip at Acquia commentedLet's try this. I think this should solve most of those problems.
Comment #55
damiankloip CreditAttribution: damiankloip at Acquia commentedSorry, that one is not good!
Comment #58
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #59
damiankloip CreditAttribution: damiankloip at Acquia commentedA couple of notes about the test coverage removal:
This comment is incorrect for test assertions below. However, the actual assertions are no longer relevant. We don't expect JSOn to be returned anymore and the HTML cases are already tests for 406 responses above.
Same with this hunk, it's no longer relevant. As we don't support the same fallback type functionality with this patch that we do now, and all other permutations are already tested in this method I think.
Comment #60
Wim LeersCurious what @dawehner has to say about this patch :)
I will look into the root cause of needing this:
I already have suspicions, but I want to be certain. @damiankloip and I already talked about one possible solution: let display plugins implement an optional interface where they can provide this information themselves — the
RestExport
display plugin would then implement that interface, hence avoiding this exception. But I'd rather not need such an additional interface of course.Comment #61
dawehnerWhy this happens is quite easy. Route filters are invoked when matching a path in
\Drupal\Core\Path\PathValidator::getPathAttributes
.One solution could be to skip route filters by using the route provider directly and then apply access checking on top of it.
Comment #62
dawehnerIMHO we should add a pointer to #2852107: PathValidator::getUrlIfValid() does not support non-HTML/non-GET routes and call it a day.
Comment #63
damiankloip CreditAttribution: damiankloip at Acquia commentedSo we could remove it/fix it when that issue is done?
Comment #64
dawehner@damiankloip
I would say we keep the workaround what we have now, and maybe add a pointer to the other issue.
Comment #65
Wim LeersWhy not simply do this instead?
Comment #66
dawehnerNice idea!
Comment #67
dawehnerI just fear that this could be a BC problem with potential plugins extending this one. Yes they might do it wrong, but is this change really required?
Comment #68
Tybor CreditAttribution: Tybor commentedComment #69
Ky1eT CreditAttribution: Ky1eT as a volunteer commentedComment #70
Tybor CreditAttribution: Tybor at Achieve Agency commentedConfirmed not working in Drupal 8.2.5, as well as 8.4.x
I used Kalabox to recreate a Pantheon site in 8.2.5 and simplytest.me to test in 8.4.x.
Steps to recreate in Drupal 8:
0. Create a "drupal" project from https://simplytest.me. After you've chosen your project type, set the version to 8.4.x (at the bottom of the list).
Once you have your fresh site created and are on the home page:
1. Go to Structure >> Taxonomy >> Tags (List Terms) >> Add Term (Can be anything) and create a term.
2. Click to view the term and you will be taken to the view page of the term. It will be https://{{Your Site Here}}/taxonomy/term/{{Term #}}
3. Add "?_format=json" to the end as a query string and you will see a JSON- returned string with the error message: "message": "Not acceptable format: json"
Summary is not up to date, however tag has already been applied.
Comment #71
dawehnerOn the other hand. This is what you got from extending classes. You want to inherit its behaviour, if you want a different one, specify it.
Comment #72
Wim Leers#71 So this means you're saying we should disregard what you said in #67?
What do you think is still necessary here?
Comment #73
dawehnerI'd say go with it now. We fix a way bigger problem with this issue than we maybe introduce.
Comment #74
alexpottIt'd be great to get an issue summary update and also clarify the BC implications of making this change. It's seems younger @dawehner is more cautious than older @dawehner.
Unused use.
Comment #75
damiankloip CreditAttribution: damiankloip at Acquia commentedAlso, the trouble with the approach from #65 is that we still link to the rest display path from the browser/views listing which will not work anyway. The previous approach didn't link in that case, just displayed the path. Otherwise, I totally like that idea.
EDIT: sorry I think I misread the patch. The result of fromRoute() will only return if the format matches? So would the behaviour of rest export displays not being linked still remain?
Comment #76
damiankloip CreditAttribution: damiankloip at Acquia commentedI tested the latest patch and we do get linked to a 406 response, if you follow it from the browser (which is kind of the point of link tags.).
Also thinking about this a little more, I think when no formats are selected we need to implicitly add all serializer formats that would be available to the route options. Otherwise, you we will will hit the same problem as we have now, that the REST route will response to HTML/anything.
Comment #77
damiankloip CreditAttribution: damiankloip at Acquia commentedLike this... Use the format options from the serializer style plugin if the configured format options are empty. This then honours our behaviour without accepting HTML.
This patch doesn't solve my observation from #75/#76 (first part) yet.
Comment #78
dawehnerI wonder whether its worth to test the case you have no formats selected. To be honest I think this is kind of an edge case
Comment #79
damiankloip CreditAttribution: damiankloip at Acquia commentedWell, in the UI we tell users that no formats selected will be equivalent to all formats. So seems fair to actually do that explicitly.
I think a test for this case would be good. I think we would just need to modify the existing test coverage. I think we still have some coverage for the none selected case...
Comment #80
damiankloip CreditAttribution: damiankloip at Acquia commentedWe only really need to add one additional test for the none selected case that HTML requested results in a 406.
Comment #81
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #84
damiankloip CreditAttribution: damiankloip at Acquia commentedLet's start with a reroll.
Comment #85
damiankloip CreditAttribution: damiankloip at Acquia commentedCorrectly rerolled.
Comment #87
damiankloip CreditAttribution: damiankloip at Acquia commentedThis should fix all of the test failures (Might leave one...) I think. This has uncovered a good bug in
\Drupal\simpletest\WebTestBase::drupalGetWithFormat
when a query string is used. As it was just using the+=
operator before, it was not deep merging the 'query' key, so _format was not being added to those requests.Hopefully now this just leaves the question about how we want to handle the links in the UI. I think we have a couple of options:
1. Revert back to my original method of catching the MethodNotAllowedException and display the path (I think this is my favoured option for now and maybe a follow up to discuss anything else).
2. Add new methods to display plugins to generate UI links, maybe 'getUiLink' or something? This would have a default implementation in PathPluginBase, which RESTExport could then override to basically add a '_format' to the URL for the display. Similar to the getUrl method we already have, but not that. The only trouble with that is do we just choose the first available format if there is more than one configured?
Comment #90
Pavan B S CreditAttribution: Pavan B S at Valuebound commentedShort Array syntax should be used.
Applying the patch, please review
Comment #91
Pavan B S CreditAttribution: Pavan B S at Valuebound commentedComment #92
damiankloip CreditAttribution: damiankloip at Acquia commentedMaybe let's concentrate on the actual problems in this issue first. That is just a nitpick that can be picked up in a final review. If you want a well earned commit credit please do a worthwhile review or something.
Surprisingly that patch failed with the same 2 fails as before.
Comment #94
damiankloip CreditAttribution: damiankloip at Acquia commentedOK, now let's get back to it. This should fix the remaining failures.
Now we just need to do something about my comment from #87 I think:
Comment #95
damiankloip CreditAttribution: damiankloip at Acquia commentedAnnnd, need the patches too..
Comment #96
Wim LeersThis is due to #2825347: 'Accept' header still is used for determining response format of error responses, ?_format= has no effect AND serialization module's exception subscriber does not support status code 500 error responses, which landed only recently. This change makes sense :)
+1 — indeed a follow-up to discuss anything else seems appropriate. We should fix this major bug first, and then we can still make the solution more pluggable/more nicely architected. Because that's something that A) needs more discussion, B) is kind of out of scope for this issue.
So, let's do that, then this is RTBC again.
Comment #97
damiankloip CreditAttribution: damiankloip at Acquia commentedSounds good, +1 for consensus! Here is a new patch with the try/catching again.
Comment #98
Wim LeersComment #100
Wim LeersDrupal CI had a bad moment.
Comment #102
Wim LeersDrupal CI had another bad moment:
Comment #104
Wim LeersAgain.
Comment #106
Wim LeersAgain.
Comment #108
Wim LeersAgain.
Comment #109
Wim LeersThis also blocks #2353611: Make it possible to link to an entity by UUID.
Comment #111
damiankloip CreditAttribution: damiankloip at Acquia commentedAnd back once more. Hopefully for the last time?!
Comment #113
damiankloip CreditAttribution: damiankloip at Acquia commentedRerolled - git resolved all conflicts.
Comment #115
dawehnergit++, just saying
Comment #116
Wim LeersFail reproduced. On it.
Comment #117
Wim LeersRoot cause: #2863267: Convert web tests of views switched
ViewTestBase
fromWebTestBase
toBrowserTestBase
, which does not havedrupalGetJSON()
.Simple fix.
Comment #118
Wim LeersComment #119
damiankloip CreditAttribution: damiankloip at Acquia commentedLooks ok to me! Thanks, Wim.
Comment #121
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #123
Wim LeersTrivial fix. Caused by changes introduced in #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available. Specifically, that issue/patch/commit introduced
\Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber::on4xx()
, which causes 4xx errors other than a 403 or 404 when the requested format ishtml
to also be handled by that exception subscriber, which means we don't fall back to\Drupal\Core\EventSubscriber\FinalExceptionSubscriber
(which always sendstext/plain
responses) for those anymore.You can see several
text/plain
totext/html
changes in #2293697's patch too.In other words: 406, 415 etc error responses can be HTML responses too now. That's all!
Comment #124
Wim Leers#2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available is in, so this is no longer blocking that issue.
Comment #125
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWe're removing some important comments from this function, and I think this replacement is insufficient. This issue is still tagged as "Needs issue summary update", so perhaps that should be done first, and then a more informative code comment could be figured out here?
Are we sure it's safe to remove this? Do we have test coverage for what this says it was doing?
This change is inconsistent with the message provided in the assertResponse() call. Also, we already have a call with an explicitly passed in 'json' format a few lines down already. But I don't think we want to remove the original test of a GET request without an explicit format. Shouldn't we still test that, but change what we're expecting the result to be?
I think same here. I agree with having the explicit test, but shouldn't we also retain the test where we don't pass in an explicit format?
I think this try/catch is too broad. I think it's only needed around fromUserInput(). And then the catch for that can do the same thing that the else is doing: use
base:$path
. Or am I missing a subtlety for why we want $url to not be a Url object?Do we have test coverage for this case? I don't think I see it, but maybe it's there and I missed it?
Comment #126
damiankloip CreditAttribution: damiankloip at Acquia commentedSome very good points.
1. Hmm, not sure about this exactly but tried to improve it a bit :)
2. I don't think we really need it but I have restored it anyway, so it can work as a mechanism for setting the default available format rather than != html OR default.
3. Removed that hunk, it is indeed the same as the block a little below. Added but isn't testing without an explicit format the same as HTML as Drupal will default to html anyway?
4. This is just testing the response formats content, but I guess no harm in adding some other assertions for that...
5. Won't both fromUserInput() and fromUri() throw these Not found exceptions? I would need to refresh my memory though. There seemed to be a good reason to put both in the try() block to initially.
6. Hmm, not sure if there is explicit coverage for that... We might need a new test case for that, with a new test view that contains a page and rest export on the same path.
Comment #128
damiankloip CreditAttribution: damiankloip at Acquia commentedSorry, not sure what I was doing with those tests last night. They should all return 406 as html is no longer supported on the RestExport routes. Which makes the additional coverage with no explicit format passed (exactly the same as trying to pass 'html' ?) even more moot?
Comment #129
damiankloip CreditAttribution: damiankloip at Acquia commentedSo I think this is more correct, and was actually pretty much correct beforehand. testResponseFormatConfiguration tests all the cases with configured formats and their responses, from no format, to html explicitly, to full browser accept headers. testSerializerResponses just tests that, the actual serialized responses. So now that we do not support a fallback through allowing HTML in on the routes this coverage is not needed.
Comment #130
dawehnerWe talked about this issue on the REST major issue triage and agreed this is major, given it blocks for example taxonomy terms to be served by default.
Given the latest failure I think we have proved that we no longer do the fallback, yeah!
Comment #131
damiankloip CreditAttribution: damiankloip at Acquia commentedYeah, totally!
Comment #132
dawehner@damiankloip
Any idea why the testbot doesn't pick up the patch?
Comment #133
Wim LeersLet's reupload.
Comment #134
dawehnerComment #136
damiankloip CreditAttribution: damiankloip at Acquia commentedNo idea what is going on with that other patch. The fail is just my bad, if we request no format, it will default to HTML.
Comment #137
Wim LeersAFAICT committer feedback has been addressed.
Comment #138
damiankloip CreditAttribution: damiankloip at Acquia commented@Wim Leers I think possibly the only thing outstanding is an additional test that has a page and rest export view on the same path, and both are accessible. I don't think we have that yet (A good spot from Alex!).
Comment #139
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNW for #138 if that test is still missing. If it's in the patch, please set back to RTBC.
Comment #140
Wim LeersDo we want that additional test coverage in
\Drupal\rest\Tests\Views\StyleSerializerTest::testSerializerResponses()
, or in\Drupal\Tests\views\Functional\Wizard\BasicTest::testViewsWizardAndListing()
, or both?Comment #141
dawehnerI think the
StyleSerializerTest
is the better place.Comment #142
damiankloip CreditAttribution: damiankloip at Acquia commentedAgree, adding this is
StyleSerializerTest
is the best option I think. I'll try and do this today.Comment #143
damiankloip CreditAttribution: damiankloip at Acquia commentedHere is some test coverage to show failing in the current world, and should all pass with the patch here.
Comment #145
Wim LeersThat is … super clear :)
Just one remark:
This generates a
/test/serialize/shared?_format=html
URL. I think it'd be better to usedrupalGet()
instead, which would result in a/test/serialize/shared
URL, which is the actual real-world scenario.Comment #146
damiankloip CreditAttribution: damiankloip at Acquia commentedYeah, I totally agree :) I was going to change that at the time, then.. didn't. We could even just have both?
Comment #147
damiankloip CreditAttribution: damiankloip at Acquia commentedHow about we match most of the other test assertions and check no format, and XML, like this?
Comment #148
Wim LeersPerfect. All edge cases covered.
Comment #149
dawehnerMore tests is always better!
Comment #152
Wim LeersComment #153
effulgentsia CreditAttribution: effulgentsia at Acquia commented#147 doesn't apply anymore, to either 8.4.x or 8.5.x.
Comment #154
damiankloip CreditAttribution: damiankloip at Acquia commentedReroll
Comment #156
damiankloip CreditAttribution: damiankloip at Acquia commentedMissed a couple of method renames/conversions.
Comment #158
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #159
Wim LeersComment #160
larowlanDo we need a new interface for this? Now that we're expecting the style plugin to implement this method?
Should we also be asserting that we're getting the expected output in the body here? Don't our error handlers still set this header if something goes wrong?
Comment #161
dawehnerThose two plugins are coupled to each other. Maybe we could do an instanceof check or method_exists check and otherwise throw an exception?
Comment #162
Wim LeersWe're asserting that the responses have a
200
code.This patch is adding test coverage that fails in HEAD. Response bodies are already being asserted in
\Drupal\Tests\rest\Functional\Views\StyleSerializerTest::testSerializerResponses()
. We're adding test coverage for the thing that was broken.Comment #163
tedbowAs @dawehner these classes are already coupled together.
RestExport
is already relying on the public functiongetFormats
in \Drupal\rest\Plugin\views\style\Serializer that is not in an interface.This patch adds the method_exists calls for both
getFormats
andgetFormatOptions
since these are the 2 methods that are required of$style_plugin
in\Drupal\rest\Plugin\views\display\RestExport::collectRoutes()
It doesn't make sense to just check for
getFormatOptions
since it would be confusing later on and imply that 1 method was part an interface and the other wasn't.Comment #164
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis looks great, but wouldn't an
instanceof
check be cleaner? Is there any reason not to define an interface with these two methods?Comment #165
dawehnerAt least for me an interface communicates extensibility. But the question is, do we actually want extensibility here?
Comment #166
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think the key feature of an interface is that it communicates expectations. For example,
\Drupal\Core\TypedData\TranslatableInterface
. And to use a style plugin within a REST export display, we're placing expectations on that plugin, so we should communicate those expectations in an interface.Comment #167
dawehnerCould we mark the interface as internal at least?
Comment #168
damiankloip CreditAttribution: damiankloip at Acquia commentedYes, it's true they are pretty tied together. They would probably be one class if that's what views architecture permitted. I guess an interface is ok to add, it probably wont have much use in reality :) Marking it as internal could be a good idea too.
I guess it might be feasible someone would want their own variant of the serializer style plugin.... maybe...
Comment #169
tedbowI am conflicting on what to do. I chatted with @damiankloip about this.
Basically
So I was going to update to
But that has the possibility of breaking any Style plugin that does not extend
\Drupal\rest\Plugin\views\style\Serializer
but works right now. Is that an edge case? Right now the developer would already have to figure out that Style plugin would have to have agetFormats()
method for it to work. So either way whatever we are going to do break existing plugins that work withRestExport
but don't extend\Drupal\rest\Plugin\views\style\Serializer
right now because they won't have publicgetFormatOptions
method.So I could see having an new interface as @effulgentsia suggested. But then if we make it internal as @dawehner and @damiankloip suggested what is the point? I see not wanting to have a new public API but for Style plugins that we break they have to figure out they need to provide a public
getFormatOptions
. If we make the interface internal we would be basically making an interface that makes a style plugin work withRestExport
but then telling contrib not to use it.Looking at \Drupal\views\Plugin\views\style\StylePluginBase there is not interface here but the phpdoc says
I could be wrong but looking at ViewsPluginManager though this doesn't seem to be enforced. Would it make sense to just follow this pattern and update the class phpdoc for
RestExport
saythen although we have the edge of case of breaking a style plugin with this update in the future would could actually add public functions to Serializer without breaking other RestExport serializer Style plugins.
then would actually use
Comment #170
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhat is the actual coupling between
RestExport
andSerializer
? Looks to me like it's just that Serializer is the only style plugin that has an annotation ofdisplay_types = {"data"}
? But why couldn't there be a contrib style plugin that also has that annotation?The other coupling seems indeed to be the
getFormats()
andgetFormatOptions()
methods. Seems to me like those would perhaps make more sense onRestExport
than onSerializer
? If so, then I think movinggetFormats()
might be too much scope for this issue, but what about just movinggetFormatOptions()
? Or, if we think that both methods more logically belong to the "style" than to the "display", then I do think we need an interface for them. Perhaps namedConfigurableFormatInterface
orDataFormatInterface
orStylePluginForDataDisplayInterface
orDataStylePluginInterface
, or ...?I think it would be fine to start with it being @internal, but I think eventually we'll want to better flesh out the pattern of how to define the interfaces that are expected of Views plugins that are specific to particular
display_types
.Comment #171
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI also wonder if this is a more generic problem. I.e., of where display plugins have an implicit requirement for a style plugin to have a particular "option". In which case, I wonder if a more generic API on style plugins would be appropriate: e.g.,
->definesOption('format')
,->getOption('format')
,->getAllPossibleOptions('format')
? But that might take time to get right, and we shouldn't hold up this issue on it.Comment #172
tedbow@effulgentsia yes there is not coupling except the annotation and contrib style plugin could set this too.
I think moving
getFormatOptions()
toRestExport
makes sense because then we won't have the possibility of breaking existing style plugins like I describe in #169.This patch does that. Then we don't have to do the check for
method_exists('getFormatOptions')
or worry about adding an interface. There is still the problem that a style plugin could be set to workRestExport
and not have public methodgetFormats()
but predates this issue.Comment #174
dawehnerI'm totally cool with the direction we went with. People are super scared of code duplication, because someone told them they should be. In this case though its a minimal amount of code. We could have a simple trait in case anyone cares.
Comment #175
damiankloip CreditAttribution: damiankloip at Acquia commentedI am ok with this direction too. But just to clarify to others, the coupling is not just the methods, but the response from the serializer plugin that is expected in the REST export display too. As discussed with ted, people could totally implement their own plugin, or extend the current Serializer one. It would still be easy either way here.
The only thing is this is now pretty awkward as we have the available list of options coming from the display plugin but the configuration for that stored on the style plugin config. That seems pretty questionable....
Comment #176
tedbowSorry #172 was not exactly what the patch did because I didn't commit all my changes.
I didn't actually move
getFormatOptions()
from Serializer to RestExport but copy it there. So @dawehner's commentAs @dawehner says if we want to remove the duplication we can make
SerializerFormatOptionsTrait
or something.So re @damiankloip's comment here
List of options is actually still coming from Serializer because right now the
getFormatOptions()
is duplicated.This patch fixes the failing test and removes an unused
use
statement.Comment #177
Wim Leers@effulgentsia++ for poking holes in this patch and therefore Views' architecture (hitherto hidden coupling) — there are indeed theoretical BC breaks. An understandable oversight made in the past. I'm glad we're uncovering problems now, and not after it's committed and rolled out to actual sites. Unfortunate we didn't notice this BC problem sooner!
I think @damiankloip summarized it well:
Can't wait for @dawehner & @damiankloip to review this and hopefully get it back to RTBC :)
Comment #178
dawehnerYeah to not just blindly introduce interfaces ...
Comment #179
damiankloip CreditAttribution: damiankloip at Acquia commentednit: This comment doesn't need the reference the style plugin now, I don't think?
Just amended the patch comment, this is a total +1 from me now. Leaving as RTBC.
Comment #180
larowlanAdding credit for @effulgentsia and myself as our reviews changed direction of patch
Comment #182
larowlanCommitted as 4997192 and pushed to 8.5.x.
Comment #183
Wim Leers🎉
Comment #184
Wim LeersThis means REST export views no longer break the HTML view if they're on the same path — I think that's worthy of the release notes…
Comment #185
dawehnerNote: This seemed to have caused issues with all code which relied on not having to specify the format explicitely.
#2916212: Fix rest tests. is one of them.
Comment #186
larowlanShould we roll back?
Comment #187
Wim LeersNo. That test was simply relying on the buggy behavior. This was committed to 8.5.x only, which means contrib has 6 months to fix their tests. That seems reasonable, doesn’t it?
Comment #188
dawehner@Wim Leers
Well, the question is, can we do something in some automated update path? Could we automatically select 'json' as one of the format, given it was the fallback previously as well? I guess we can't given that's the entire point of the issue.
Comment #189
Wim LeersExactly.
Comment #191
xjmWe only use the release notes tag for thing that the site owner needs to know when updating. It's a good bug to have fixed, but I also don't think it has a place on the level of the frontpage post, so not tagging it for the highlights either.
Comment #192
Wim LeersPer #185 and #2937942: REST views: regression in 8.5.x: view with display using 'csv' format now returns a 406 response, need to add ?_format=csv to URL to make it work, this may need a mention in the release notes after all.
Which makes me wonder… shouldn't we create a change record for this?
Comment #193
larowlanYes please to change notice
Comment #194
Wim LeersComment #195
alberto56 CreditAttribution: alberto56 at Dcycle commentedNow that Rest Views no longer specify HTML as a possible request format, getUrlIfValid('/path/to/rest') throws an error. See #2952432: [PP-1] Since 8.5.0, getUrlIfValid('/path/to/rest') will throw an exception if there is no route found for html; how to get a valid Url for a Json path?
Comment #196
Wim LeersLooks like we forgot to create that change record.
Created one: https://www.drupal.org/node/2954953.
Comment #197
slefevre1 CreditAttribution: slefevre1 as a volunteer commentedI just ran into this issue on Drupal 8.5. I don't know if this is the right place for this comment, but in the view display, when it's REST export, it would be helpful to have the
?_format=csv
appende to the PATH SETTINGS Path: field.Comment #198
dawehnerI think the change in
Page.php
causes another regresssion: #2969297: It is not longer possible to override /taxonomy/{}/view with a more specific path without brekaing /taxonomy/{}/edit