// Delete responses have an empty body.
return new ModifiedResourceResponse(NULL, 204);
CommentFileSizeAuthor
#26 fix_issue-2815063-26.patch743 bytesleeotzu
#22 fix_issue-2815063-22.patch743 bytesleeotzu
#19 fix_issue-2815063-19.patch745 bytesyogeshmpawar
#17 fix-issue-2815063-17.patch739 bytesshruti1803
#14 fix-issue-2815063-14.patch739 byteslomasr
#2 fix-issue-2815063-2.patch743 bytesminakshiPh

Comments

Chi created an issue. See original summary.

minakshiPh’s picture

minakshiPh commented
Status: Active » Needs review
StatusFileSize
new fix-issue-2815063-2.patch743 bytes

Added the patch with fix.

Kindly review.
Thanks!

chi’s picture

chi commented
Issue tags: +english

Well, I just realized that this might not be a typo though it sounds strange. "Delete" stands for HTTP method name in this context. I would like someone with more English experience to review this.

wim leers’s picture

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

Yep, this makes sense. Thanks!

chi’s picture

chi commented

I would like to emphasize that we are not deleting anything in that line. :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 5233c3d to 8.3.x and 7a2da64 to 8.2.x. Thanks!

  • alexpott committed 5233c3d on 8.3.x 
    Issue #2815063 by minakshiPh: Typo in EntityResource::delete()

  • alexpott committed 7a2da64 on 8.2.x 
    Issue #2815063 by minakshiPh: Typo in EntityResource::delete()

(cherry...
chi’s picture

chi commented
Status: Fixed » Needs work

Still thinking the spelling is wrong. Delete is not a verb here. It is kind of adjective describing the response method (GET, POST, OPTIONS, etc). We are not deleting responses here but only noting why this response has an empty body. I propose using uppercase for the method name like follows: "DELETE responses have an empty body."

Let me know if I am mistaken.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +needs revert

Ugh, you're totally right. I don't know how I missed that :(

It should just have been s/Delete/DELETE/

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -needs revert

Let's just open an new issue for that. The bit that was changed was not the delete part. Reverting is totally OTT for this.

wim leers’s picture

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

Ehm, no, the original sentence made sense. The new sense makes no sense at all. Again, my bad :(

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -263,7 +263,7 @@ public function delete(EntityInterface $entity) {
-      // Delete responses have an empty body.
+      // Delete responses which have an empty body.
       return new ModifiedResourceResponse(NULL, 204);

"DELETE responses" have an empty body -> makes sense!

"Delete responses which have an empty body" -> makes no sense, because this code is not deleting any responses.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: fix-issue-2815063-2.patch, failed testing.

lomasr’s picture

lomasr commented
StatusFileSize
new fix-issue-2815063-14.patch739 bytes

I agree with Wim. "DELETE responses" have an empty body is a better idea . Added a patch.

shruti1803’s picture

shruti1803 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: fix-issue-2815063-14.patch, failed testing.

shruti1803’s picture

shruti1803 commented
Status: Needs work » Needs review
StatusFileSize
new fix-issue-2815063-17.patch739 bytes

Status: Needs review » Needs work

The last submitted patch, 17: fix-issue-2815063-17.patch, failed testing.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review
StatusFileSize
new fix_issue-2815063-19.patch745 bytes

I have rerolled the patch against 8.3.x because #17 failed to apply.

chi’s picture

chi commented
Issue summary: View changes

I would not use quotes in this case for the purpose of consistency. We are usually not using them in this context. See EntityResource::delete method documentation as an example.

chi’s picture

chi commented
Issue summary: View changes
leeotzu’s picture

leeotzu commented
StatusFileSize
new fix_issue-2815063-22.patch743 bytes

Changes applied as per suggestion from @chi for 8.3.x

leeotzu’s picture

leeotzu commented
chi’s picture

chi commented

The method name can remain in upper case.

Grayside’s picture

Grayside commented

All caps for HTTP method names are standard practice in formal tech writing.

leeotzu’s picture

leeotzu commented
StatusFileSize
new fix_issue-2815063-26.patch743 bytes

Updated the method name to uppercase.

chi’s picture

chi commented
Status: Needs review » Reviewed & tested by the community

  • catch committed 1f1f20f on 8.3.x 
    Issue #2815063 by leeotzu, shruti1803, minakshiPh, lomasr, Yogesh Pawar...

  • catch committed 5ea0495 on 8.2.x 
    Issue #2815063 by leeotzu, shruti1803, minakshiPh, lomasr, Yogesh Pawar...
catch’s picture

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

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

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