Problem/Motivation

The typehint for the $deserialization_target_class parameter of \Drupal\jsonapi\ResourceType\ResourceType::__construct, and the associated property (ResourceType::$deserializationTargetClass) and accessor (ResourceType::getDeserializationTargetClass) is string, but there are several tests passing NULL instead.

Steps to reproduce

$ ./vendor/bin/phpstan analyse -c core/phpstan.neon.dist --level=5 --error-format=raw | grep deserialization_target_class | sed "s|$(pwd)/||g"
 10896/10896 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

core/modules/jsonapi/tests/src/Kernel/Controller/EntityResourceTest.php:241:Parameter #3 $deserialization_target_class of class Drupal\jsonapi\ResourceType\ResourceType constructor expects string, null given.
core/modules/jsonapi/tests/src/Kernel/Query/FilterTest.php:337:Parameter #3 $deserialization_target_class of class Drupal\jsonapi\ResourceType\ResourceType constructor expects string, null given.
core/modules/jsonapi/tests/src/Kernel/Query/FilterTest.php:406:Parameter #3 $deserialization_target_class of class Drupal\jsonapi\ResourceType\ResourceType constructor expects string, null given.
core/modules/jsonapi/tests/src/Unit/EventSubscriber/ResourceResponseValidatorTest.php:84:Parameter #3 $deserialization_target_class of class Drupal\jsonapi\ResourceType\ResourceType constructor expects string, null given.
core/modules/jsonapi/tests/src/Unit/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php:50:Parameter #3 $deserialization_target_class of class Drupal\jsonapi\ResourceType\ResourceType constructor expects string, null given.
core/modules/jsonapi/tests/src/Unit/Normalizer/ResourceIdentifierNormalizerTest.php:56:Parameter #3 $deserialization_target_class of class Drupal\jsonapi\ResourceType\ResourceType constructor expects string, null given.
core/modules/jsonapi/tests/src/Unit/Normalizer/ResourceIdentifierNormalizerTest.php:61:Parameter #3 $deserialization_target_class of class Drupal\jsonapi\ResourceType\ResourceType constructor expects string, null given.

Proposed resolution

  • Change typehints to class-string
  • Pass Node::class where appropriate, and \stdClass::class everywhere else

Remaining tasks

FWIW, this property and accessor don't appear to be used in core, but it is in contrib. Is this something core should deprecate?

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3557053

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mstrelan created an issue. See original summary.

mstrelan opened merge request !13744

bbrala’s picture

bbrala
Location Netherlands
commented

Did you mean to "needs review" this?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Core does use this from ResourceTypeRepository:

    return new ResourceType(
      $entity_type->id(),
      $bundle,
      $entity_type->getClass(),
      ...

and then it's read and used in JsonApiDocumentTopLevelNormalizer:

    // Override deserialization target class with the one in the ResourceType.
    $class = $context['resource_type']->getDeserializationTargetClass();

Agree that it shouldn't be nullable, although does it make sense for null to fall back to \stdClass?

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Active » Needs review

Did you mean to "needs review" this?

Yes, I was waiting for a green pipeline and then forgot to come back to it.

Core does use this from ResourceTypeRepository

Thanks for pointing that out, I was relying on usages detected by phpstorm but should have grepped for it. You first example is just setting the value, but my point was that I didn't think it was ever accessed, which was wrong. It is also accessed in ResourceObjectNormalizer::serializeField, but with a todo comment to remove it.

Agree that it shouldn't be nullable, although does it make sense for null to fall back to \stdClass?

I think it's probably good to force an explicit decision to be made.

I think we could probably also update the typehint in \Drupal\jsonapi\Serializer\Serializer::selfSupportsDenormalization to expect class-string in this issue as well.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Let's deprecate passing null then we can add a typehint to the argument in D12. I don't see this being widely used so no point leaving it until D13 if we can help it.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Deprecated passing null, added a CR.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Reviewed & tested by the community

This is looking good. Weird commit with the phpcs change (and undo), but you probably had your reasons.

Feedback was adressed and good to clean up ^^

  • catch committed 835b45ed on 11.x 
    task: #3557053 Stop passing null as

By: mstrelan
By: bbrala
By:...
catch’s picture

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

Updated the deprecated version to 11.3.0 on commit, this isn't real API surface so seems fine to remove in 12.0.0

Committed/pushed to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !13744

  • catch committed bd8e5e58 on 11.x 
    task: #3557053 Stop passing null as $deserialization_target_class

By:...

  • catch committed e33e4213 on 11.x 
    Revert "task: #3557053 Stop passing null as"

This reverts commit...

Status: Fixed » Closed (fixed)

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