Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Chi created an issue. See original summary.

chi’s picture

Issue summary: View changes

Hal module has been moved to contrib. Should we add it as dependency?
https://www.drupal.org/node/3266403

marciaibanez’s picture

Assigned: Unassigned » marciaibanez
Issue summary: View changes

I'll work on this.

marciaibanez’s picture

Assigned: marciaibanez » Unassigned
Issue summary: View changes
Status: Active » Needs review

Hello! I added the drupal 10 support and fixed some deprecated functions using drupal-check. As for the Hal module, I don't have enough experience yet to know if we should add it as dependency and if so, how to do it, but I'd be glad to learn and help as much as I can, so please let me know if I did anything wrong or if I can do something else to help. Kindly review it :)

tmaiochi’s picture

Assigned: Unassigned » tmaiochi

I'll review this!

tmaiochi’s picture

Assigned: tmaiochi » Unassigned
Status: Needs review » Reviewed & tested by the community

The patch removed all deprecated functions and add Drupal 10 support for this module, and everything is working well as far as I can test. I don't know too about Hal module mentioned in #2, if it is not RTBC just let me know!

heddn’s picture

Status: Reviewed & tested by the community » Needs work

The MR is for 1.x branch, but this issue is for 2.x. And we'll also need to handle that hal has moved to contrib in Drupal 10.

See https://git.drupalcode.org/project/default_content/-/blob/2.0.x/default_...

marciaibanez’s picture

Assigned: Unassigned » marciaibanez
marciaibanez’s picture

Assigned: marciaibanez » Unassigned
Status: Needs work » Needs review

Hi @heddn, thanks for letting me know, I didn't notice that, I'm sorry. I changed the MR for the 2.0 now, but I still don't know exactly what to do about the Hal module. I'm going to study about how to do it, but if you or anyone else need this in a short time, please feel free to work on it, I'm leaving it unassigned. If I can do anything else meanwhile, please let me know :)

heddn’s picture

Something weird is happening. MR 17 is linked to this issue. But also to #2933777: REST/Serializer improvements in core/contrib make it less suitable for default_content use case which is closed/fixed.

alexpott’s picture

I think we need a new major because the HAL module has been removed from Drupal 10. See #3049857: Remove HAL module from core and create a contrib project for it

webfaqtory’s picture

I have tried the new HAL contrib module but it's not ready for prime time yet. It depends on PHP8.1 and composer 2 and the namespace is not yet fully defined and will change.

berdir’s picture

2.x is still alpha, and AFAIK hal is just there for backwards compatibility to allow people to switch over. The new format does not depend on it.

Let me see if I can drop that.

berdir’s picture

Pushed a commit that does that. all kernel tests are passing on D10, but I'll need to convert the functional test and its test module. And update the README.

Also made a bunch of other fixes for events, test modules and so on.

berdir’s picture

Removed the functional test and that test module, an identical module and mostly identical kernel test exists for the YAML import.

Still not 100% sure about this approach. There are some options like an optional hal integration where we only check those files and services if the hal module is enabled. But we'd still need test coverage for it then, which is not possible right now.

Despite only being alpha, a 3.x release is also still an option, because people probably don't read release notes even if it's still alpha :)

berdir’s picture

StatusFileSize
new36.19 KB

Can't make the MR work and can't see a way to open a new one, so switching to a patch.

berdir’s picture

Fixing the event classes.

berdir’s picture

Fixing composer.json

andrea.cividini’s picture

StatusFileSize
new36.86 KB

@Berdir +1 for the patch #22. Tests working on Drupal 10.0.0-alpha6.
I just changed two very minor things to remove a deprecation warning - attaching patch.

berdir’s picture

Thanks. When updating a large patch like this with small changes, always provide an interdiff to make it easier for others to review the changes you made: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

andrea.cividini’s picture

StatusFileSize
new961 bytes

@Berdir you are absolutely correct, here's the interdiff, sorry for missing that

benjifisher’s picture

StatusFileSize
new308 bytes
new36.88 KB

Thanks for all the work on this!

When I try to enable the module, I get

PHP Fatal error: Trait "Drupal\serialization\Normalizer\SerializedColumnNormalizerTrait" not found in /var/www/html/web/modules/contrib/default_content/src/Normalizer/ContentEntityNormalizer.php on line 29

That is easy enough to fix: patch and interdiff attached.

berdir’s picture

Status: Needs review » Needs work

Interesting, I'd like to avoid that dependency just for that trait. Might copy & paste it. Why are the tests no failing on that? kernel test magic?

#3164451: Make hal an optional dependency made hal/serialization optional but we didn't notice this there either. But that means this issue needs a major rework anyway to basically not remove all that code.

berdir’s picture

Status: Needs work » Needs review

Created a merge request again, tested locally on D10, had to make a bunch of fixes. Also copied the only method that we need from that trait. Can indeed not reproduce that in tests, I suspect it's because test classloading works different.

  • Berdir committed 65aabd2 on 2.0.x
    Issue #3268215 by Berdir, marciaibanez, andrea.cividini, benjifisher:...
berdir’s picture

Status: Needs review » Fixed

All green :)

Merged, to unbreak HEAD as the serialization dependency already exists there now.

Please test this in your projects on D9 and D10 and let me know if there are any issues.

  • Berdir committed 674b4ca on 2.0.x
    Issue #3268215 by Berdir: Adjusting drupal/hal dev dependency to require...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.