Problem/Motivation

Currently we only override the paragraph entity form classes. This leads to cases, where the actual paragraph does not have links like canonical, edit, etc., even though there exists forms for these links.

Furthermore this will bring many out of the box fixes and improvements, like views support for CRUD links and much more!

Steps to reproduce

Proposed resolution

Refactor paragraphs edit to use more streamlined paragraph entity routes / link templates. This will also make it able to display paragraph edit,delete and clone inside a view.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 3582207-4.x-refactor-paragraphs-9.patch19.34 KBgrevil

Issue fork paragraphs_edit-3582207

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:

About issue forks

Comments

grevil created an issue. See original summary.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @grevil yes this definitely makes sense and allows a lot better integration then, following established core-standards! Let's start a MR. If maintainers agree, it would be great to have a new default 4.x branch. Otherwise we should at least tag it with 3.1.0, but I think a fresh branch would make sense for BC. https://www.drupal.org/project/paragraphs_modal_edit may rely on it e.g.

grevil opened merge request !21

grevil’s picture

grevil commented
Issue summary: View changes
Status: Active » Needs review

All done, please review! Note, that this still needs some further adjustments to resolve the todos. Nothing critical though, could be resolved in follow-up issues.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

GREAT WORK @grevil many issues resolved and cleaned up things a lot. Perfect starting point for 4.0.0-alpha1!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Issue tags: +Needs tests

This should have tests to ensure it works as expected, especially regarding permissions to ensure not giving access to anyone for editing or exposing information, e.g. on the canonical, edit, delete or clone.

grevil’s picture

grevil commented

Resolved your comments, please do a final check!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Issue summary: View changes
grevil’s picture

grevil commented
StatusFileSize
new 3582207-4.x-refactor-paragraphs-9.patch19.34 KB

Static patch for the time being.

lrwebks made their first commit to this issue’s fork.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Reviewed & tested by the community » Needs work

@lrwebks pipeline fails!

lrwebks’s picture

lrwebks commented

PHPUnit (previous minor) fails because of version incompatibility, so I am not sure what we can do about it?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs work » Needs review

Reason was an unused use-statement and an upstream cspell issue with a trait method name.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

All green, ready for the (to be created) 4.x branch! 🎉

bbrala’s picture

bbrala
Location Netherlands
commented

Awesome work! Left some small comments.

Also i wonder, would it be needed/useful to provide an upgrade path? Although it seems that is only for permissions?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Great, thank you @bbrala! We'll come back to this in this or the next week!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » grevil
Status: Reviewed & tested by the community » Needs work

Back to NW to resolve the comments finally! :)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Version: 3.x-dev » 4.x-dev
grevil’s picture

grevil commented
Assigned: grevil » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests

Alright, now we should be good to go!

Someone please do a final review! :)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@bbrala would you check and sign this off maybe? Because I was participating in code here... I think that would make sense.

grevil’s picture

grevil commented
Status: Needs review » Needs work

Hold up, clone throws an error and delete emits a warning. Do we not have tests, that test the seperate forms? Back to NW.

grevil’s picture

grevil commented
Assigned: Unassigned » grevil
grevil’s picture

grevil commented

Alright, I added and refined existing tests and they currently fail as expected (because of the reasons above).

Gonna fix the code now.

grevil’s picture

grevil commented

Ok, found an issue with the module. In Drupal, when "target_bundles" is NULL, ALL bundles are allowed, but "getPotentialCloneDestinations" treats it as no bundle is allowed instead.

I added a quick fix, but it is far from perfect yet. I first need to check, what "target_bundles_drag_drop" and why it was introduced this way back in #3270785: getPotentialCloneDestinations() doesn't always work. No time left today though.

grevil’s picture

grevil commented
Assigned: grevil » bbrala
Status: Needs work » Needs review

Alright, that should be it! I refactored "getPotentialCloneDestinations". The old implementation didn't make much sense to me.

Again: For paragraph fields "target_bundles" is set to NULL explicitly to allow ALL paragraph types. Otherwise the value will be an array of allowed paragraph types (I added a check, to be 100% sure, that we are working with an array in that case. If it is a string or integer for whatever reason, we are using an empty array as fallback).

"negate" has only an effect, if "target_bundles" is NOT NULL. Otherwise we simply diff the target_bundles with all other available target_bundles.

I also adjusted the clone labels with internal feedback by @anybody. Please review!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

RTBC+Ready for alpha1 from my side! But would like to have @bbrala's okay if possible.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

Ok I'll merge this now, think @bbrala is surely busy. Releasing an alpha1 and merging this into the fresh 4.x branch won't do any harm.

  • anybody committed 75c3629d on 4.x authored by grevil 
    feat: #3582207 [4.x] Refactor paragraphs edit to use more streamlined...
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: bbrala » Unassigned
Status: Reviewed & tested by the community » Fixed

Merged! @grevil will you tag an alpha1?

@bbrala still feel free to check the code and create an issue if you find any further points to improve in 4.x! Thank you!!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

bbrala’s picture

bbrala
Location Netherlands
commented

Was sick for a bit, sorry about that :)

grevil’s picture

grevil commented

No worries! Thanks for making 4.x the main branch :)