#2992833: Add a version negotiation to revisionable resource types added the concept of resource versioning. This changed the constructor of \Drupal\jsonapi\ResourceType\ResourceType(). Which means JSON:API Extras will stop working.

CommentFileSizeAuthor
#23 3025037-23.requires-2.x-dev.patch8.91 KBgabesullice
#23 3025037-23.patch8.25 KBgabesullice
#23 interdiff.txt1.37 KBgabesullice
#20 3025037-20.requires-2.x-dev.patch7.79 KBgabesullice
#20 3025037-20.patch7.13 KBgabesullice
#20 interdiff.txt2.11 KBgabesullice
#16 3025037-16.requires-2.x-dev.patch5.68 KBgabesullice
#16 3025037-16.patch5.03 KBgabesullice
#16 interdiff.txt672 bytesgabesullice
#13 3025037-13.requires-2.x-dev.patch5.33 KBgabesullice
#13 3025037-13.patch5.03 KBgabesullice
#13 interdiff.txt3.23 KBgabesullice
#12 3025037-12.requires-2.x-dev.patch2.1 KBgabesullice
#12 3025037-12.patch5.03 KBgabesullice
#12 interdiff.txt308 bytesgabesullice
#10 3025037-10.patch1.98 KBwim leers
#10 interdiff.txt2.26 KBwim leers
#7 3025037-7.patch3.28 KBwim leers
#7 interdiff.txt870 byteswim leers
#6 3025037-6.patch2.46 KBwim leers
#4 Screen Shot 2019-01-10 at 14.23.01.png328.53 KBwim leers
#2 3025037-2-no_changes_yet_FAIL.patch746 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 3025037-2-no_changes_yet_FAIL.patch746 bytes

Let's verify JSON:API Extras fails against JSON:API HEAD.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Uhm… that passed 😮

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new Screen Shot 2019-01-10 at 14.23.01.png328.53 KB

But it's definitely wrong:

This must mean JSON:API Extras' test coverage is not actually testing what it thinks it's testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Oh, wait:

00:01:39.816   - Installing drupal/jsonapi (2.0.0): Cloning 8.x-2.0

That's why.

If I run the test locally, it fails:

Testing Drupal\Tests\jsonapi_extras\Functional\JsonApiExtrasFunctionalTest
E                                                                   1 / 1 (100%)

Time: 13.71 seconds, Memory: 6.00MB

There was 1 error:

1) Drupal\Tests\jsonapi_extras\Functional\JsonApiExtrasFunctionalTest::testRead
Undefined index: attributes

/Users/wim.leers/Work/d8/modules/jsonapi_extras/tests/src/Functional/JsonApiExtrasFunctionalTest.php:127

ERRORS!
Tests: 1, Assertions: 14, Errors: 1.

Process finished with exit code 2
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 3025037-6.patch2.46 KB

This gets us further:

Testing Drupal\Tests\jsonapi_extras\Functional\JsonApiExtrasFunctionalTest
F                                                                   1 / 1 (100%)

Time: 13.9 seconds, Memory: 6.00MB

There was 1 failure:

1) Drupal\Tests\jsonapi_extras\Functional\JsonApiExtrasFunctionalTest::testRead
Failed asserting that 0 is identical to 61.

/Users/wim.leers/Work/d8/modules/jsonapi_extras/tests/src/Functional/JsonApiExtrasFunctionalTest.php:118

FAILURES!
Tests: 1, Assertions: 12, Failures: 1.

Process finished with exit code 1
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt870 bytes
new 3025037-7.patch3.28 KB

Ugh, that's just because I didn't have e0ipso/shaper installed.

Let's make the test fail explicitly if that library is missing, rather than failing midway through. That's far less confusing. Now it fails like this:

Testing Drupal\Tests\jsonapi_extras\Functional\JsonApiExtrasFunctionalTest
F                                                                   1 / 1 (100%)

Time: 1.6 seconds, Memory: 6.00MB

There was 1 failure:

1) Drupal\Tests\jsonapi_extras\Functional\JsonApiExtrasFunctionalTest::testRead
The e0ipso/shaper library is missing. You can install it with `composer require e0ipso/shaper`.

/Users/wim.leers/Work/d8/modules/jsonapi_extras/tests/src/Functional/JsonApiExtrasFunctionalTest.php:35

FAILURES!
Tests: 1, Assertions: 1, Failures: 1.

And after installing the library:

Testing Drupal\Tests\jsonapi_extras\Functional\JsonApiExtrasFunctionalTest
.                                                                   1 / 1 (100%)

