Closed (fixed)
Project:
Drupal core
Version:
8.9.x-dev
Component:
jsonapi.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
11 Oct 2017 at 17:41 UTC
Updated:
3 Apr 2020 at 21:57 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
wim leers#2795109: [BUGFIX] Support configuration entities with array values explicitly mentions
_core. We should look into that issue, investigate whether some of the changes made there need to be re-evaluated.Comment #3
wim leersComment #4
wim leersThis is also related to #2860350: Document why JSON API only supports @DataType-level normalizers.
Comment #5
wim leers#2915414: Omit "_core" key from normalized config entities, which includes the default config hash landed in core, time to mirror this behavior in JSON API!
Comment #6
wim leersJust patch for now, AFAICT JSON API has no test coverage for config entities yet…
Comment #7
wim leersComment #8
wim leersRelevant issue for the GraphQL module: https://github.com/drupal-graphql/graphql/issues/64
Comment #9
e0ipsoSee ConfigEntityNormalizerTest.php
Comment #10
wim leersI meant no functional test coverage. We added functional test coverage for this in core at
\Drupal\Tests\rest\Functional\EntityResource\ConfigurableLanguage\ConfigurableLanguageResourceTestBase::testGetDefaultConfig().We'll also need denormalization test coverage, which core only did via a unit test (
\Drupal\Tests\serialization\Unit\Normalizer\ConfigEntityNormalizerTest::testDenormalize()). Only a unit test, because core doesn't yet have full support for config entity denormalization.Comment #11
e0ipsoI am OK having duplicated test coverage. The more the better. However, let's call it duplicated test coverage instead of missing test coverage. I think that way we can send the correct message and avoid some people to think that this module is untested.
Comment #12
wim leersIt's easy to have unit tests that you think are testing the right thing, but actually aren't. We've all seen that happen I think.
That's why it's important to have functional tests for at least the common cases. It's okay for them to be testing fewer edge cases than unit tests. So I do think is the appropriate word.
Comment #13
e0ipsoIf you feel strongly about that I'm OK with it. 😊
Comment #14
wim leersWell, we might now as well block this on #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only. Then we'll be able to add explicit integration test coverage that's similar to #2915414: Omit "_core" key from normalized config entities, which includes the default config hash.
Comment #15
wim leers#2932031-32: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases is adding test coverage for this already, but for the wrong behavior in HEAD (based on the test coverage that #2915414: Omit "_core" key from normalized config entities, which includes the default config hash added to core). That means that this issue will simply be able to change one test assertion and we'll have the desired integration test coverage :)
Keeping at "PP-1" despite #2930028 having landed, because this is now blocked on #2932031.
Comment #16
wim leers#2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases landed!
Comment #17
wim leersPer #15, test coverage is now trivial to add :)
Comment #18
gabesulliceComment #20
e0ipsoThanks all!
Comment #22
rpayanmMoving to Drupal core's issue queue.
I'm working on https://www.drupal.org/project/drupal/issues/3122113