Problem/Motivation

First reported for Drupal core's REST module, see #2915414: Omit "_core" key from normalized config entities, which includes the default config hash. Also applies to JSON API. Let's fix this in JSON API after core fixes it.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

#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.

wim leers’s picture

Status: Active » Postponed
wim leers’s picture

wim leers’s picture

Title: [PP-1] Omit "_core" key from normalized config entities, which includes the default config hash » Omit "_core" key from normalized config entities, which includes the default config hash
Status: Postponed » Active
wim leers’s picture

Status: Active » Needs review
StatusFileSize
new1.71 KB

Just patch for now, AFAICT JSON API has no test coverage for config entities yet…

wim leers’s picture

Status: Needs review » Needs work
wim leers’s picture

Relevant issue for the GraphQL module: https://github.com/drupal-graphql/graphql/issues/64

e0ipso’s picture

AFAICT JSON API has no test coverage for config entities yet…

See ConfigEntityNormalizerTest.php

wim leers’s picture

I 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.

e0ipso’s picture

I 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.

wim leers’s picture

It'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 missing is the appropriate word.

e0ipso’s picture

If you feel strongly about that I'm OK with it. 😊

wim leers’s picture

Title: Omit "_core" key from normalized config entities, which includes the default config hash » [PP-1] Omit "_core" key from normalized config entities, which includes the default config hash
Status: Needs work » Postponed
Related issues: +#2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only
wim leers’s picture

#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.

wim leers’s picture

Title: [PP-1] Omit "_core" key from normalized config entities, which includes the default config hash » Omit "_core" key from normalized config entities, which includes the default config hash
Assigned: Unassigned » wim leers
Status: Postponed » Active
wim leers’s picture

Assigned: wim leers » Unassigned
Status: Active » Needs review
Issue tags: -Needs tests
StatusFileSize
new829 bytes
new2.48 KB

Per #15, test coverage is now trivial to add :)

gabesullice’s picture

Status: Needs review » Reviewed & tested by the community

  • e0ipso committed 1f0f3f2 on 8.x-1.x authored by Wim Leers
    Issue #2915539 by Wim Leers, e0ipso, gabesullice: Omit "_core" key from...
e0ipso’s picture

Status: Reviewed & tested by the community » Fixed

Thanks all!

Status: Fixed » Closed (fixed)

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

rpayanm’s picture

Project: JSON:API » Drupal core
Version: 8.x-1.x-dev » 8.9.x-dev
Component: Code » jsonapi.module

Moving to Drupal core's issue queue.

I'm working on https://www.drupal.org/project/drupal/issues/3122113