Time: 12.35 seconds, Memory: 6.00MB

OK (1 test, 41 assertions)
gabesullice’s picture

gabesullice
Primary language English
commented

Slack conversation:

gabesullice [8:40 AM]
@e0ipso, have you given thought to how Extras will choose to support JSON:API "minors"

can Extras just track the latest minor of JSON:API?
i.e., when you update to 2.1, you must update to Extras to 3.n
given that it relies so heavily on internals, I think that's a fair thing to ask.

I understood wanting to support 1 & 2 at once, because that meant requiring users to make a major version upgrade if not, I'm not sure that's the case now though

e0ipso [8:43 AM]
@gabesullice J:A E should only support latest minor stable

Which means that the next version of JSON:API Extras will just need to support 2.1 and 2.1 only :D This will make support SOO much simpler.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

This will make support SOO much simpler.

Indeed!

Then I'll update this patch accordingly, and we should then wait to commit this until after JSON:API 2.1 ships.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs work » Postponed
StatusFileSize
new interdiff.txt2.26 KB
new 3025037-10.patch1.98 KB

Done.

gabesullice’s picture

gabesullice
Primary language English
commented
Assigned: Unassigned » gabesullice
Status: Postponed » Active

Queued tests, obviously they'll fail because of a dependency issue. I'll make a patch that updates the required version.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new interdiff.txt308 bytes
new 3025037-12.patch5.03 KB
new 3025037-12.requires-2.x-dev.patch2.1 KB

Here's a version that will test with 2.x-dev. No changes in the other patch, just uploading for readability.

Update: :facepalm: the other patch did include changes that I meant to make next, see following interdiff.

gabesullice’s picture

gabesullice
Primary language English
commented
Title: Make JSON:API Extras compatible with the upcoming 2.1 release » [PP-1] Make JSON:API Extras compatible with the upcoming 2.1 release
Assigned: gabesullice » Unassigned
Status: Needs review » Postponed
StatusFileSize
new interdiff.txt3.23 KB
new 3025037-13.patch5.03 KB
new 3025037-13.requires-2.x-dev.patch5.33 KB

This updates the codebase to use the new CacheableNormalization object added by #3022584: Consolidate and simplify NormalizerValue objects: introduce CacheableNormalization. Postponing until that issue lands.

gabesullice’s picture

gabesullice
Primary language English
commented
Title: [PP-1] Make JSON:API Extras compatible with the upcoming 2.1 release » Make JSON:API Extras compatible with the upcoming 2.1 release
Status: Postponed » Needs review

#3022584: Consolidate and simplify NormalizerValue objects: introduce CacheableNormalization just landed.

Status: Needs review » Needs work

The last submitted patch, 13: 3025037-13.requires-2.x-dev.patch, failed testing. View results

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt672 bytes
new 3025037-16.patch5.03 KB
new 3025037-16.requires-2.x-dev.patch5.68 KB

Hm, looks like I'm not getting the composer thing right.

The last submitted patch, 16: 3025037-16.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 16: 3025037-16.requires-2.x-dev.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Rather than depending on JSON:API Extras' tests running on DrupalCI: are the JSON:API Extras tests green when running them locally?

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.11 KB
new 3025037-20.patch7.13 KB
new 3025037-20.requires-2.x-dev.patch7.79 KB

Ain't nobody got time for that! :P

Jokes aside, we're going to need a version of the 2.x-dev patch regardless. That's because we'll need to require ^2.1 instead of 2.x-dev, that tag just doesn't exist yet. Also, @e0ipso wanted a more automated way to test this stuff, being able to just click the "retest" link after each commit gets us closer to that.

The last submitted patch, 20: 3025037-20.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 20: 3025037-20.requires-2.x-dev.patch, failed testing. View results

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.37 KB
new 3025037-23.patch8.25 KB
new 3025037-23.requires-2.x-dev.patch8.91 KB

This should be back to green.

The last submitted patch, 23: 3025037-23.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Looks great! I think this is ready for an @e0ipso review :)

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Assigned: Unassigned » e0ipso

  • e0ipso committed 78fa722 on 8.x-3.x authored by gabesullice 
    Issue #3025037 by gabesullice, Wim Leers: Make JSON:API Extras...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Reviewed & tested by the community » Fixed

This was merged. It will break HEAD unless you're also running JSON:API HEAD. This will resolve itself on Monday when we tag releases for both modules.

Status: Fixed » Closed (fixed)

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