Discovered by @e0ipso in #3014277-78: ResourceTypes should know about their fields. Drupal\jsonapi\ResourceType\ResourceType::updateDeprecatedFieldMapping() (which is the backwards compatibility layer) needs this fix:

-    $is_fieldable = $entity_type_manager->getDefinition($entity_type_manager)->entityClassImplements(FieldableEntityInterface::class);
+    $is_fieldable = $entity_type_manager->getDefinition($entity_type_id)->entityClassImplements(FieldableEntityInterface::class);

@e0ipso already provided a patch, we just need tests.

CommentFileSizeAuthor
#9 3085885-9.patch2.69 KBgabesullice
#9 3085885-9-tests-only.patch1.75 KBgabesullice
#9 interdiff-4-9.txt1.39 KBgabesullice
#4 3085885-3.patch2.72 KBgabesullice
#4 3085885-3-tests-only.patch1.78 KBgabesullice
3014277--error-on-update--78.patch899 bytesgabesullice

Comments

gabesullice created an issue. See original summary.

gabesullice credited e0ipso.

gabesullice’s picture

gabesullice
Primary language English
commented

Reuploading @e0ipso's patch with tests and adding a test only patch too.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 3085885-3-tests-only.patch1.78 KB
new 3085885-3.patch2.72 KB
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Let's fix #3014277-77: ResourceTypes should know about their fields here as well.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Followup to #3014277: Wrong variable used as argument in ResourceType::updateDeprecatedFieldMapping » Follow-up to #3014277: Wrong variable used as argument in BC layer-providing ResourceType::updateDeprecatedFieldMapping()
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +backward compatibility

Test-only patch:

Testing Drupal\Tests\jsonapi\Kernel\ResourceType\ResourceTypeTest
E                                                                   1 / 1 (100%)

Time: 3.04 seconds, Memory: 6.00MB

There was 1 error:

1) Drupal\Tests\jsonapi\Kernel\ResourceType\ResourceTypeTest::testUpdateDeprecatedFieldMapping
Illegal offset type in isset or empty

/Users/wim.leers/Work/d8/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:45
/Users/wim.leers/Work/d8/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php:25
/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/EntityTypeManager.php:143
/Users/wim.leers/Work/d8/core/modules/jsonapi/src/ResourceType/ResourceType.php:504
/Users/wim.leers/Work/d8/core/modules/jsonapi/src/ResourceType/ResourceType.php:348
/Users/wim.leers/Work/d8/core/modules/jsonapi/tests/src/Kernel/ResourceType/ResourceTypeTest.php:41

ERRORS!
Tests: 1, Assertions: 1, Errors: 1.

Process finished with exit code 2

Full patch:

Testing Drupal\Tests\jsonapi\Kernel\ResourceType\ResourceTypeTest
.                                                                   1 / 1 (100%)

Time: 1.59 seconds, Memory: 6.00MB

OK (1 test, 3 assertions)

So: 🚢

gabesullice’s picture

gabesullice
Primary language English
commented

#5: @jibran, I don't think so. From #3014277-84: ResourceTypes should know about their fields

... One is a deprecation on accessing internal properties, the other is a deprecation for constructing a resource type with an outdated argument.

Status: Reviewed & tested by the community » Needs work

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

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new interdiff-4-9.txt1.39 KB
new 3085885-9-tests-only.patch1.75 KB
new 3085885-9.patch2.69 KB 
+++ b/core/modules/jsonapi/tests/src/Kernel/ResourceType/ResourceTypeTest.php
@@ -2,9 +2,9 @@
-use Drupal\Tests\token\Kernel\KernelTestBase;
+use Drupal\KernelTests\KernelTestBase;

Gets me every time.

The last submitted patch, 9: 3085885-9-tests-only.patch, failed testing. View results

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Gets me every time.

glad I'm not the only one

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed 6625869 and pushed to 8.8.x. Thanks!

  • larowlan committed 6625869 on 8.8.x 
    Issue #3085885 by gabesullice, e0ipso, Wim Leers: Follow-up to #3014277...

Status: Fixed » Closed (fixed)

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