Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Following the description in the parent issue, goal is to implement "duplicate" feature on the current stable release.
Proposed resolution
Implement "duplicate" option in Paragraph "closed" overview. Use dropdown button.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#53 | interdiff-2829316-51-53.txt | 505 bytes | drobnjak |
#53 | implement_duplicate-2829316-53.patch | 10.59 KB | drobnjak |
#51 | interdiff-2829316-49-51.txt | 6.06 KB | drobnjak |
#51 | implement_duplicate-2829316-51.patch | 10.64 KB | drobnjak |
#49 | implement_duplicate-2829316-49.patch | 10.73 KB | drobnjak |
Comments
Comment #2
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedCreated completely new function duplicateSubmit based on addMoreSubmit and part of code used for duplicate.
Last paragraph item is copied into a new array place, with delta increase by 1. Same procedure is following for the previous item, and all before it, until delta of the item that is duplicated currently. Items before duplicated item should not be changed.
I also created new link in dropdown button in "closed" paragraph mode for duplicate feature. There is a unknown reason that link is sometimes recognized as "edit", sometimes as "collapsed" and few times as "duplicate" without any changes in the code in between.
Comment #3
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedTests will probably fail, as the feature is also not working properly but let's give it a try.
Comment #4
Berdiryou shouldn't have to do this by hand, there are php functions to insert things into an array: http://php.net/manual/en/function.array-splice.php. See the last example with length 0, that inserts purple.
What you are missing is calling createDuplicate() on the paragraph object. If you don't do that, we use the same paragraph twice and making changes to one will update both (actually the second will probably win and only those changes will be kept.
It is important to test that. Duplicate, make sure the initial text is duplicated but then make a change and ensure that they are separate and the old text still exists on the original.
Comment #5
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedUploading patch with changes from comment #4. There is still problem with the button so the "duplicate" feature still can not be tested.
Comment #6
johnchqueDiscussed with @drobnjak, this should make it work. One thing that we should think about is what we gonna do when we have already more than 2 paragraphs and we want to duplicate the first one? does it work? or it justs over writes the old one? we need tests about it. :)
And also didn't fix the "array" things. :)
Comment #7
johnchqueJust tested, it works when we have multiple Paragraphs and adds the right one in the correct position! Nice!!!!
Comment #8
johnchqueThis works duplicating just below the original paragraph. I noticed that it doesn't work well for nested ones, because when duplicating them the children are also added but as a "reference" of the original one and they are all updated equally. We need to loop over them and create duplicates of each children. might be recursive?
Comment #9
BerdirYes, the recursion is fun. I think what you want to do is override createDuplicate() on the paragraph entity and loop over fields, any ERR reference that points to paragraphs should then also be duplicated. Then it will automatically be recursive.
Comment #10
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedAdded support for nested paragraphs to evade having changes on one nested paragraph to apply also on duplicated one.
Currently the duplicate option is not working. Error is somewhere in the last foreach.
$field variable has ERR Items but probably a part is missing in targeting entity that should be duplicated.
Comment #11
Berdiras discussed, this needs to live within createDuplicate() of Paragraphs entity, or doesn't work for nesting.
You forgot a lot of spaces ;)
Also, you are creating the duplicate, but what then? You need to actually set it back on $item->entity, so that $item->entity is then a new entity.
Comment #12
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedI override createDuplicate() method and add recursion for nested paragraph elements. Still not working properly.
Comment #13
chr.fritschCould you fire an event when a paragraph is duplicated?
Im thinking about an use case when you have a gallery paragraph. When you duplicate this, the gallery inside should also be duplicated.
Comment #14
johnchque@drobnjak please see:
AFAIK, @Berdir suggested to overwrite the createDuplicate method in the Paragraph Entity.
We cannot rely on this, the type can be "container" or anything, do this as @Berdir suggested, loop over the fields, check the field type, if it is entity_reference_revisions get its target_type if it is 'paragraph' loop over it.
And @chr.fritsch the current purpose of this feature is to duplicate the paragraph and everything inside it, so if we have nested paragraphs, the children inside it will be also duplicated.
Comment #15
chr.fritschWill the duplication happen with for a media_entity which is inside a paragraph, too?
Comment #16
BerdirNot at the moment, but that is somewhat out of our control. A gallery is also something that you reference (even if it is inline entity form) and it is its own thing, unlike a paragraph.
I guess what we could do is look into supporting the replicate module if it is available (We already integrate with it, see ReplicateFieldSubscriber). That would give you an event to do additional stuff. So if replicate.module is available, use its API, otherwise do it ourself as a fallback.
Or we could even only offer the duplicate feature if replicat exists, to avoid duplicating the code.
Comment #17
chr.fritschYes, thats what i mean. Provide some events to do additional stuff on replication. So if you could integrate the replicate module, i am totally happy :-)
Comment #18
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedcreateDuplicate is now moved to Paragraph Entity class. Also, if check for nested paragraph is removed and now we are iterating over fields for every type of the paragraph and looking for ERR.
Duplicate feature is working now for all types except nested paragraph. The problem is somewhere in the last part when recursion is called -
$item->$entity = $entity->createDuplicateParagraph();
Not sure that I am doing this the right way.
Comment #19
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedFeature is finally working. Tested also with nested paragraphs. Interdiff too big.
Comment #20
johnchqueGood work! Some nitpicks.
Missing: /**
* {@inheritdoc}
*/
Space missing after //. Let's change this with: // Loop over entity fields and duplicate nested paragraphs.
Let's fix the indentation here.
Use [].
Missing doc with explanation about what it does.
Btw, we need tests about this functionality. Let's add them in ParagraphsWidgetButtonsTest.
Comment #21
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedChanges from comment #20. Also, test coverage added.
Comment #22
johnchqueextra space before "=="
Change it to: Creates a duplicate of the paragraph entity.
Comment #23
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedChanges from comment #22.
Comment #24
johnchqueStill using array.
Let's add a comment before this. "Create the duplicated paragraph and insert it below the original."
We are not actually "testing" the closed mode, change this by: "Change edit mode to "closed"."
Comment #25
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedChanges from comment #24
Comment #26
miro_dietikerWhy is a duplicated element closed? I'd say it should always be edit since its content are likely duplicates and are subject for override.
You said nesting was challenging and has extra complexity. But the tests don't cover it.
Comment #27
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedDuplicated element is now open. I also added test coverage for the nesting case, testing that the changes on nested elements are not also made on nested elements of the duplicated paragraph.
Comment #30
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedRebasing.
Comment #31
johnchqueLet's assert in the same way the duplicated content, just below this line, copy it and change it to field_paragraphs[1]...
Let's also assert here the $second_paragraph_text.
And then we will be ready for commit this. :)
Comment #32
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedChanges from comment #31
Comment #35
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedNow without mistake.
Comment #36
johnchqueNow is much better, also tested manually, looks really good!
Comment #39
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedRebasing after commit on #2831409: Remove Confirm Deletion / restore actions for paragraphs
Comment #40
johnchqueRTBC-ing again,
Comment #41
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedWe are now using replicate module if it is enabled. Addressing the comment #16 from @Berdir
Comment #42
miro_dietikerAn empty test class does something? We should either write a test there or add a comment why this is not needed.
Missing space at ) {.
Comment #43
BerdirThe replicate test extends the other one, which means we execute all the test methods of the parent class as well. There isn't really anything specific to assert, as the result for the user is exactly the same. We'd have to add a test replicate integration to do something special, which I think is overkill.
Comment #44
miro_dietikerComment #45
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedPatch re-roll. Everything is working properly and tests are passing locally.
Comment #47
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedChange the class that ReplicateTest is extending. Removed duplicate feature test from the WidgetButtonTest class, since it is not needed.
Comment #49
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedReverting extra deleted content from previous patch.
Comment #50
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedProviding a review for #47.
Tested manually with built-in paragraphs types and multiple nested ones. It works quite nice.
Some code (style) comments:
Leftovers.
We could loop over field definitions ($field_name, $field_definition) instead and if there is a match (ERR, Paragraph target entity type in one condition) loop over duplicate field items in the same way.
Code indentation.
A new line is missing.
Does 'duplicate' serve as an identifier or it's related to the second?
One more space.
This seems to be $widget_state.
Can we add a comment why we merge original deltas with
['1' => 1]
?Dot at the end.
An extra line.
Not needed as the parent class uses it too.
An extra empty line.
Sort of needed.
This helper method would make sense in ParagraphsExperimentalTestBase.
Comment #51
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedAddressing the changes from comment #50.
Comment #52
johnchqueDo we really need the paragraphs_mode to be "duplicate"? I think we can just remove it?
Comment #53
drobnjak CreditAttribution: drobnjak at MD Systems GmbH commentedAgreed. I was using it at the beginning but then forgot to remove it, completely obsolete.
Comment #55
miro_dietikerPretty nice, committed. :-)
Note though this is only visible if the default paragraph edit mode is closed. It took me a few mins to identify this problem!
I think, we should split the tests into the two user journeys ASAP because the UX will also differ more soon.
Back to needs work to create follow-ups. Please close when done.
Comment #56
tduong CreditAttribution: tduong at MD Systems GmbH commentedAdded followup #2855768: Remove mode specification from ParagraphsExperimentalDuplicateFeatureTest , not sure if I got the issue right.