GraphQL 4: Upgrade core modules

[unclecheese]

To implement

• snapshot-admin - GraphQL v4 compat silverstripe-versioned-snapshots#40

To merge

• graphql
• framework - changelog entry
• admin
• CMS
• asset-admin
• versioned
• versioned-admin
• elemental

Backward compatibility

These upgrades were done without attention to backward compatibility. This shouldn't have happened. The groundwork for BC had been established (graphql-legacy.yml, and writing data-graphql-legacy to the document for runtime client side compat). We'll need to revisit the upgrades for asset-admin and elemental.

Assuming changing the admin graphql schema in GraphQL 3 is not an API change, this should be pretty straightforward.

Tasks:

• Rewrite GraphQL 3 queries to have the same args, fields, etc as GraphQL 4 does / will. (CaseInsensitiveFieldAccessor will help)
• Implement a solution for including/excluding unit tests based on GraphQL version
• Backport GraphQL 3 unit tests
• Update travis config to test GraphQL 3 and GraphQL 4 (e.g. put all the monkey patching behind an env var condition)
• Regression test with GraphQL v3
• Regression test with GraphQL v4

Setup

See .travis.yml files for new required forks.

Build status (with backward compat)

✅ silverstripe/admin https://travis-ci.com/github/silverstripe/silverstripe-admin

✅ silverstripe/asset-admin https://travis-ci.org/github/unclecheese/silverstripe-asset-admin/builds/741754720

✅ silverstripe/cms https://travis-ci.org/github/unclecheese/silverstripe-cms

✅ silverstripe/versioned https://travis-ci.org/github/unclecheese/silverstripe-versioned/builds/741841567

✅ silverstripe/versioned-admin https://travis-ci.com/github/unclecheese/silverstripe-versioned-admin/jobs/429439571 (behat)

✅ dnadesign/silverstripe-elemental https://travis-ci.org/github/dnadesign/silverstripe-elemental/builds/741743836

[unclecheese]

Asset-admin is green: https://travis-ci.org/github/unclecheese/silverstripe-asset-admin/builds/736198735

[unclecheese]

Elemental is green https://travis-ci.org/github/unclecheese/silverstripe-elemental/builds/737026167

[unclecheese]

CMS is green https://travis-ci.org/github/unclecheese/silverstripe-cms

[chillu]

@unclecheese Can you please ensure that there's builds for both 3.x and 4.x variations? Technically doesn't require all tests to run, but specifically the Behat tests on CMS, Elemental and AssetAdmin would be useful to test across both, right?

[unclecheese]

No, I don't think that's the case. When these PRs get merged, the CMS recipe will require GraphQL 4, right?

EDIT: have addressed this in the original issue.

[unclecheese]

We have a new PR that should go into the next minor release of GraphQL / recipe-cms: #310. This PR adds a new API that allows customisation of the field name formatting (e.g. casing), and allows the global use of CaseInsensitiveFieldAccessor.

The reason for this is that in doing the upgrades, it became clear that the easiest path for backward compatibility was to make the GraphQL 3 queries look like they will in GraphQL 4, rather than try to maintain two versions of the same query (which often wasn't an option due to assignment at build time). Since changing the admin GraphQL schema is not an API change, these changes will be suitable for a minor release.

All the above modules have had the BC strategy implemented, tests backported, and backward compatibility tests added to the Travis matrix. Most of them are still showing failures that I'm still working through.

[chillu]

I think we should mark all the legacy PHP classes (e.g. SilverStripe\AssetAdmin\GraphQL\ReadFileQueryResolver) as @deprecated to discourage their direct usage, with a note saying "consider upgrading to GraphQL v4"

[unclecheese]

All the core modules, except for graphql itself, are passing with backward compatibility. This card is ready for review.

[chillu]

Are you still working on frameworktest? If its more than a day of work, we could make a new major release line for GraphQL v4 compat there? A bit of double maintenance going forward on the two release lines, but there's really no semver commitments on that module.

[unclecheese]

Sorry, frameworktest was merged a while ago. silverstripe/silverstripe-frameworktest@0302087

[chillu]

Do you think we should mark all PHP classes in the _legacy folders as @deprecated, and point to their new implementations? If you're just going through an IDE without the file paths in your face, it's quite easy to get confused - e.g. SilverStripe\GraphQL\Resolvers\ApplyVersionFilters is legacy (v3), but SilverStripe\GraphQL\Resolvers\VersionFilters isn't (v4). Due to semver, we can't change the class name of the former, so we need to make this clear via other means (PHPDoc)

[unclecheese]

Yeah, probably a good idea. I'd also like to figure out a way to hide all the generated code from the IDE, as well. (Try searching for Page when you have four schemas)

[chillu]

I've written up a 4.8.0 changelog entry to avoid confusion for early user of recipe 4.x-dev: silverstripe/silverstripe-framework#9762. We can flesh this out with a proper GraphQL v4 pre-release announcement (coinciding with 4.0.0-alpha1), but unlike the basic changelog entry that's not a blocker for merging the core PRs.

[chillu]

I see you've used classmap in asset-admin, but not in the other modules - was that on purpose? The _legacy/ folders are throwing warnings on composer install, e.g.

Class SilverStripe\Versioned\Tests\GraphQL\Legacy\Plugins\VersionedReadTest located in ./tests/php/GraphQL/Plugins/VersionedReadTest.php does not comply with psr-4 autoloading standard. Skipping.

[unclecheese]

No, just haven't cleaned it up yet. They were all classmap at one point and then got migrated to the array syntax.

Unfortunately, it seems like even the array syntax won't hold up in Composer 2. It's complaining in either configuration.

[chillu]

All merged, there's a few follow up issues emerging from this though:

silverstripe/silverstripe-versioned-snapshots#40
#320
#317
#321