SqlBase::prepareQuery() should be called also on count

Attached is an equivalent fix for D7.

I do wonder if the preExecute() should be done on the original query before clone is called, so that the hook_query_tag_alter()'s get called once instead of twice, when the same query is used for both a count and an execute... But the D9 version does it here in the counting, so I've kept it the same here.

I did a rebase of the MR. I'm not sure what are the actionable next steps here. From reading #46, there's a whole rework desired. What is the MVP?

My most recent visit to this issue is from ✨ SQL source plugins: allow defining conditions and join in migration yml Needs work , which is the nearly the number #1 asked for feature for migrate.module right now. I'd hate to block real progress on a lot of nice to haves.

On second thought, we'll also want to post a CR for this. I'll do that after I get a review.

We discussed this issue at 📌 [meeting] Migrate Meeting 2025-01-30 2100Z Active . We agreed that the changes suggested in Comment #46 should be done in a separate issue, so I am adding the issue tag for that and I am starting to review the MR here.

The MR looks good to me. The test coverage is great, but I have a few suggestions to polish it a little.

I ran the test locally (with my suggested changes) and it passed. When I reverted the changes to SqlBase, the test failed as expected.

@heddn, I think this issue is close enough to RTBC that you can go ahead and draft the change record.

I added 📌 Improve separation of concerns in SqlBase Active , so I am removing the tag for a followup issue.

CR added and feedback on MR addressed.

@heddn:

CR added and feedback on MR addressed.

Thanks for that. I made a few edits to the change record, and I reviewed the changes. LGTM

Then I remembered the T in RTBC.

I decided that I should do something close to a real-world test. I installed Drupal 7 and created 50 nodes with the devel_generate module. I checked the D7 database: there are 27 Article nodes, 9 of which have nid less than 17.

I set up a D11 site to connect to the D7 site: in settings.php,

$settings['migrate_node_migrate_type_classic'] = FALSE;
$databases['migrate']['default'] = [
  'database' => 'db',
  'username' => 'db',
  'password' => 'db',
  'host' => 'ddev-drupal7-db',
  'driver' => 'mysql',
];

I enabled the migrate_drupal and mymigration modules, including this implementation of hook_query_TAG_alter():

function mymigration_query_migrate_alter(AlterableInterface $query) {
  if ($query->hasTag('migrate_d7_node:article')) {
    $query->condition('n.nid', 17, '<');
  }
}

With 11.x, I see all 27 Article nodes:

benji@drupal-web:/var/www/html$ drush cr && drush ms d7_node:article
 [success] Cache rebuild complete.
 ----------------- -------- ------- ---------- ------------- --------------- 
  Migration ID      Status   Total   Imported   Unprocessed   Last Imported  
 ----------------- -------- ------- ---------- ------------- --------------- 
  d7_node:article   Idle     27      0          27                           
 ----------------- -------- ------- ---------- ------------- ---------------

With the feature branch, I see just 9:

benji@drupal-web:/var/www/html$ drush cr && drush ms d7_node:article
 [warning] include_once(/var/www/html/core/modules/datetime/datetime.module): Failed to open stream: No such file or directory Extension.php:160
 [warning] include_once(): Failed opening '/var/www/html/core/modules/datetime/datetime.module' for inclusion (include_path='/var/www/html/vendor/pear/archive_tar:/var/www/html/vendor/pear/console_getopt:/var/www/html/vendor/pear/pear-core-minimal/src:/var/www/html/vendor/pear/pear_exception:.:/usr/share/php') Extension.php:160
 [success] Cache rebuild complete.
 ----------------- -------- ------- ---------- ------------- --------------- 
  Migration ID      Status   Total   Imported   Unprocessed   Last Imported  
 ----------------- -------- ------- ---------- ------------- --------------- 
  d7_node:article   Idle     9       0          9                            
 ----------------- -------- ------- ---------- ------------- --------------- 

The warning goes away if I clear caches a second time. It is probably because the feature branch does not have the commit for 📌 Deprecate views_field_default_views_data Active .

I can now honestly say this issue is RTBC.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I rebased and resolved the merge conflict.

The only conflict was that this issue added a missing @return comment, and a different (more generic) one was added in 📌 Fix Drupal.Commenting.FunctionComment.MissingReturnComment in non-tests Needs review . I kept the comment added in this issue.

Back to RTBC.

This change feels correct but I think we need a follow-up to somehow make query protected in a future version of Drupal as nothing should be calling it outside the class.

I think we can change it to abstract protected function query(); and then somehow detect classes where the implementation is public. But this is all follow-up as far as I can see.

I rebased the branch on 11.x to see if we can get a green test run.

@alexpott:

Maybe we can make query() protected as part of 📌 Improve separation of concerns in SqlBase Active .

The tests still pass.

Committed d928672 and pushed to 11.x. Thanks!

Doesn't backport to 11.1.x cleanly and has a CR so leaving in 11.x and this will be released in 11.2

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