Enforce need to sort source data when using highwater marks

Highwater is a tough nut and is a traditional area of bugs. Can we add a test, as we obviously don't have anything that is broken/fixed with this change.

The intent (which was documented for the D7 migrate module, but does not appear to be documented for D8) is that when using highwater marks you need to sort your source data by the field you're using for your highwater. Then, setting the highwater for every row processed guarantees that you don't remigrate stuff, and if the migration is interrupted unexpectedly you don't miss anything. Well, there is a boundary condition if more than one row has the same value in the highwater field - for safety's sake, they'll all be migrated, even if they were before.

For unsorted data, this patch will indeed prevent unnecessary remigration. However, it will cause you to miss rows that *should* be remigrated, if they've changed since the previous migration (i.e., their highwater field's value is higher than the highwater from that migration), if they come after rows with even higher highwater fields.

The general rule of thumb:

1. track_changes is the most reliable method to detect changed rows
2. If your data has a field which reliably increases with each change (practically speaking this is nearly always a timestamp of course) and you can sort by that field, then highwater marks are a more efficient way to detect changed rows.

@mikeryan, thanks. Always love your summaries.

Right, so plugins extending from SqlBase.php do have the orderBy clause automatically added but those extending from SourcePluginBase will not.

I've updated the doc page at https://www.drupal.org/docs/8/api/migrate-api/migrate-api-overview#comme.... The high_water_property doc in SourcePluginBase should at a minimum state that sorted source data is required for highwater marks to work correctly. It might also be good to incorporate the rule of thumb above.

I take it then, we need to roll a new patch for the docs, not necessarily change the code in the way we have it listed in the latest patch?

This issue got me thinking... #2994072: Consolidate migration mechanisms for reimporting changes

I take it then, we need to roll a new patch for the docs, not necessarily change the code in the way we have it listed in the latest patch?

I was going to say yes, but the code change wouldn't hurt - it would actually be a (very slight) performance improvement in that the highwater mark wouldn't get rewritten when you have multiple rows with the same highwater value. And it would make unsorted data break less (which may not be a good thing - you'd be less likely to notice that it was indeed broken). I don't have a strong opinion on this...

This is not a novice issue and it needs an issue summary update.

+++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
@@ -383,7 +383,7 @@ public function next() {
-      if ($this->getHighWaterProperty()) {
+      if ($this->aboveHighwater($row)) {

OMG yes, I wrote about this same problem in
#2859314-21: Highwater condition with unjoined maps skips unprocessed and NEEDS_UPDATE rows:

And since later rows are processed, and it's \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next() that updates the high water mark upon accessing the source, not upon saving the mapping, then any unprocessed rows with high water property values below the high water mark are simply never even considered — they're filtered away in the source plugin's query!

But as you showed here, the problem is even bigger: not only is it saving the high water before a row is processed, it is simply overwriting whatever the previous value was, no matter if it's greater than or lesser than 😨