Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
\Drupal\migrate\Plugin\MigrateProcessInterface
documents that multiple entry points can be used in the process plugin, but that is logic that lives on the \Drupal\migrate\ProcessPluginBase
class.
Proposed resolution
Move the documentation from MigrateProcessInterface to where the mentioned logic is implemented, i.e. \Drupal\migrate\ProcessPluginBase
and adjust as needed.
Remaining tasks
Write a patch.
User interface changes
N.A.
API changes
N.A.
Data model changes
N.A.
Comment | File | Size | Author |
---|---|---|---|
#32 | 2949400-32.patch | 2.44 KB | ayushmishra206 |
#32 | interdiff_27-32.txt | 1.31 KB | ayushmishra206 |
#27 | 2949400-27.patch | 2.57 KB | ayushmishra206 |
#25 | 2949400-25.patch | 2.57 KB | ayushmishra206 |
#25 | interdiff_21-25.txt | 1.67 KB | ayushmishra206 |
Comments
Comment #2
marvil07 CreditAttribution: marvil07 as a volunteer commentedOne fix.
Comment #3
zymbian CreditAttribution: zymbian as a volunteer commented+1 nice fix.
Comment #8
marvil07 CreditAttribution: marvil07 as a volunteer commentedMoving component, marking as NW for a reroll.
This probably still makes sense even if the text changed a bit at #2936821: unclear docs in MigrateProcessInterface.
Comment #9
quietone CreditAttribution: quietone as a volunteer commented@marvil07, thanks for moving this to the migration.system! Now it will get some attention, the migrate maintainers work on that component and while I occasionally search for certain tags, I haven't searched the documentation component.
I wonder if this would benefit from an example? Something along these lines.
I'd prefer commas instead of parenthesis.
Move the 'See' to an @see.
Comment #10
AkashKumar07 CreditAttribution: AkashKumar07 at OpenSense Labs commentedI am working on this.
Comment #11
AkashKumar07 CreditAttribution: AkashKumar07 at OpenSense Labs commentedPlease review the patch.
Comment #12
quietone CreditAttribution: quietone as a volunteer commented@AkashkumarOSL, thanks for working on this issue. Good to see progress here. Even for a small patch it helps the reviewer if you provide an a comment about what you did or did not do. Speaking for myself, before I look at the patch I would like to know if you have made fixes for 9.1 and 9.2 or had questions about it.
Looking at this again I see that we should just move the doc paragraph from MigrateProcessInterface to ProcessPluginBase. I mean, ignore the latest patch here and start from HEAD, then move the text from MigrateProcessInterface to ProcessPluginBase. Although the last line of that text will not make sense in ProcessPluginBase. I will leave it up to the creator of the next patch to decide what to do.
Comment #13
chandrashekhar_srijan CreditAttribution: chandrashekhar_srijan at Srijan | A Material+ Company for Drupal India Association commentedAdded doc paragraph in the ProcessPluginBase for method multiple() for Drupal 9.1.x
Please review.
Comment #14
chandrashekhar_srijan CreditAttribution: chandrashekhar_srijan at Srijan | A Material+ Company for Drupal India Association commentedAdded interdiff for #13
Comment #15
quietone CreditAttribution: quietone as a volunteer commented@chandrashekhar_srijan, thanks for the patch and interdiff, nice.
This can be removed. It is out of scope.
What needs to happen here is to move the paragraph at the top of MigrateProcessInterface to ProcessPluginBase.
The doc for MigrateProcessInterface can be this
and what was there gets moved to ProcessPluginBase, with a few tweaks.
I still think there is room for improving the text. Ideas welcome!
Comment #16
bandanasharma CreditAttribution: bandanasharma as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedAdded the new patch and make the changes as per @quietone comment.
Comment #17
mikelutzThis copied the docs from the interface to the base plugin, but didn't remove/change the docs on the interface. Back to NW for the adjustments needed to the interface.
Comment #18
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedMade the changes please review.
Comment #19
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #20
mikelutzI think we still need some of that documentation on the interface. It just needs to make sense.
Comment #21
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedMade the requested changes in #20 from #15. Please review
Comment #22
mikelutzRequeuing the test.
Comment #23
quietone CreditAttribution: quietone as a volunteer commentedThat first sentence doesn't read well. s/using extending/extending/
Please check the spacing in this paragraph. There is at least one double space where it should be a single space.
Delete these and add
@see d7_field_formatter_settings.yml
to the end of the existing @sees.This is a repeat of what is in MigrateProcessInterface and does not need to be repeated here.
I applied that patch and there are two sections of @see in ProcessPluginBase instead of one. See Drupal API documentation standards for order of documentation sections for more details on that.
Comment #24
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #25
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedPlease review if this is what you recommended. Thanks @quietone
Comment #26
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedhey @ayushmishra206, i tried to apply the patch and I found two white space errors on line 33 and 39 of the patch.
Other than that, all the changes as suggested above are done.
Comment #27
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedThanks for the review i solved those errors in this patch.
Comment #28
meghasharma CreditAttribution: meghasharma as a volunteer and at QED42 for Drupal India Association commentedComment #29
meghasharma CreditAttribution: meghasharma as a volunteer and at QED42 for Drupal India Association commentedpatch applied cleanly..
now there is no error in the #27 patch, all the changes as suggested above are done.
Comment #30
quietone CreditAttribution: quietone as a volunteer commented@ayushmishra206, thanks for continuing to work on this. I just found a few final items.
I applied the patch and read the comments and found a few things that could be improved. I checked and there are no coding standard errors.
s/alternatives/alternative
This last sentence isn't needed since this is ProcessPluginBase.
Let's move this to below the SkipOnEmpty line and keep all the classes together.
Comment #31
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #32
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commented@quietone happy to work! I have made the changes, please review.
Comment #33
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #34
asishsajeev CreditAttribution: asishsajeev at Zyxware Technologies commentedComment #35
asishsajeev CreditAttribution: asishsajeev at Zyxware Technologies commentedPatch applied successfully.
I Read the comments. All the changes suggested above are done.
Comment #36
asishsajeev CreditAttribution: asishsajeev at Zyxware Technologies commentedComment #37
alexpottCommitted and pushed a677705b3b to 9.1.x and 9b0a971a8e to 9.0.x and 0f3ef3d489 to 8.9.x. Thanks!
Backported to 8.9.x as this is a documentation only change.