Assertion fails when uploading a file to a file field that is a BaseFieldDefinition.
assert(is_null($entity) || $field_definition->getTargetEntityTypeId() === $entity->getEntityTypeId() && $field_definition->getTargetBundle() === $entity->bundle());
This assertion is incorrect because base fields never have a bundle but entities that are not "bundleable" default to returning the entity type form $entity->bundle().
Not sure on the best solution, we could check if $field_definition is an instance of BaseFieldDefinition and then not do that assertion?
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3048453-7.patch | 1.7 KB | benjy |
| #2 | 3048453-2.patch | 1.59 KB | benjy |
Comments
Comment #2
benjy commentedProposed patch.
Comment #3
wim leersWow, great catch! I'm pretty sure this is going to be a bug in REST's
\Drupal\file\Plugin\rest\resource\FileUploadResourcetoo. Nice example of wrong assumptions!We'll need test coverage for this;
\Drupal\Tests\jsonapi\Functional\FileUploadTestis the best fit I think. Core doesn't have an entity type with a@FieldType=fileas a base field, so I think we'll need to create that, much like\Drupal\entity_test\Entity\EntityTestMapField.Note that this fix will first need to land in Drupal core since JSON:API has been committed to Drupal 8.7 core. But to have a faster feedback cycle, I propose we first develop a patch here, where test runs take only a few minutes rather than an hour. I'd be happy to reroll the patch to apply to Drupal core later. Once it is committed to Drupal core, we'll backport it to the contrib module for sites on Drupal 8.5 or 8.6.
Comment #4
wim leersOh, and the change itself looks good to me! Marking for test coverage.
Comment #5
benjy commentedInteresting point here, it would be cool if you could run Drupal core tests for only the selected module, many changes, like this are so isolated you wouldn't want to run the full suite until you were pretty much ready to commit the change.
Comment #6
wim leersYes, that would be cool :) You actually can nowadays: you’d just need to also patch the
drupalci.ymlfile!Comment #7
benjy commentedHere's the core version of the patch.