Closed (fixed)
Project:
Default Content
Version:
2.0.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
7 Mar 2022 at 18:38 UTC
Updated:
16 Aug 2022 at 18:19 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
chi commentedHal module has been moved to contrib. Should we add it as dependency?
https://www.drupal.org/node/3266403
Comment #3
marciaibanezI'll work on this.
Comment #5
marciaibanezHello! 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 :)
Comment #6
tmaiochi commentedI'll review this!
Comment #7
tmaiochi commentedThe 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!
Comment #8
heddnThe 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_...
Comment #9
marciaibanezComment #10
marciaibanezHi @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 :)
Comment #11
heddnSomething 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.
Comment #12
alexpottI 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
Comment #13
webfaqtory commentedI 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.
Comment #14
berdir2.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.
Comment #15
berdirPushed 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.
Comment #16
berdirRemoved 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 :)
Comment #20
berdirCan't make the MR work and can't see a way to open a new one, so switching to a patch.
Comment #21
berdirFixing the event classes.
Comment #22
berdirFixing composer.json
Comment #23
andrea.cividini commented@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.
Comment #24
berdirThanks. 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...
Comment #25
andrea.cividini commented@Berdir you are absolutely correct, here's the interdiff, sorry for missing that
Comment #26
benjifisherThanks for all the work on this!
When I try to enable the module, I get
That is easy enough to fix: patch and interdiff attached.
Comment #27
berdirInteresting, 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.
Comment #29
berdirCreated 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.
Comment #31
berdirAll 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.