See #2930028-61: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.

Nutshell: rest_test_entity_field_access() returns access results without reasons, due to bugs in \Drupal\Core\Access\AccessResult::orIf(), the reasonless access results would "infect" the access results with reasons, resulting in the reason being omitted. #2938053: AccessResult::orIf() fails to retain the reason if both operands are neutral or forbidden, but the first contains a reason and the second one does not fixed that.

This means JSON API's test coverage now can be updated. In fact, it must, because it'll fail against latest 8.5 + 8.6.

CommentFileSizeAuthor
#9 2954847-9.patch3.28 KBwim leers
#9 interdiff-6-9.txt1.08 KBwim leers
#6 interdiff-4-6.txt853 byteswim leers
#6 2954847-6.patch2.23 KBwim leers
#4 2954847-4.patch2.75 KBwim leers
#2 2954847-2-no_changes_just_trigger_test_run_8.6.patch501 byteswim leers
#2 2954847-2-no_changes_just_trigger_test_run_8.5.patch501 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2954847-2-no_changes_just_trigger_test_run_8.5.patch501 bytes
new 2954847-2-no_changes_just_trigger_test_run_8.6.patch501 bytes

The last submitted patch, 2: 2954847-2-no_changes_just_trigger_test_run_8.5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 2954847-4.patch2.75 KB

Great, those failed as expected! That proves HEAD is failing now, since #2938053: AccessResult::orIf() fails to retain the reason if both operands are neutral or forbidden, but the first contains a reason and the second one does not was committed to 8.5.x/8.6.x earlier today.

Now let's update the expectations, and then this should be green again!

Status: Needs review » Needs work

The last submitted patch, 4: 2954847-4.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2954847-6.patch2.23 KB
new interdiff-4-6.txt853 bytes 
+++ b/tests/src/Functional/ResourceTestBase.php
@@ -1507,7 +1507,7 @@ abstract class ResourceTestBase extends BrowserTestBase {
-          'detail' => "The current user is not allowed to PATCH the selected field ($id_field_name). The entity ID cannot be changed",
+          'detail' => "The current user is not allowed to PATCH the selected field ($id_field_name). The entity ID cannot be changed.",

This change should be reverted. We'll need to make this change after #2938035: When PATCHing a field is disallowed, no reason is given for *why* this happens lands.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

This should be green and RTBC'able.

Status: Needs review » Needs work

The last submitted patch, 6: 2954847-6.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-6-9.txt1.08 KB
new 2954847-9.patch3.28 KB

Obviously I was wrong :)

wim leers’s picture

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

Yay, green!

Self-RTBC'ing because this fixes broken tests against 8.5.x HEAD and 8.6.x HEAD.

wim leers’s picture

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

Waited for tests to pass on both 8.5 and 8.6. Committed!

  • Wim Leers committed 9759699 on 8.x-1.x 
    Issue #2954847 by Wim Leers: Core bug #2938053 fixed, now JSON API's...

Status: Fixed » Closed (fixed)

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