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 10 readiness #69

Merged
merged 14 commits into from
Jun 21, 2023
Merged

Drupal 10 readiness #69

merged 14 commits into from
Jun 21, 2023

Conversation

rosiel
Copy link
Member

@rosiel rosiel commented May 24, 2023

GitHub Issue: #63 , supercedes PR #66

  • Other Relevant Links (Google Groups discussion, related pull requests,
    Release pull requests, etc.)

What does this Pull Request do?

Tells Drupal the jsonld module is compatible with Drupal 10 (and also Drupal 9). This assertion still needs to be proven, hence this PR is in draft. Turned out it is impossible to be D9 and D10 compatible simultaneously, so this now proposes a 3.x branch to accommodate Drupal 10.

What's new?

  • update composer.json
  • update info.yml
  • update testing scenarios
  • (new) Update README.md.

How should this be tested?

Install the module in Drupal 10 (and in D9) and set up an RDF mapping to jsonld and expose it at an endpoint. Does it work? Any messages in the watchdog logs?

Additional Notes:

Any additional information that you think would be helpful when reviewing this
PR.

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/committers

jsonld.info.yml Outdated Show resolved Hide resolved
.github/workflows/build-2.x.yml Outdated Show resolved Hide resolved
@rosiel
Copy link
Member Author

rosiel commented May 24, 2023

Currently stymied by the pdf module, which doesn't have a D10 release though a patch generated by Rector is RTBC several times over, and folks are clamoring for it to get merged.

The pdf module is installed in all our workflow testing environments by islandora_ci.

@rosiel
Copy link
Member Author

rosiel commented May 24, 2023

I've got it running on a Drupal 10 site. @whikloj I'm trying to test but not 100% sure if this is what I'm supposed to do:

I installed restUI and got this error:
Fatal error: Declaration of Drupal\jsonld\Normalizer\NormalizerBase::supportsNormalization($data, $format = null) must be compatible with Drupal\serialization\Normalizer\NormalizerBase::supportsNormalization($data, ?string $format = null, array $context = []): bool in /var/www/html/web/modules/contrib/jsonld/src/Normalizer/NormalizerBase.php on line 23

@whikloj
Copy link
Member

whikloj commented May 24, 2023

@rosiel that is probably because we don't have return types defined and that (in PHP 8) means we are not correctly extending the base class.
Based on 10.1.x version of Drupal, it appears we should have a bool return type for that method.
https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/serialization/src/Normalizer/NormalizerBase.php#L26

You probably want to check the other methods we are overriding to ensure our signatures match the base class.

@rosiel
Copy link
Member Author

rosiel commented May 24, 2023

OK, I updated three files based on running into errors, and now I can load the jsonld version of a node. CI will still fail because of pdf, though.

@rosiel
Copy link
Member Author

rosiel commented May 25, 2023

Installing this on Drupal 9 fails:

PHP Fatal error: Declaration of Drupal\jsonld\Normalizer\NormalizerBase::supportsNormalization($data, ?string $format = null, array $context = []): bool must be compatible with Drupal\serialization\Normalizer\NormalizerBase::supportsNormalization($data, $format = null) in /var/www/html/drupal/web/modules/contrib/jsonld/src/Normalizer/NormalizerBase.php on line 23

Does this mean that because the core serializer class' signature has changed, therefore we need a new branch for D10?

@whikloj whikloj mentioned this pull request May 25, 2023
@whikloj
Copy link
Member

whikloj commented May 25, 2023

I guess this will have to be a new branch with a new major version.

@rosiel rosiel changed the base branch from 2.x to 3.x May 25, 2023 19:01
@rosiel
Copy link
Member Author

rosiel commented May 25, 2023

I've altered the target branch, changed the workflows (they'll still fail due to pdf), and added a note in the README. For linking purposes, this addresses part of Islandora/documentation#2235.

@rosiel rosiel marked this pull request as ready for review May 25, 2023 19:16
@rosiel
Copy link
Member Author

rosiel commented Jun 14, 2023

Since Islandora/islandora_ci#18 was merged, I updated the tests so they don't point to my patch branch of the Islandora CI repo.

Copy link

@alxp alxp left a comment

Choose a reason for hiding this comment

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

Works for me and PHPUnit shows no new warnings.

@alxp alxp merged commit 2e95dca into Islandora:3.x Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants