Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
class Row {
/**
* Sets a source property.
*
* This can only be called from the source plugin.
*
* @param string $property
* A property on the source.
* @param mixed $data
* The property value to set on the source.
*
* @throws \Exception
*/
public function setSourceProperty($property, $data) {
This is misleading/incorrect documentation. The source properties can also be modified in other places, such as prepare row. Let's document what phase in the migration process the frozen flag is flipped.
As an example of this getting called, see: https://github.com/heddn/d8_custom_migrate/blob/master/src/Event/Migrate...
Proposed resolution
Update the doc bloc of setSourceProperty adding an @see to SourcePluginBase:next where 'frozen' is set. Also update the doc bloc of SourcePluginBase:next to explain 'frozen'.
Remaining tasks
Comment | File | Size | Author |
---|---|---|---|
#53 | 2579361-53.patch | 4.01 KB | quietone |
#53 | interdiff-48-53.txt | 926 bytes | quietone |
#48 | 2579361-48.patch | 3.99 KB | quietone |
Comments
Comment #2
heddnComment #3
heddnFrom looking at the code, that point seems to be directly after prepareRow. Reference SourcePluginBase->next().
Comment #4
jhodgdonHm... Maybe the thing to do is to say something like "this can't be called if source properties are frozen" and link to the frozen property/method, and then add some documentation there about when "frozen" normally happens?
By the way, which class called Row is this for -- what is the namespace?
Comment #5
heddnThis is in migrate. Documenting when frozen is called is probably a valid strategy.
Comment #11
John.nie CreditAttribution: John.nie commentedBut how to set SourceProperty, the core code is frozen, and can't approach to set custom.
how to approach this purpose?
Comment #12
quietone CreditAttribution: quietone at Acro Commerce commentedLet's tag this for documentation.
Comment #13
quietone CreditAttribution: quietone at Acro Commerce commented@john.lee, let's answer your question in the other issue you opened about this #2974209: Migrate Row is frosen by core, failed to customize property. and leave this one to focus on the documentation.
I will copy your question to that issue.
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedAdding tag so migrate maintainers find this
Comment #18
quietone CreditAttribution: quietone as a volunteer commentedAnything else to add?
Comment #19
quietone CreditAttribution: quietone as a volunteer commentedComment #20
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch#18 and it works fine.
RTBC
Comment #21
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedComment #22
quietone CreditAttribution: quietone as a volunteer commented@Abhijith S, Thanks for the review! I appreciate your confidence in this patch but can you expand why you think this 'works fine'? A good comment would state what you did to review, if this fixes the problem in the IS and if the changes are accurate. And then, if you still think this is RTBC the tests need to run to confirm that this isn't introducing coding standard errors. And a final tip, you can save yourself some time and not upload an image of the file with the patch applied. Any developer can apply the patch locally if they want to, and when the tests are run the patch will be applied before the tests, so that would give proof that that the patch applies. And besides, it would be impractical to create images for large documentation patches.
Thx.
Comment #23
quietone CreditAttribution: quietone as a volunteer commentedThis is more likely to get a review if in the migration system.
Comment #24
quietone CreditAttribution: quietone as a volunteer commentedAnd tag for bug smash. I'll ask for a review in #bugsmash.
Comment #25
daffie CreditAttribution: daffie commentedI understand and agree with the first added line.
Could you explain why the link was added.
Comment #26
LendudeHmm I took a look in
\Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next
and I gotta say it really takes some digging through code to get to the meaning of "if the row is not frozen". The doc block for next() doesn't mention it at all, so if you just want to look at documentation and not have to dig through code, this update of the doc block of setSourceProperty doesn't really help that much.Since the 'freezing' is done in
\Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next
wouldn't it be good to document that there? Which means the current addition to setSourceProperty would be enough. Just a thought....Comment #27
quietone CreditAttribution: quietone as a volunteer commented@daffie, the link was added because next() is where the row is frozen, which is the first thing someone will want to know when reading the comment.
Yes, SourcePluginBase::next takes a bit of reading. I like the idea of expanding the doc for next here as well. Is this sufficient or is more needed. Not running tests until there is agreement on the changes.
Comment #28
Lendude@quietone yeah #27 makes it really clear to me what is going on. Nice!
Comment #29
quietone CreditAttribution: quietone as a volunteer commented@Lendude, thank you.
Updated adding a proposed resolution.
Comment #30
daffie CreditAttribution: daffie commented@quietone: Thank you for your explanation it is now clear to me.
The extra documentation to the docblock of Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next() is also helpful in understanding the added documentation to the docblock of Drupal\migrate:Row::setSourceProperty().
@Lendude also likes the added documentation!
For me it is RTBC.
Comment #31
benjifisherComment #32
alexpottI might be wrong but I think this can be clearer. Does't "row needs an update" mean that "it is above the highwater mark or the source row has changed". So this should be something like:
Also isn't the trackChanges flag involved in all of this too?
Comment #33
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedMade the change suggested in #32. Please review.
Comment #34
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #35
alexpott@ayushmishra206 my review in #32 was more tentative than "make these changes" - I think we need a migrate maintainer to chime in.
Comment #36
quietone CreditAttribution: quietone as a volunteer commentedIt can mean anything. The status of the row can be set to update by custom code or drush, for whatever reason. And the method to set the status to update, idMap->setUpdate is not used in core except in tests. I have added an explanation on how a row is considered changed, which does use track_changes.
There is no interdiff because this patch is so small and no code changes and not running tests until there is agreement on the text.
Comment #37
DamienMcKennaI think it might be useful to be clear on whether this can be used in hook_migrate_prepare_row() to modify the $row object.
Comment #38
benjifisherI have not reviewed the proposed text for accuracy, but I have some suggestions for readability:
I have not re-wrapped the lines, but something like this:
Comment #39
quietone CreditAttribution: quietone as a volunteer commented@benjifisher, thanks that is easier to read no.
Changes for #38 made.
No interdiff because this is so small and not running tests until the text is approved.
Comment #40
quietone CreditAttribution: quietone as a volunteer commentedAdding reference to hook_prepare_row for #37. Also an addition for highwater.
Comment #41
dwwThanks to everyone who worked to improve these docs. Always appreciate having good documentation!
TL;DR: Maybe the new docs should go into the base comment on the class itself, instead of onto
next()
.A) The middle sentence here is still clumsy and needs help (at least different punctuation, possibly re-wording).
Meanwhile, the rest of the docblock (not touched by the patch) for next() includes:
B) Reading the whole thing with the patch applied, it's awkward that the initial paragraph (as expanded here) mentions both the ID map and highwater support, but then the subsequent paragraphs try to introduce those concepts. Seems we either need to incorporate the rest of the existing docs into the updates being done here, or we should talk about ID maps in the context of the rest of the docs about the ID map. Ditto highwater support.
Also, just reading the new docs:
C) This is a comment about a method called "next()". Is that what "the next step" refers to?
D) Naive me: "wow, that seems like a lot of effort on a row that we might not process at all... why isn't the first step to determine if the row will be processed, and only prepare it if it is going to matter?". We're probably documenting how it works, and changing how it works is of course out of scope for this issue. But I wonder if all those details are relevant to document, especially since we might want to change how it behaves.
E) Stepping back: there's a lot of good information being added here, but I wonder if the fine print of the
next()
documentation is really the best spot. Should some/most of these additions go into the base class comment itself, instead ofnext()
?F) Speaking of the base class docs, they mention:
Is that still true? If so, the newly added docs appear to contradict that. At least it's confusing that the new docs are talking about both as if they can both be happening.
G) Generally, don't we put
()
at the end of method names in our comments? E.g. "prepareRow()" not just "prepareRow"?Apologies that this is not a very actionable review. :( I'm raising concerns without providing good alternatives / suggestions for improvement.
Sorry,
-Derek
Comment #42
quietone CreditAttribution: quietone as a volunteer commented@dwww, thank you for the fresh perspective! All good points.
I changed the text in response to all the items in #41. Then I made two patches, one with the changes in the next() method and one with the changes in the class doc. Only the first sentence of the documentation changes in SourcePluginBase should be different.
Which is better?
Comment #43
dwwYay, glad that review was helpful!
I prefer the "class" version, but it's not up to me. ;) I think a more wholistic understanding of how a source plugin works and processes / iterates over the rows is really helpful. Great work!
One remaining nit:
A) Double "the". Also in the other patch (although wrapped differently).
B) hook_prepare_row() -> add the parens?
Also, at the risk of further scope creep:
C) "- the row needs an update"
It'd be fabulous if this updated comment explained what that means. ;) We're defining the conditions that must be true for a row to be updated... by saying the row needs an update. In this context, I assume "row" means "a row in the source data". How can a source data row "need an update"? Isn't that the same as "the source data changed since we last imported it?" (track_changes)? If so, why duplicate it? If not, what's different about this condition?
Otherwise, I think this is really close to RTBC.
Thanks again!
-Derek
Comment #44
quietone CreditAttribution: quietone as a volunteer commentedI have no objection to expanding the scope of this to include the suggestion #43. It is much better to do this all at once than in separate issues.
This patch has fixes for #43 A, B and C. I moved the description of the criteria for the row being processed to sub items. That is definitely a personal bias - I find it easier to skim lists to find what I want/need than to read a paragraph.
I have also gotten off the fence and decided I prefer this documentation in the class doc. Even though next() is doing all this work these are things that a dev needs to know when writing migrations. So, let's make it as visible as possible.
Comment #45
jibran@quietone asked for a review in #bugsmash channel.
"For each row, the map row" sounds confusing. How about changing "map row" to "map record" to avoid confusion?
Should these sentences satrt with capitalized letter? #EnglishIsNotMyFirstLanguage
Let's explain the 'high_water_property' property here just like we explained 'track_changes' below.
s/track_changes/track_changes property/
I think this is an important piece of information we should keep it somewhere.
Comment #46
quietone CreditAttribution: quietone as a volunteer commented@jibran, thanks.
I was surprised to find that this needed a reroll.
1. Interesting, 'map row' is how migrate refers to the corresponding row in the migrate_map table. What could this be confused with? I am not sure. I have modified the sentence and added an @see for MigrateIdMapInterface. Is that better? I am not sure on this one.
2. Fixed
3. Added a sentence here. There is more about the high_water_property in the new 'Available configuration key'. And highwater is added to the api docs in #2944846: Improve description of key concepts in migrate.api.php documentation.
4. Fixed
5. It is not included because it contain false information.. It says the that the highwater field is compared with the last time the migration was executed which is not true.
Comment #47
jibranThank you for addressin the review.
The current value of the high_water_property set on the source plugin is used as a highwater mark.
Comment #48
quietone CreditAttribution: quietone as a volunteer commentedNice! I like it.
This patch makes the change in #47.
Comment #49
jibranThe patch looks good to me.
Comment #50
Wim LeersThis makes it sound like the highwater mark is the value of
high_water_property
that the source plugin instance receives. Which is not the case.I think this would be clearer:
The source plugin tracks the highest encountered value of the processed source rows. It is the source plugin's high_water_property configuration that determines which property in the source row is used for this.
Comment #51
quietone CreditAttribution: quietone as a volunteer commentedNot how I read the sentence but that is why we have multiple reviewers. :-) I think the second sentence in the explanation isn't needed because the high_water_property is in the 'Available configuration keys' section.
What about this?
The highwater mark is the highest encountered value of the property defined by the configuration key high_water_property.
Comment #52
Wim Leers#51++
Comment #53
quietone CreditAttribution: quietone as a volunteer commented@Wim Leers, thanks
Comment #54
jibranBack to RTBC as #50 has been addressed.
Comment #55
Wim Leers👍 Thank you, @quietone & @jibran!
Comment #56
alexpottCommitted and pushed d4c2aa96db to 9.2.x and c0d67dbd42 to 9.1.x. Thanks!
Comment #59
alexpott