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 #65

Closed
wants to merge 3 commits into from
Closed

Use external hal #65

wants to merge 3 commits into from

Conversation

whikloj
Copy link
Member

@whikloj whikloj commented Jun 30, 2022

GitHub Issue: #63

What does this Pull Request do?

Includes the contrib hal module and moves the requirement for this module to Drupal 9.4 or higher.

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?

Install and still be able to generate JSON-LD

Interested parties

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

@alxp
Copy link

alxp commented Jun 30, 2022

I just upgraded a Drupal 9.3 site to 9.4.1 via composer update --with dependencies. I still see 'Hal' in core/modules. Are we missing something?

@alxp
Copy link

alxp commented Jun 30, 2022

I think this PR should be against a new major version of jsonld , which will require Drupal 10.

@whikloj
Copy link
Member Author

whikloj commented Jun 30, 2022

Sorry @alxp are you suggesting that we don't release this until 10 comes out?

@alxp
Copy link

alxp commented Jun 30, 2022

On the Drupal.org page it says that HAL is deprecated in 9.4.x and will be removed in 10.x. It may be harmless to have the two instances of Hal but would be cleaner to have it only install if it's needed.

https://www.drupal.org/node/3263629

@whikloj
Copy link
Member Author

whikloj commented Jun 30, 2022

Yeah, I had noted that on the linked issue but it still seemed important to make this change. If we aren't going to do this until 10, then I'll close this PR as we aren't really testing 10 (in the whole stack) with any significance yet.

@whikloj
Copy link
Member Author

whikloj commented Jun 30, 2022

I'll close and open a new Draft PR (too bad I can't switch this back) and whenever 10 comes out and we're ready then we can revisit this.

@whikloj whikloj closed this Jun 30, 2022
@whikloj whikloj mentioned this pull request Jun 30, 2022
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.

2 participants