Discovered in #2869426-71: EntityResource should add _entity_access requirement to REST routes.

CommentFileSizeAuthor
#5 core-comment_access_messsaging-2950125-5.patch1.22 KBowenbush

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2950127: Add helpful reason for 'update' and 'delete' access not being allowed to FileAccessControlHandler, +#2950129: Add helpful reason for 'update' and 'delete' access not being allowed to MediaAccessControlHandler
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue tags: +dcruhr18
owenbush’s picture

owenbush commented

Looking at this at DrupalCon Nashville.

owenbush’s picture

owenbush commented
StatusFileSize
new core-comment_access_messsaging-2950125-5.patch1.22 KB

I have added messaging to comments for when an update is attempted. There was no 'delete' operation that I could see in the CommentAccessControlHandler class.

owenbush’s picture

owenbush commented
Status: Active » Needs review

Needs review.

Status: Needs review » Needs work

The last submitted patch, 5: core-comment_access_messsaging-2950125-5.patch, failed testing. View results

josephdpurcell’s picture

josephdpurcell commented
Status: Needs work » Active

Yay! Tests are failing! This is good--it means the tests are working as they should.

Take a look at the tests that are failing; resolution should be a matter of updating those failed tests to use the new error message you've created. I hope this helps!

owenbush’s picture

owenbush commented
StatusFileSize
new core-comment_access_messsaging-2950125-9.patch2.15 KB
new interdiff.txt946 bytes

Thank you. I have updated the test case and added an interdiff too.

owenbush’s picture

owenbush commented
Status: Active » Needs review
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

This looks very good, the added message looks solid and we have testcoverage that proves that this works.

josephdpurcell’s picture

josephdpurcell commented
Status: Reviewed & tested by the community » Needs work
josephdpurcell’s picture

josephdpurcell commented
Status: Needs work » Reviewed & tested by the community

Ah! Sorry, wrong click. Setting back to RTBC per #11.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Crediting @Wim Leers for creating this issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -45,7 +45,12 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+        if (!$access_result->isAllowed()) {

I deliberated whether this should be an instanceof check against AccessResultReasonInterface. I think the code is more readable like this.

Committed 2023ce8 and pushed to 8.6.x. Thanks!

  • alexpott committed 2023ce8 on 8.6.x 
    Issue #2950125 by owenbush, Wim Leers: Add helpful reason for 'update'...

Status: Fixed » Closed (fixed)

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