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
The query condition for highwater is only added when the map is ignored or the map is joinable.
Proposed resolution
Set the $condition_added flag to true in the high water if block. That way the following test on that flag will cause the highwater condition to be added to the query for all cases.
Remaining tasks
Make a follow to check if this patch will break update migrations. See #21.
User interface changes
N/A
API changes
N/A
Data model changes
N/A
---
The original IS:
Sorry folks I have no enough time to document everything right.
Here:
https://github.com/drupal/drupal/blob/8.2.x/core/modules/migrate/src/Plu...
— the condition is not really added since `$condition_added` is not set to `TRUE`. It would e.g. on a local database (see the line 197), but if the source database is not 'joinable' `$condition_added` just gets no chances.
The patch:
diff --git a/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
index 0eac141..6bf48b4 100644
--- a/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
+++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
@@ -216,6 +216,7 @@ protected function initializeIterator() {
$high_water_field = $this->getHighWaterField();
$conditions->condition($high_water_field, $high_water, '>');
$this->query->orderBy($high_water_field);
+ $condition_added = TRUE;
}
if ($condition_added) {
$this->query->condition($conditions);| Comment | File | Size | Author |
|---|---|---|---|
| #29 | interdiff.txt | 3.55 KB | quietone |
| #29 | highwater_condition-2824267-29.patch | 8.67 KB | quietone |
| #25 | highwater_condition-2824267-23.patch | 6.43 KB | quietone |
| #23 | highwater_condition-2824267-23.patch | 6.43 KB | shashikant_chauhan |
| #16 | interdiff.txt | 693 bytes | quietone |
Comments
Comment #2
OnkelTem CreditAttribution: OnkelTem commentedComment #3
iMiksuI've updated it back to active as we need to provide a proper patch so that we can let bots to run the tests. See Making a Drupal patch with Git.
Novice task: Can anyone provide the patch file that @OnkelTem has described in summary?
Comment #4
iMiksuAlso we need steps to reproduce the problem.
Comment #5
quietone CreditAttribution: quietone as a volunteer commentedBumped into this problem while working on #2807875: Convert Comment's Migrate source tests to new base class, see 2) in #12
Comment #6
mikeryanCan we get a fail test demonstrating the problem?
That test will not be a novice task...
Comment #7
faline CreditAttribution: faline at CI&T commentedCreate a patch file that @OnkelTem described in summary.
Comment #9
quietone CreditAttribution: quietone as a volunteer commentedWell, here is an attempt at a fail patch. The problem is it still fails with the fix! And just like #7 it fails on asserting the expected count with plugin count. And the failing test in #7, CommentSourceWithHighWaterTest and this one, HighWaterNotJoinableTest, use different test classes. CommentSourceWithHighWaterTest uses MigrateSqlSourceTestCase and #9 uses MigrateSqlSourceTestBase.
It seems that CommentSourceWithHighWaterTest executes SqlBase::initializeIterator and setups the query correctly to return a count of 1, but something changes that to 0. And then HighWaterNotJoinableTest does not execute SqlBase::initializeIterator so the count will always not take into account the high water mark. This is confusing to me.
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedResults as expected. Removing novice tag.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedChanged the expected count test to use iterator_count.
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedNow we see that the high water value was not initialized properly in MigrateSqlSourceTestCase. Well it was, until #2485385: Move highwater field support to the source plugin, and do not expose its internals on MigrationInterface was committed. With that fixed, this should pass tests now.
Comment #18
mikeryanComment #19
mikeryanSo many test LOC for a one-line fix!
Looks good to me...
Comment #20
mikeryanComment #21
ohthehugemanatee CreditAttribution: ohthehugemanatee at Amazee Labs commentedThis patch will break update migrations, I think.
Update migrations work by marking everything in the map as needs_update, joining the map to the source, and adding a condition to the source query to include anything with needs_update set. With this patch, if you run an update migration that has a highwater mark set, you will exclude all the rows below the highwater mark.
Comment #22
alexpottNeeds a reroll and #21 needs answering. Also the issue summary update does not appear to have been done.
Comment #23
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedupdated the patch.
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedThis should be fixed. Unfortunately, it will highlight an existing problem - #2844595: SQLBase breaks GROUP BY queries
Comment #25
quietone CreditAttribution: quietone as a volunteer commentedWorking on the items in #22. The reroll has been done and setting to NR for testing. The IS has been done.
The remaining item is to address that concern that this will break update migrations. See #21. For that, can we open a follow up?
Comment #27
quietone CreditAttribution: quietone as a volunteer commentedSetting to NW due to #21.
Comment #28
quietone CreditAttribution: quietone as a volunteer commentedRE #21.
I don't think so. The highwater condition is included in an OR condition group. The final query created complies with the comment in SqlBase::initializeIterator.
Comment #29
quietone CreditAttribution: quietone as a volunteer commentedNow with a test for highwater when rows are marked for update.
Comment #30
ohthehugemanatee CreditAttribution: ohthehugemanatee at Amazee Labs commentedWell blow me over with a feather. :)
When I tested by hand as a part of #2841262: Highwater only modifies the Sql query when map table is joinable, including --update in the drush command didn't prompt a review of all nodes. The test looks good to me, though, and clearly comes back green.
What I don't understand, is how does this work when the map is NOT joinable? If mapJoinable() returns FALSE, the query conditions look like: "(original conditions) AND (above high water)." How do we know which rows have NEEDS_UPDATE set?
In any case I think it's safe to split this off into a separate issue if it is even a real problem.
Comment #31
phenaproximaI like the change to MigrateSqlSourceTestBase. Looks great. Was meaning to do that myself all along, but was waiting for a use case. And here it is. Full steam ahead! You have my blessing.
Comment #32
mikeryanComment #33
mikeryanI think you're right - we only want to add the highwater condition when we're joining to the map, otherwise we need to fallback to the SourcePluginBase iterating the full query and testing NEEDS_UPDATE and highwater marks as it goes. Might be as simple as just moving the highwater condition inside the joinable condition...
I'm going back-and-forth on doing that here versus a follow-up, thoughts?
Comment #34
quietone CreditAttribution: quietone as a volunteer commentedSince this is a blocker, I go with followup.
Comment #35
mikeryanFollowup created: #2859314: Highwater condition with unjoined maps skips unprocessed and NEEDS_UPDATE rows. This patch looks good to me for its scope.
Comment #36
alexpottCommitted and pushed 8fa8911 to 8.4.x and bd6b010 to 8.3.x. Thanks!
Comment #40
Wim LeersThe problem that @ohthehugemanatee identified in #30 is much more severe than it was made out to be in this issue. See #2859314-20: Highwater condition with unjoined maps skips unprocessed and NEEDS_UPDATE rows and subsequent comments.