Remove GraphQL from the CMS

emteknetnz commented 2 months ago

GraphQL is used inconsistently in the CMS for some of the XHR requests made by react components. The majority of XHR requests are currently made to "regular Silverstripe controller endpoints", which also includes "Form Schema" endpoints used to process Form requests.

This lack of standardisation has lead to inefficiencies in development an maintenance for the CMS Squad. There has also been a fair bit of negative feedback from several developers in the community who think they're supposed to do all admin XHR requests using GraphQL, which has proven to be very difficult for people to learn and use.

This card is about removing GraphQL from the CMS entirely and be replaced with "regular Silverstripe controller endpoints" in the name of standardisation.

Removing GraphQL from the CMS also means that projects no longer need to have the .graphql-generated directory by default.

Acceptance criteria

[x] Functionality that GraphQL enabled is retained and replaced with regular Silverstripe controller endpoints
[x] Overall test coverage is not reduced
[x] All GraphQL code and config is removed (excluding the actual silverstripe/graphql module)
[x] GraphQL dependency is not included in any commericially supported composer.json files
[x] The .graphql-generated folder is not created when creating a project with silverstripe/installer ~~or silverstripe/recipe-kitchen-sink~~ (will probably still be created for recipe-sink because we're including the graphql module)
[x] silverstripe/graphql is still included in silverstripe/recipe-kitchen-sink
[x] PRs target CMS 6
[x] Removed classes and methods are deprecated in CMS 5
[ ] In docs.silverstripe.org, the "Developer Guides > GraphQL" section is moved to "Optional features" (have raised new card for this)
[x] Any other relevant documentation referring to graphql in docs.silverstripe.org is updated accordingly
[ ] Create follow up PRs to use the correct deprecation version in CMS 5 e.g. for asset-admin it should be deprecated in 2.3.0, not 5.3.0. After merging ensure merge-up is done correctly as the deprecated methods will have been removed

New issues created

https://github.com/silverstripe/doc.silverstripe.org/issues/304

CMS 5 PRs

CMS 6 multi PR CI

https://github.com/creative-commoners/recipe-kitchen-sink/actions/runs/10627272875

CMS 6 PRs

Other PRs

Notes

This refers exclusively to GraphQL endpoints inside the CMS
Commercial support for the silverstripe/graphql module is not being changed at this point

michalkleiner commented 2 months ago

And asset-admin will still use it or also switch to standard controllers?

emteknetnz commented 2 months ago

Any supported module, including asset-admin, that uses some graphql for some of its functionality will standardise entirely on standard controllers for CMS 6

I've linked the PRs that were used in a POC to prove viability in the description - I'll continue using these same PR's in this issue. That includes a PR for asset-admin

NightJar commented 2 months ago

Should this not have been an RFC first?

https://docs.silverstripe.org/en/5/project_governance/request_for_comment/

michalkleiner commented 2 months ago

I'm also concerned how this came about. Haven't heard a single mention about it and now it's being worked towards completely removing GraphQL from the CMS. This feels like it has "3 to 4 upgrade pain" written all over it when upgrading to CMS 6 for customised projects. And even if it ended up being decided to still go ahead and remove it, there should've been some discussion.

emteknetnz commented 1 month ago

That's fair. Yes a public discussion should have come first.

I've raised a new issue for this discussion https://github.com/silverstripe/silverstripe-admin/issues/1805

GuySartorelli commented 1 month ago

CMS 5 PRs merged. Reassigning to Steve to merge up and rebase CMS 6 PRs

GuySartorelli commented 1 month ago

Webpack PR merged, reassigning to Steve to rebuild dist files

GuySartorelli commented 1 month ago

@emteknetnz https://github.com/silverstripe/silverstripe-admin/pull/1802 needs rebasing - after that everything is ready to go! Don't bother redoing the kitchen sink run, it's just yarn.lock.

GuySartorelli commented 1 month ago

@emteknetnz Just one more PR needs rebasing.

There's also an acceptance criteria I think might have been added after refinement:

Create follow up PRs to use the correct deprecation version in CMS 5 e.g. for asset-admin it should be deprecated in 2.3.0, not 5.3.0. After merging ensure merge-up is done correctly as the deprecated methods will have been removed

Not sure what that's about... do you need to do anything more for this?

emteknetnz commented 1 month ago

Have rebased blog PR, and raised new CMS 5 PRs for asset-admin and versioned to use the correct version for deprecation notices

GuySartorelli commented 4 weeks ago

PRs merged

silverstripe / silverstripe-admin