Problem/Motivation

#2800873: Add XML GET REST test coverage, work around XML encoder quirks introduced a new test field: field_rest_test_multivalue. This is fine.

Unfortunately, it did not get the same special treatment as field_rest_test, which got this:

      case 'view':
        // Never ever allow this field to be viewed: this lets EntityResourceTestBase::testGet() test in a "vanilla" way.
        return AccessResult::forbidden();

This prevented that field from showing up in normalizations, which meant that each entity type could still write its own GET test coverage without it needing to be aware of this automatically added field (that is used for testing certain edge cases).

Proposed resolution

Also disallow view access for the field_rest_test_multivalue field.

Remaining tasks

Patch.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#7 2928702-7.patch6.59 KBwim leers
#7 interdiff.txt1.48 KBwim leers
#5 2928702-5.patch6.32 KBwim leers
#3 2928702-3.patch6.41 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Major
Issue tags: +blocker
Related issues: +#2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field

This is already negatively impacting other patches/issues, see #2825487-155: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2928702-3.patch6.41 KB

Status: Needs review » Needs work

The last submitted patch, 3: 2928702-3.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2928702-5.patch6.32 KB

#3 was on top of another patch. My bad. Rebased, unchanged.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work

Nitpicks:

  1. +++ b/core/modules/rest/tests/modules/rest_test/rest_test.module
@@ -27,6 +28,16 @@ function rest_test_entity_field_access($operation, FieldDefinitionInterface $fie
+    switch ($operation) {
+      case 'view':

    I don't understand why this is a switch, this can be a simple if instead. 

  2. +++ b/core/modules/rest/tests/modules/rest_test/rest_test.module
@@ -27,6 +28,16 @@ function rest_test_entity_field_access($operation, FieldDefinitionInterface $fie
+        // Never ever allow this field to be viewed: this lets EntityResourceTestBase::testGet() test in a "vanilla" way.

    80 cols.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.48 KB
new 2928702-7.patch6.59 KB

Thanks for the review!

  1. Consistency with the code just before it.
  2. Fixed. Also fixed the occurrence of the exact same comment in the code just before it.
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration

See #2543726-222: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration, that issue would also benefit by this.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

@Wim Leers looks good. This will make other tests easier to understand!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2928702-7.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Reviewed & tested by the community

Retesting because #7 got

exception: [Warning] Line 179 of core/lib/Drupal/Core/Cache/ApcuBackend.php:
apcu_store(): Unable to allocate memory for pool.

Errors, so pretty sure unrelated DrupalCI errors.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2928702-7.patch, failed testing. View results

borisson_’s picture

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

Same error as in #11.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2928702-7.patch, failed testing. View results

wim leers’s picture

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

This is maddening.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Very nice clean-up, and simply applying patterns from elsewhere in core to here.

Committed and pushed to 8.5.x. Thanks!

  • webchick committed ee22a47 on 8.5.x 
    Issue #2928702 by Wim Leers, borisson_, tedbow: Make...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yay, was able to do the following rerolls thanks to this:

  1. #2825487-174: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field
  2. #2543726-245: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration

Status: Fixed » Closed (fixed)

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