Closed (fixed)
Project:
Drupal core
Version:
8.4.x-dev
Component:
rest.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
16 Jan 2017 at 06:14 UTC
Updated:
26 Sep 2017 at 21:55 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
sumanthkumarc commentedComment #4
sumanthkumarc commentedComment #5
sumanthkumarc commentedComment #6
benelori commentedComment #7
wim leers@benelori: thanks for taking this on! :)
Comment #8
naveenvalechaAssigning to myself to take this up.
Comment #9
wim leersThanks @naveenvalecha!
Comment #10
wim leersComment #12
wim leersHere's the needed test coverage.
Comment #13
wim leersTurns out that
\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::getEntityResourceUrl()is inadequate, and this has gone unnoticed for a long time.Turns out
ContactFormis the only@ConfigEntityTypein Drupal core that specifies acanonicallink template. This is why we're only finding this now!That method does this:
Simple enough, right? But …
ConfigEntityBaseoverrides thetoUrl()method:Which means that we were getting the
edit-formURL instead of thecanonicalURL, even when we know a canonical URL exists. Fortunately, this is easy to fix.Comment #14
wim leersThe above patch almost passes — the only thing that's missing is the expected helpful messages for 403 responses. Fortunately, this too is trivial fix 😀
Comment #16
wim leersAdded HAL+JSON test coverage.
Now ready for final review.
Comment #18
Anonymous (not verified) commentedReally nice correction! 💎
all other just nice ;)
https://www.drupal.org/pift-ci-job/753333 - it seems CI also really likes this patch <3333
Comment #19
larowlanI'd like to see some coverage around the recipients field, particularly for the multi-value instance (comma separated). But that can be in the existing follow-up. It's a bit of a snowflake in so far as we're cramming multiple values into a single field.
I think a comment here to explain the findings in #13 would help - and probably prevent someone thinking that the 'canonical' argument was superfluous. I thought it was at first. Knocking back to needs work for that (docs gate), but will keep an eye out for when this gets back to RTBC so it doesn't sink to bottom of list.
Comment #20
wim leers@ContactFormentity form. When you save that form, it stores them as a list of strings. (I tested manually to verify.) Test coverage added!Comment #23
larowlanCommitted as a87e8aa and pushed to 8.5.x.
Cherrypicked as 3d0abda and pushed to 8.4.x.
Thanks @WimLeers et al