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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

heddn’s picture

Issue summary: View changes
heddn’s picture

From looking at the code, that point seems to be directly after prepareRow. Reference SourcePluginBase->next().

jhodgdon’s picture

Hm... 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?

heddn’s picture

This is in migrate. Documenting when frozen is called is probably a valid strategy.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

John.nie’s picture

But how to set SourceProperty, the core code is frozen, and can't approach to set custom.
how to approach this purpose?

quietone’s picture

Issue tags: +API Documentation

Let's tag this for documentation.

quietone’s picture

@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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Issue tags: +migrate

Adding tag so migrate maintainers find this

quietone’s picture

Anything else to add?

quietone’s picture

Status: Active » Needs review
Abhijith S’s picture

FileSize
53.32 KB

Applied patch#18 and it works fine.
after

RTBC

Abhijith S’s picture

Status: Needs review » Reviewed & tested by the community
quietone’s picture

Status: Reviewed & tested by the community » Needs review

@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.

quietone’s picture

Component: documentation » migration system

This is more likely to get a review if in the migration system.

quietone’s picture

Issue tags: +Bug Smash Initiative

And tag for bug smash. I'll ask for a review in #bugsmash.

daffie’s picture

Status: Needs review » Needs work

I understand and agree with the first added line.

+++ b/core/modules/migrate/src/Row.php
@@ -177,6 +177,8 @@ public function getSource() {
+   * @see \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next

Could you explain why the link was added.

Lendude’s picture

Hmm 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....

quietone’s picture

Status: Needs work » Needs review
FileSize
1.63 KB

@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.

Lendude’s picture

@quietone yeah #27 makes it really clear to me what is going on. Nice!

quietone’s picture

Issue summary: View changes

@Lendude, thank you.

Updated adding a proposed resolution.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

@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.

benjifisher’s picture

Version: 8.9.x-dev » 9.2.x-dev
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -335,7 +335,10 @@ public function rewind() {
+   * the system. A row will be processed if it has not already been imported,
+   * or the row needs an update, or it is above the highwater mark or the source
+   * row has changed. When set to be processed the row is also marked frozen
+   * and no further changes to the row source properties are allowed.

I 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:

+   * the system. A row will be processed if it has not already been imported or 
+   * it needs an update. Rows need an update when they are above the highwater
+   * mark or the source row has changed. When set to be processed the row is
+   * also marked frozen and no further changes to the row source properties are
+   * allowed.

Also isn't the trackChanges flag involved in all of this too?

ayushmishra206’s picture

Made the change suggested in #32. Please review.

ayushmishra206’s picture

Status: Needs work » Needs review
alexpott’s picture

@ayushmishra206 my review in #32 was more tentative than "make these changes" - I think we need a migrate maintainer to chime in.

quietone’s picture

FileSize
1.79 KB

Does't "row needs an update" mean that "it is above the highwater mark or the source row has changed".

It 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.

DamienMcKenna’s picture

I think it might be useful to be clear on whether this can be used in hook_migrate_prepare_row() to modify the $row object.

benjifisher’s picture

Status: Needs review » Needs work

I have not reviewed the proposed text for accuracy, but I have some suggestions for readability:

  1. Set off the cases as a bulleted list.
  2. Change "which" to "that".
  3. Add a comma after "When set to be processed".

I have not re-wrapped the lines, but something like this:

   * The migration iterates over rows returned by the source plugin. This
   * method determines the next row that will be processed and imported into
   * the system. A row will be processed in any of these cases:
   * - the row has not already been imported
   * - the row needs an update
   * - the row is above the highwater mark
   * - the source row has changed
   * A row is considered changed only if track_changes is set
   * on the source plugin and the source values for the row have changed since
   * the last import. When set to be processed, the row is also marked frozen and
   * no further changes to the row source properties are allowed.
   *
   * The method tracks the source and destination IDs using the ID map plugin.
quietone’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

@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.

quietone’s picture

FileSize
2.18 KB

Adding reference to hook_prepare_row for #37. Also an addition for highwater.

dww’s picture

Thanks 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().

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -333,9 +333,20 @@ public function rewind() {
+   * The migration iterates over rows returned by the source plugin. For each
+   * row, the map row, if it exists is deleted and then the prepareRow method
+   * and hook_prepare_row are executed. The next step is to determine if the row
+   * will be processed.

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:

* The method tracks the source and destination IDs using the ID map plugin.
*
* This also takes care about highwater support. Highwater allows to reimport
* rows from a previous migration run, which got changed in the meantime.
* This is done by specifying a highwater field, which is compared with the
* last time, the migration got executed (originalHighWater).

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:

* The migration iterates over rows returned by the source plugin. For each
* row, the map row, if it exists is deleted and then the prepareRow method
* and hook_prepare_row are executed. The next step is to determine if the row
* will be processed.

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 of next()?

F) Speaking of the base class docs, they mention:

* The high_water_property and track_changes are mutually exclusive.

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

quietone’s picture

@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?

dww’s picture

Yay, 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:

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -15,6 +15,25 @@
+ * the the prepareRow() method, which also invokes hook_prepare_row. The row is

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

quietone’s picture

FileSize
3.7 KB

I 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.

jibran’s picture

@quietone asked for a review in #bugsmash channel.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -15,6 +15,32 @@
    + * plugin. For each row, the map row, if it exists, is deleted before allowing
    

    "For each row, the map row" sounds confusing. How about changing "map row" to "map record" to avoid confusion?

  2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -15,6 +15,32 @@
    + * - the row has not already been imported
    ...
    + * - the row needs an update
    ...
    + * - the row is above the highwater mark
    + * - the source row has changed
    

    Should these sentences satrt with capitalized letter? #EnglishIsNotMyFirstLanguage

  3. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -15,6 +15,32 @@
    + * - the row is above the highwater mark
    

    Let's explain the 'high_water_property' property here just like we explained 'track_changes' below.

  4. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -15,6 +15,32 @@
    + *   - A row is considered changed only if track_changes is set on the source
    

    s/track_changes/track_changes property/

  5. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -332,17 +359,6 @@ public function rewind() {
    -   * This also takes care about highwater support. Highwater allows to reimport
    -   * rows from a previous migration run, which got changed in the meantime.
    -   * This is done by specifying a highwater field, which is compared with the
    -   * last time, the migration got executed (originalHighWater).
    

    I think this is an important piece of information we should keep it somewhere.

quietone’s picture

FileSize
2.55 KB
3.95 KB

@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.

jibran’s picture

Thank you for addressin the review.

  1. This reads well now.
  2. Thanks.
  3. Please see the suggestion below.
  4. Thanks.
  5. Thank you for clearing it up.
+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -15,6 +15,34 @@
+ *   - The highwater mark is the current value of the high_water_property.

The current value of the high_water_property set on the source plugin is used as a highwater mark.

quietone’s picture

Nice! I like it.
This patch makes the change in #47.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good to me.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -15,6 +15,35 @@
+ *   - The current value of the high_water_property set on the source plugin is
+ *     used as a highwater mark.

This 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.

quietone’s picture

Not 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.

Wim Leers’s picture

#51++

quietone’s picture

@Wim Leers, thanks

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as #50 has been addressed.

Wim Leers’s picture

👍 Thank you, @quietone & @jibran!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed d4c2aa96db to 9.2.x and c0d67dbd42 to 9.1.x. Thanks!

  • alexpott committed d4c2aa9 on 9.2.x
    Issue #2579361 by quietone, ayushmishra206, heddn, jibran, Wim Leers,...

  • alexpott committed c0d67db on 9.1.x
    Issue #2579361 by quietone, ayushmishra206, heddn, jibran, Wim Leers,...
alexpott’s picture

Version: 9.2.x-dev » 9.1.x-dev

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.