Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
#2124677: Expose URI in file fields when serializing an object added this test, and includes this in its setUp() function:
// Set up the serializer to return an entity property.
$this->serializer->normalize(Argument::cetera())
->willReturn(['value' => 'test']);
This will be called for normalizing the target_id property on EntityReferenceItems. This is an inaccurate mock (simulation) of the real world.
Consequently, when other patches land that make changes elsewhere in the system (such as #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API making changes to ComplexDataNormalizer::normalize()), that results in false negatives (incorrect test failures).
Proposed resolution
Fix the unit test to simulate the real world accurately.
The reason you can be absolutely certain that the normalized value for an entity reference field's target_id property should be
'test'
and not
['value' => 'test']
can be found in the many functional/integration tests that we have by now. For example, from NodeResourceTestBase:
'type' => [
[
'target_id' => 'camelids',
'target_type' => 'node_type',
'target_uuid' => NodeType::load('camelids')->uuid(),
],
],
…
'uid' => [
[
'target_id' => (int) $author->id(),
'target_type' => 'user',
'target_uuid' => $author->uuid(),
'url' => base_path() . 'user/' . $author->id(),
],
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 2927566-3.patch | 3.65 KB | Wim Leers |
| #8 | 2927566-3-mock-only-FAIL.patch | 808 bytes | Wim Leers |
Comments
Comment #2
Wim LeersThis fixes the incorrect mock that got in the way of #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API.
Comment #3
Wim LeersAnd this fixes all the others too.
The fact that I can change only some, not all, and tests still pass, mean that #2124677: Expose URI in file fields when serializing an object introduced a weak/broken test. It's because we're mocking far too much here.
Also, the reason you can be absolutely certain that the normalized value for an entity reference field's
target_idproperty should beand not
can be found in the many functional/integration tests that we have by now. For example, from
NodeResourceTestBase:… which of course makes us question the value of this unit test in the first place. But dealing with that is out of scope for this issue. This is about fixing the immediate, blocking problem.
Comment #4
Wim LeersFix-mock-only patch that should fail, plus patch that will be green.
Comment #8
Wim LeersApparently the "fail" patch in #4 was missing one newline. That's what I get for manually editing patches.
Comment #11
Wim LeersThe patch in #8 that was supposed to pass tests failed due to DrupalCI infra problems:
Retesting. Besides, the same exact patch was green in #4.
Comment #12
borisson_This looks great as a simplification of that test.
Comment #14
Wim LeersComment #15
webchickOopsie. :P Good catch.
Committed and pushed to 8.5.x. Thanks!
Comment #17
Wim LeersYay, @mpdonadio already rebased #2926508-41: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API on top of this!