Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove GraphQL from the CMS #1799

Closed
9 of 11 tasks
emteknetnz opened this issue Jul 29, 2024 · 11 comments
Closed
9 of 11 tasks

Remove GraphQL from the CMS #1799

emteknetnz opened this issue Jul 29, 2024 · 11 comments

Comments

@emteknetnz
Copy link
Member

emteknetnz commented Jul 29, 2024

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

  • Functionality that GraphQL enabled is retained and replaced with regular Silverstripe controller endpoints
  • Overall test coverage is not reduced
  • All GraphQL code and config is removed (excluding the actual silverstripe/graphql module)
  • GraphQL dependency is not included in any commericially supported composer.json files
  • 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)
  • silverstripe/graphql is still included in silverstripe/recipe-kitchen-sink
  • PRs target CMS 6
  • 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)
  • 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

CMS 5 PRs

CMS 6 multi PR CI

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
Copy link
Contributor

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

@emteknetnz
Copy link
Member Author

emteknetnz commented Jul 29, 2024

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

This was referenced Jul 29, 2024
@GuySartorelli
Copy link
Member

Webpack PR merged, reassigning to Steve to rebuild dist files

@GuySartorelli
Copy link
Member

@emteknetnz #1802 needs rebasing - after that everything is ready to go!
Don't bother redoing the kitchen sink run, it's just yarn.lock.

@GuySartorelli
Copy link
Member

@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
Copy link
Member Author

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

@GuySartorelli
Copy link
Member

PRs merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants