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

+++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
@@ -232,8 +241,9 @@ protected function initializeIterator() {
-  public function count() {
-    return $this->query()->countQuery()->execute()->fetchField();
+  public function count($refresh = FALSE) {

This looks like it introduces the $refresh parameter which is unused, but this is in fact also present on the parent class.

#2598670: Source count caching does not work for SQL sources will remove count() in favor of doCount() (where this would go).

Basically the patch looks good, but it needs tests (a test-only patch to demonstrate the problem would be good).

1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
   @@ -232,8 +241,9 @@ protected function initializeIterator() {
   +    $query = clone $this->prepareQuery();
   

   Do we really need to clone the prepared query?

2. +++ b/core/modules/migrate/tests/modules/migrate_source_test/src/Plugin/migrate/source/TestSqlSource.php
   @@ -0,0 +1,58 @@
   +    $configuration = ['key' => 'default', 'target' => 'default'];
   

   += or array_merge would be more flexible, should we want to reuse this test plugin with configuration in the migration definition (or, we could just pass the key/target in the migration definition and dispense with this constructor entirely).

3. +++ b/core/modules/migrate/tests/modules/migrate_source_test/src/Plugin/migrate/source/TestSqlSource.php
   @@ -0,0 +1,58 @@
   +    if (!$this->isQueryPrepared) {
   

   This will always be TRUE after parent::prepareQuery(), no? isQueryPrepared should be captured before the parent call and used after it...

4. +++ b/core/modules/migrate/tests/src/Kernel/SqlSourceTest.php
   @@ -0,0 +1,78 @@
   +    // migrated. After the migration run, this method would return only records
   +    // that need update.
   

   No, count() should always return the total number of source records, regardless of whether the migration has been run.

I came from #3069776: SQL source plugins: allow defining conditions and join in migration yml and just want to attach a one-line patch to see if anything is broken.
I believe many things have changed since 8.4.x, but if it looks obviously wrong - please let me know.

Attaching a test-only patch (modified version of #17) and another one, with the fix.
Merge request will contain the same changes as a complete patch.

This needed a rebase, but there were no conflicts. So there's no code of mine in what I pushed. I closely reviewed https://git.drupalcode.org/project/drupal/-/merge_requests/968/diffs and while I could maybe quibble about a few pedantic docs nits, it doesn't seem worth the trouble of improving them and/or having to get someone else to RTBC this. Assuming the bot is happy, I think this is ready.

Bot's happy. There's expanded test coverage to show that doCount() didn't used to respect what happens in prepareQuery(). This is blocking #3069776: SQL source plugins: allow defining conditions and join in migration yml which would be a big win if we got it done. As I said, we could maybe nitpick some comments here and there, but I don't think we need to hold this up any longer.

As Gitlab MR URLs are subject to unexpected changes (ref #3204538), I'm attaching a patch copy of MR 968 as at 2bdd5263 (comment 33).

Noting that I brought this up at Migrate Meeting 2022-03-10 2100Z because I am cautious about changes to the count. quietone and mikelutz were in attendance and agreed to this change. Also, mikeryan, migrate guru and former migrate maintainer, agreed in #6.

We should get this onto 10.0.x. Setting to NW for rebasing the MR.

Only core committers can change the metadata for Gitlab MRs. So while I could rebase and push the branch, Gitlab will still think it's trying to merge it to 9.4.x until @Matroskeen or a core committer gets involved. I'm happy to do the rebase/push if someone else wants to do the Gitlab shenanigans...

No problem, I just changed the target branch.
I'm sorry I couldn't find time today to do the actual rebase.

Thanks @Matroskeen! No worries!! I know you've got much bigger concerns these days (like surviving an imperialist invasion of your country) than any of these issues. I'll do the rebase right now.

Done. https://git.drupalcode.org/project/drupal/-/merge_requests/968/commits is now only showing the 2 commits for this issue to be merged to 10.0.x.

Bot is happy. As I understand it, #37 was in support of RTBC after rebase, so restoring that status. Still not my code, just the results of git rebase -i 10.0.x, removing everything that wasn't from this issue, and a git push -f.

So should \Drupal\migrate\Plugin\migrate\source\SqlBase::__toString() also call prepareQuery?

I also think that this means we should document that query() should never really be called by anything other than prepareQuery(). It is a shame it is a public function. The \Drupal\migrate\Plugin\migrate\source\SqlBase::query() completely lacks documentation that I feel should be improved here.

There are good points in #44, that I was trying to address in the last commits. I'm bad at writing the documentation but I made an attempt, and hopefully will receive some suggestions from other people.

I remind everyone that this is a blocker for #3069776: SQL source plugins: allow defining conditions and join in migration yml, so I would appreciate your reviews.
Thank you!

I think we should go further here in cleaning this up, defining roles of the various query methods, fixing documentation, and removing ambiguity. It seems clear to me that originally, having query tag alters adding conditions after prepareQuery wasn't really considered. In retrospect, that's a big oversight, as the most useful thing to do with a tagged query is alter it to add conditions or filters. While prepareQuery() was made with the intention that it wasn't going to add anything that would affect the count, clearly by adding tags, it has the potential to, so at minimum, the tags need to be added to the count.

We also do a bunch of stuff to the query in initializeIterator that we don't want reflected in the count. filtering rows that have been migrated, managing highwater sorts, ect. These things should filter the source down to rows we actually want to migrate this particular run, without reducing the actual count of total rows.

At the end of the day, we need two different queries, a count query and an actual migration query. I think we should have some sort of prepare or alter function for each to explicitly define what goes where. We can bikeshed naming and formats, but I think we should either add a prepareCountQuery() method to prepare the count query, and leave prepareQuery to prepare the migration query, or deprecate prepareQuery and add prepareCountQuery and prepareMigrationQuery, and then document the heck out of them. in the base class, prepareMigrationQuery will likely call prepareCountQuery, and then alter the query from there, but this leaves the option for an extending class to separate them further if necessary without having to override doCount to do custom count stuff. This also avoids and BC issues where someone may have extended and put stuff in prepareQuery that they didn't want reflected in the count (stuff that probably should have gone into a custom initializeIterator instead, but you can't currently add stuff to the iterator query without duplicating the entire method, so its often easier to put it into prepareQuery())

Going this route gives us a new place to put count changing query alters that don't have potential BC breakage, and core can move the adding of tags and metadata into there as a bugfix. We can also move most, if not all of the query altering in initializeIterator into a separate method that can be tacked on to or overridden completely without needing to duplicate the entire initializeIterator method. query() can stay public, though its raw state is likely only useful to the two prepare methods, it should be documented that it is unlikely to be useful otherwise. As far as __toString, I'm not sure if that should be the original query, the count query or the final migration query. Perhaps it should remain as just the original query for historical/bc reasons. If the prepare methods are public, it's easy enough to get the string from those two if you needed it.

As Gitlab MR URLs are subject to unexpected changes (ref #3204538), I'm attaching a patch copy of MR 968 as at 6d4a063a (comment 45).

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 #3069776: SQL source plugins: allow defining conditions and join in migration yml, which is 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 #3500283: [meeting] Migrate Meeting 2025-01-16 1400Z. 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 #3500698: Improve separation of concerns in SqlBase, 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 #3489415: Deprecate views_field_default_views_data and related functions.

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 #3489416: Fix Drupal.Commenting.FunctionComment.MissingReturnComment in non-tests. 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 #3500698: Improve separation of concerns in SqlBase.

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