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

Drupal 11 readiness. #1407

Merged
merged 18 commits into from
Aug 2, 2024
Merged

Drupal 11 readiness. #1407

merged 18 commits into from
Aug 2, 2024

Conversation

apathak18
Copy link
Contributor

Prepare drupal-graphql module ready for Drupal 11

@klausi
Copy link
Contributor

klausi commented Jun 15, 2024

Thanks, if we start to support Drupal 11 we should enable it in our testing. Can you enable it in testing.yml?

@klausi klausi added the 4.x label Jun 15, 2024
@apathak18
Copy link
Contributor Author

apathak18 commented Jun 16, 2024

Thanks, if we start to support Drupal 11 we should enable it in our testing. Can you enable it in testing.yml?

@klausi Initial change in the workflow to include D10.3.x and D11.0.x
Now Jobs are failing and will push changes as per the failed jobs.

Copy link
Contributor

@klausi klausi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

The test fails Call to undefined method GraphQL\Server\ServerConfig::setPersistentQueryLoader() probably happen because you updated the graphql library.

If we are forced to do that then we need to fix our API calls as well.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
.github/workflows/testing.yml Outdated Show resolved Hide resolved
.github/workflows/testing.yml Outdated Show resolved Hide resolved
@klausi
Copy link
Contributor

klausi commented Jun 28, 2024

I just merged #1410 to enable Drupal 10.3 testing. So in this PR we only need to add one extra run for Drupal 11 without PHPStan.

@Kingdutch
Copy link
Contributor

Thanks for opening this!

I do believe we should split this PR up into multiple PRs and possibly over multiple releases.

For example the webonyx/graphql-php dependency was flagged already as breaking change in #1335 so if that's a blocker for Drupal 11 then we likely need to release GraphQL 5.0.0 with Drupal 10.x and 11 compatibility.

We should also evaluate the other dependency upgrades and determine whether they're needed or nice to have. Ideally doing them in a way where extending modules can focus on one change at a time.

I think it's important for a smooth upgrade experience that we apply Symfony's upgrade philosophy here. We should ensure that consumers can always focus on upgrading one thing at a time. For example, they can focus on upgrading the PHP version and then separately the GraphQL package before making the jump to Drupal 11. Or Drupal 11 first and then the GraphQL package.

I know from experience that modules that force adopters to upgrade multiple composer packages at the same time and/or the PHP version as well, are a lot harder than having only one of those be included in a release.

With the insights that you already have of what might need to be upgraded it'd be great to open up an issue that outlines which changes we need to make so we can plan the right approach.

@apathak18 apathak18 force-pushed the 8.x-4.x branch 6 times, most recently from a4642a4 to ca2eab9 Compare July 18, 2024 17:55
@apathak18
Copy link
Contributor Author

@Kingdutch @klausi I've rebased the PR and revert the webnoyx package to v14 as v15 starts failing all tests.

Right now tests are passing for all D10.x.x versions except 11.0.x (Am I missing something or is there any configuration for database server which needs to be updated)
See:https://github.com/drupal-graphql/graphql/actions/runs/9996316696/job/27630424860?pr=1407#step:11:1257

@vishalkhode1
Copy link

@Kingdutch @klausi I've rebased the PR and revert the webnoyx package to v14 as v15 starts failing all tests.

Right now tests are passing for all D10.x.x versions except 11.0.x (Am I missing something or is there any configuration for database server which needs to be updated) See:https://github.com/drupal-graphql/graphql/actions/runs/9996316696/job/27630424860?pr=1407#step:11:1257

@apathak18 The tests are failing because of old version of Ubuntu, you should update .github/workflows/testing.yml file and change runs-on: ubuntu-latest to runs-on: ubuntu-24.04

@apathak18
Copy link
Contributor Author

@Kingdutch @klausi 11.0.x pipeline reports phpstan issues ~~ Mentioned below are Changelog of those deprecations:

  1. https://www.drupal.org/node/2999981 (format_size)
  2. https://www.drupal.org/node/3363700 (file_validate)

Can we bump the drupal minimum version to D10.2? Otherwise we need to handle these deprecations to support backward compatibility.

@apathak18 apathak18 force-pushed the 8.x-4.x branch 14 times, most recently from 78935d1 to b9ade93 Compare July 24, 2024 18:04
@apathak18
Copy link
Contributor Author

@klausi All feedbacks are addressed ~~ also rebased & resolved the conflicts, now this one is ready for a review.

Copy link
Contributor

@klausi klausi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, 2 minor points remaining.

I will also manually test the new test subscriber if it would fail when the file does not exist.

.github/workflows/testing.yml Outdated Show resolved Hide resolved
@klausi
Copy link
Contributor

klausi commented Aug 2, 2024

Please don't do force pushes, makes it harder for me to update local test branches. All the commits here don't matter, we will squash merge them aynway at the end.

@apathak18 apathak18 requested a review from klausi August 2, 2024 12:22
@apathak18
Copy link
Contributor Author

Ohh I'll note this from ahead.
For now I'm done with all the feedbacks, please check.

Copy link
Contributor

@klausi klausi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, all looking good now. Thank you for your patience!

@klausi klausi merged commit b677475 into drupal-graphql:8.x-4.x Aug 2, 2024
5 checks passed
@apathak18
Copy link
Contributor Author

Excellent, all looking good now. Thank you for your patience!
Thanks
Quote of the day: Patience is a key to success!!

@vipin-mittal-acquia
Copy link

Hello @klausi, When can we expect stable release of GraphQL? Thanks!

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

Successfully merging this pull request may close these issues.

5 participants