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

Use external hal #66

Closed
wants to merge 1 commit into from
Closed

Use external hal #66

wants to merge 1 commit into from

Conversation

whikloj
Copy link
Member

@whikloj whikloj commented Jun 30, 2022

GitHub Issue: #63

What does this Pull Request do?

Includes contrib module HAL dependency

What's new?

  • Does this change require documentation to be updated? no
  • Does this change add any new dependencies? yes
  • Does this change require any other modifications to be made to the repository
    (i.e. Regeneration activity, etc.)? no
  • Could this change impact execution of existing code? no

How should this be tested?

Still be able to produce JSON-LD serializations of Drupal objects.

Interested parties

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

@whikloj
Copy link
Member Author

whikloj commented Jun 30, 2022

This PR will not be merged until Drupal 10 is released.

@whikloj
Copy link
Member Author

whikloj commented Jun 30, 2022

This replaces #65

@rosiel
Copy link
Member

rosiel commented Jul 6, 2022

suggestion to make this a 3.x branch of jsonld (because changing the supported Drupal from 9 only to 10 only would make this a major version, wouldn't it?)

@whikloj
Copy link
Member Author

whikloj commented Jul 7, 2022

That is fine by me, probably makes sense not to rush to create a 3.x branch yet as any other PRs would then need to be applied to both 2.x and 3.x.

IMHO Better to wait until Drupal 10 is actually released and we are starting to make sure we (Islandora) are compliant.

@rosiel rosiel added the drupal10 For Drupal 10 Readiness label Dec 2, 2022
@kayakr
Copy link

kayakr commented Apr 17, 2023

@whikloj @rosiel It would be good to have core_version_requirement: ^9 || ^10 so that this module can applied to D9 sites as they prepare for Drupal 10.

@rosiel
Copy link
Member

rosiel commented Apr 17, 2023

Yeah, D10's been released so we should get on this!

From what I gather, the composer requirement for the external hal is needed if and only if we're on Drupal 10, but it will conflict with Drupal 9. I don't think it's possible (please correct me if i'm wrong) for there to be a branch where the Composer file is compatible with 9 or 10.

I think (again, I hope y'all know more than me) that we'll need to create a jsonld:3.x branch which will be for D10 and only D10. This PR (with a few modifications) can start this off. But islandora/islandora currently has a requirement for jsonld:^2. I think if we change that to >2, then we'll be able to start moving forward in testing with Drupal 10. There will be other problems, guaranteed, but it'll be a start.

php-versions: ["7.4", "8.0", "8.1"]
drupal-version: ["9.3.x", "9.4.x-dev"]
php-versions: ["8.0", "8.1"]
drupal-version: ["10.0.x-dev", "10.1.x-dev"]
Copy link
Member

Choose a reason for hiding this comment

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

"10.0" instead of "10.0.x-dev"

@@ -20,7 +20,8 @@
}
],
"require" : {
"php": ">=7.4"
"php": ">=8.0",
"drupal/hal-hal": "^1.0"
Copy link
Member

Choose a reason for hiding this comment

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

For D10, this seems it should be "drupal/hal": "^2" at least per https://www.drupal.org/project/hal

- hal
- serialization
- rdf
- drupal:hal
Copy link
Member

Choose a reason for hiding this comment

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

should be hal:hal etc.

@rosiel rosiel mentioned this pull request May 24, 2023
@whikloj
Copy link
Member Author

whikloj commented May 25, 2023

Closing in favour of #69

@whikloj whikloj closed this May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
drupal10 For Drupal 10 Readiness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants