- 🇮🇪Ireland lostcarpark
Just to add a long overdue update.
I'm working on a port to Drupal 10. It will probably also work on Drupal 9.
The D9/10 version will be a full rewrite, as the structure of D8+ is too different to be worth trying to reuse the old code. I am running a D7 site in parallel so I can compare the functionality.
At present I am developing a service to handle the moving of comments. The service implements a method to move comments and allows the new parent entity and comment to be specified.
To move a comment, three fields need to be updated: the entity_id (the entity the comment belongs to), pid (the parent comment under the entity) and thread (a string indicating the position of the comment in the entity's comment thread).
So far it is working for moving individual comments, but not comments with children, so I'm working on recursively moving children next.
I have test cases working for moving individual comments.
My plan is to build the comment mover service and a strong set of test cases to prove it works reliably.
I then will produce UI elements.
In order to get an initial version out fairly quickly, I'll probably only focus on moving comments between entities for the first version, then look at more complex functionality like converting nodes to comments and comments to nodes.
- Assigned to lostcarpark
- Status changed to Needs work
over 1 year ago 5:51am 17 May 2023 - 🇧🇪Belgium BramDriesen Belgium 🇧🇪
Sounds great, happy to review or brainstorm with you if needed :-).
A few extra things. I'm not sure if there are any active maintainers left.
- If there is one, it would help a lot if a new branch could be created (2.0.x?). So @lostcarpark can work with GitLab branches and target the new branch for his MR
- If there are none, a ticket to request (co-)maintainership should be created in this queue, and if no reply comes on that it should be moved to the https://www.drupal.org/project/projectownership → queue after two weeks ( all info on how that works can be found here → ).
- 🇷🇺Russia Nikita Petrov
I would be happy to pass the maintainer privileges for this module to @lostcarpark, but I do not know how to do it, can anybody give me a link to a related drupal.org documentation, please?
- 🇧🇪Belgium BramDriesen Belgium 🇧🇪
You need to have sufficient permissions in order to do that. Normally as a maintainer that can manage maintainers you have a "maintainers" tab on the top of the module homepage (not issue queue). From there it should be straight forward adding a new maintainer. (That's a lot of maintainer in once sentence 😅)
- 🇧🇪Belgium BramDriesen Belgium 🇧🇪
Removing D8 tag as it's EOL :-) our focus needs to be 9 & 10
- Issue was unassigned.
- Status changed to Active
over 1 year ago 10:03am 17 May 2023 - 🇮🇪Ireland lostcarpark
Agree focus needs to be D9/10. I'm focusing mainly on D10, but I don't think I'm doing anything that won't work in D9. Hopefully we can get a 2.0.x branch set up and set up CI to run tests against both.
I suspect that it could also work for D8, but I don't think it has any relevance, and I don't plan on testing.
- 🇮🇪Ireland lostcarpark
@BramDriesen, thanks for the suggestions.
I have now set up 📌 Create a new branch for D9/10 Fixed to request a 2.0.x branch and 💬 Offering to co-maintain Comment mover Fixed to request co-maintainership.
- 🇷🇺Russia Nikita Petrov
Ok, in this case I cannot transfer ownership (I do not have such an option on module's page), and as far as I know the previous maintainers are no longer available - @dragonwize @webchick
I suggest you to use the https://www.drupal.org/project/projectownership → queue. - 🇧🇪Belgium BramDriesen Belgium 🇧🇪
Seems like thins are moving quickly ;-) @lostcarpark is now a maintainer 🥳
- @lostcarpark opened merge request.
- 🇧🇪Belgium BramDriesen Belgium 🇧🇪
Nice work! Guess you can configure the automated tests as well (on the project page under automated tests). Not sure if you need a dev release or not for that. You probably also need to check 2.0.x as a supported branch somewhere in one of those project setting screens.
Spotted a few nits in the code but will keep those for another time if you like 😁.
-
lostcarpark →
committed 19dc49ee on 2.0.x
Resolve #2900193 "Port to drupal 9"
-
lostcarpark →
committed 19dc49ee on 2.0.x
- 🇮🇪Ireland lostcarpark
I have merged my initial code into 2.0.x-dev to allow me to set up automated testing.
This is very much a development release, and is not ready for use on sites.
Development so far has focused on the comment moving service. At present this provides one public function,
moveComment
, which moves a comment to a new parent entity and parent comment. If parent comment is null, comment will be attached directly to the entity. If the moving comment has any child comments, they will also be moved to the new entity and their thread values recalculated.At present it assumes comment bundle is "comment", which is the default, but I'm going to look at supporting other bundles next. I'm not yet sure whether moving comments between bundles will be practical.
I have not yet looked at converting notes to comments or comments to nodes. I expect this will come after an initial alpha release just supporting moving comments.
There is currently no UI (apart from some temporary debugging code in the block).
I want to do some more work on the service, and add some more test cases.
I'll start work on UI shortly, and hope to have an alpha release people can try out in the near future.
- 🇧🇪Belgium BramDriesen Belgium 🇧🇪
Awesome! Updating the version as well now this is possible ;-)
- Status changed to Needs work
over 1 year ago 6:20pm 18 May 2023 - 🇮🇪Ireland lostcarpark
I'm looking at how I can make the comment mover code more generalised.
Currently, it can move comments of the default "comment" comment type between entities. Since comment types need to be associated with an entity type, this means it won't currently work for comments belonging to non-content entity types.
However, I suspect most sites only use the default comment type, so this restriction would not affect the majority of sites.
If we support multiple comment types, some questions arise. Should moving comments between different comment types, which may support different sets of fields be allowed?
I think there would probably need to be an admin page, or possibly fields added to the edit comment type page, to allow control of which comment types allow comment moving.
I suspect in the majority of sites, only the default comments will be used anyway, so maybe I'm overthinking it.
Opinions welcome!
- 🇧🇪Belgium BramDriesen Belgium 🇧🇪
If we support multiple comment types, some questions arise. Should moving comments between different comment types, which may support different sets of fields be allowed?
That sounds like a difficult path to conquer. That would mean that you need to make a mapping between each and every comment type (bundle). So let's say you would have a site with 10 comment types. That would mean you need to build around 90 mappings 🤯 since you the config page would probably be structured like Comment type A maps to fields on B,C,D,E,F,G,H,I,J and then the same for the 9 others. (10x9=90).
I would say to stick to one bundle type. At least for now 😅.
A second option would be to allow this, but to only allow identical fields that exist on both to be mapped. But that might give more issues then it's worth, because even how well documented that could be. No one RTFM 🤣.
- 🇷🇺Russia Nikita Petrov
The ability to move comments between different entity types is a feature which doesn't affect the architecture of your module (as far as I can tell, I do not know Drupal 9/10 so I just guess). Therefore no need to think about it right now. Implement the core functionality and then add such a nice feature later.
- 🇧🇪Belgium BramDriesen Belgium 🇧🇪
I guess this has become a plan then. You can also split out larger tasks into issues of their own if needed.
- 🇮🇪Ireland lostcarpark
Agreed, there may be sub issues as required.
One thing to note. I am using PHP property promotion, which allows dependency injection to use about a third of the amount of code required without it. As this is a PHP 8 feature, I'm setting the minimum PHP version for this module to 8.1, which is in line with Drupal 10. I don't think this is a big issue, since D9 goes end of life in November. It may require some people to update their PHP version, but they'll need to do this for D10 anyway. I don't think it's worth delaying adopting PHP 8 features for a few months of increased D9 support.
- @lostcarpark opened merge request.
- 🇮🇪Ireland lostcarpark
Trying to push some changes, but 2.0.x branch doesn't exist in fork for this issue, so I'm getting a bit stuck. I'm sure I just need to figure out how to get the fork updated.
However, I think it makes sense to create a new issue for the Alpha 1 release, so that's what I'm going to do.
- 🇮🇪Ireland lostcarpark
Just a brief update to note that I now have the clipboard functionality working in Kernel tests. I'm working on getting connected to user interface. I think this will be fairly straightforward. I've some caching issues that I think will need a little figuring out, but hopefully it will be reasonably straightforward.
- Status changed to Needs review
over 1 year ago 7:16am 16 June 2023 - 🇮🇪Ireland lostcarpark
Comment Mover 2.0.0-alpha1 now released.
More features coming soon, so alpha2 should follow fairly quickly, but thought it good to get an early release out so hopefully people can start testing. Any feedback appreciated.
- 🇮🇪Ireland lostcarpark
I have completed a function to convert a comment to a node, with a test. Although it's not very much code (20 lines for the conversion function, 11 lines for the test), there was quite a bit of investigation required to get to this point.
I still need to add to the UI, but I just wanted to note this as an important milestone on the road to the next alpha, and having got this worked out, progressing the change should be fairly straightforward.
- 🇮🇪Ireland lostcarpark
Currently having an issue with 📌 Comment mover 2.0.0-alpha2 release Needs work that is holding up progress. I am trying to delete a comment in a test, and the test is failing.