drupal-graphql / graphql

GraphQL integration for Drupal 9/10
287 stars 202 forks source link

Drupal 10 compatibility for 3.x #1347

Closed Luigisa closed 1 year ago

Luigisa commented 1 year ago

Major update.

Draft release notes

The "graphql-php" library has been updated to one of the latest versions, making it compatible with PHP8. This can cause compatibility breaks with custom code within existing projects.

This version is no longer compatible with Drupal 8.

The code has been updated to stop using deprecated functions in Drupal 10 and also to make it compatible with PHP8.

The graphql explorer and voyager libraries have also been updated with the same changes as in the 4.x branch. This way, the core library "core/once" is used, and "jquery.once" is no longer used.

We recommend upgrading from 3.x to 4.x to take advantage of the improvements made in the GraphQL module and the underlying GraphQL library. If you still require automatic generation of the schema then take a look at the GraphQL Compose or GraphQL Core Schema projects which have implemented automatic schema generation on top of the 4.x GraphQL module.

Luigisa commented 1 year ago

Thanks for starting this! Adding 10.x compatibility is not an easy task I think. Although the pull request makes clear what we need to think about I do think it needs more work.

  1. There is some JavaScript reformatting in there. That makes reviewing slightly more difficult (e.g. changing all ' to "). I'm not sure if that fixes any new Drupal coding standards of is just a local settings thing. Take a look at assets/explorer/src/index.js (if it's not a Drupal 10 thing, you may want to move it to a separate PR to make reviewing easier, and add some motivation for why that's the new normal).
  2. There's similar reformatting in the PHP test files.
  3. We're "not dropping" 8.x support in our version constraints, which I think is a good thing because we'd need a major version change. However, I'm also not sure that all the services we've adopted to be 10.x compatible actually already exist in 8.x (or even all 9.x versions). So this will need some discussion: either we accept that some Drupal versions are no longer supported and those people can no longer receive 3.x security updates if one comes out in the future; or we must make a major version change somehow so that the older version can still be patched.
  4. I've flagged only 1 empty class docblock comment, but there were more. We need to make sure that (automated) fixes we perform are not just removing the error from our tooling, but are actually a benefit to developers. If we can't provide a proper comment that helps developers then it's better to keep the error around as a reminder we need to fix this in the future.
  1. I understand. It was my local configuration. I will only upload changes that affect jquery.eleven.
  2. Ok, I review it.
  3. This point seems important to me. My opinion is to leave only support for drupal 9.5 and higher, adding drupal 10. I would not support drupal 8, because it is deprecated. And in a very short time drupal 9 will be too.
  4. Ok, i review it.

Thanks

pfrenssen commented 1 year ago

Great work, thanks!

There was a competing implementation on drupal.org: https://www.drupal.org/project/graphql/issues/3297335

Our issue queue on drupal.org is not enabled, but it looks like the Project Update Bot has permission to create issues. A merge request was created there by @andreylugovtsov . I tried to close this issue but I cannot post there. Would it be possible for a maintainer to close this issue?

Luigisa commented 1 year ago

I can't close the ticket because the issues are not enabled for that project on drupal.org.