ContentEntity migration source doesn't consider the migration map

1. The testbot found some coding standards problems. Please fix those (and get some passing tests) before marking an issue NR.

2. It looks as though this issue should be postponed on #3188918: Allow EntityQuery to be converted to the underlying SQL query and #3158436: Give batch_size feature to Drupal\migrate_drupal\Plugin\migrate\source\ContentEntity so it can scale.

3. Why is ContentEntity::mapJoinable() so much simpler than SqlBase::mapJoinable()?

4. Depending on the answer to the previous question, it might make sense to move mapJoinable() to the new trait. Maybe just some of the conditions are common to the two classes, in which case we could move those conditions to a new method in the trait.

5. If we do that, then it would be really easy to add the ignore_map option to SqlBase. I have seen cases where joining the tables causes problems. It might not be in scope for this issue.

6. I think we need some test coverage for the new option.

@anmolgoyal74: I think MR 247 is for the 8.9.x branch. If you look at MR !246, then you should be able to apply my suggestion to make the same change to the trait.

@joachim:

Thanks for the reply.

The SqlBase::mapJoinable() considers highwater marks ...

Why don't we check highwater marks in ContentEntityBase?

If there are two common checks, it is a matter of taste. If there are three common checks, then it is worth adding something to the trait.

Feedback.

a.) I believe there is a simpler approach to enforcing highwater. Retain the original and simpler initial population method of $ids in method initializeIterator. A simple sql query that returns ids is efficient and effective. The key is to do the enforcement at the php script level and not additionally entangle highwater and batchsize in the db sql queries.

https://gist.github.com/chriscalip/17d4f8ac3160d878dd8f6d92d04bc7a0#file...
Is an implementation of highwater enforcement at php level.

b.) Intention.

- drush mim XXX Execution process only-new records.
- drush mim XXX --update Execution process all previously migrated and new records.

https://gist.github.com/chriscalip/17d4f8ac3160d878dd8f6d92d04bc7a0#file...
Admittedly, gist does workaround to enforce intention that there is a difference between intention to process only-new records and intention to process all (previously migrated and new records.). If we can increase scope to include advance handling between only-new and process all via (--update). That would be great.

@joachim

No, loading array of many ids will not cause PHP memory exhaustion instead loading an array of many entities will lead to memory exhaustion. An entity is several magnitudes higher in memory consumption than an integer. The migration system will collapse even earlier before it reaches method initializeIterator if the row count is in the trillions.

Hence

> $ids = $this->query()->execute();
> $ids = filter($ids);
> $ids = subsets($ids);

Will scale, I have seen it myself.

The whole point is to do filtering/subsets on the IDs to a smaller set of IDs. Do you have a counter example that a simple check of

$isHigher = ($highWater <= $target);
// wherein $highwater is the highwater value.
// wherein $target is the intended target row id; could be revision-id or base-id.

will not work?

Per request I added comments on method initializeIterator.

Counter argument accepted. Fair enough.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Restarting this with an isolated patch that uses an alter hook which calls back to the source plugin. I've also not kept the trait, it's just two instances and both the return argument and the optional argument are only needed for SqlBase, making this more complicated and refactoring SqlBase at the same time seems to cause some test fails too. And as shown further down, entity queries have other things to consider.

Test fails show that this has never been used/tested on an entity with translations, as it caused the query to explode due to the missing table alias. That seemed easy enough to fix at first and I was able to get ContentEntitySource green without major issues.

But then I tried to write a test that actually verifies that the join works, and the combination of how source plugins work, translations, entity queries caused my brain to melt.

This turned out to be _hard_. Entity queries essentially ignore that the fact that such a thing as translations exist, match on any data table row they can and will just aggregate multiple matches back to the ids in the result array. First I thought that I can't get it work with included translations, but it turned out that was actually the easier version, the tricky part was when they were not included, then I needed to ensure to only query the default translation or the query still found translations not matching the map table. The test now verifies with 40 (!) permutations of included translation, revision (which doesn't really do much yet, but it will be a fun one to support once revisions as a source actually work, the current state makes no sense to me at all), map join settings and every possible migrate map combination with two languages.

The test verifies both the query result (which essentially returns only entity ids that need to have at least one translation still migrated) and the returned row count (which is then filtered down on the specific translations and always considers the map status).

I like you too phpcs.

Hm, missed cspell, kinda annoying, using the same comment as in SqlBase for that.

It's related to that, yes. But my change here seems to work fine according to the tests.

We have been running our migration for a few days now (yeah, it's slow) and it's been working well from what see so far. We have noticed one bug though, and that is that the join is also applied to the count query, so that the total is essentially just the remaining articles. I think that doesn't really affect the actual migration, but migrate status is confused then.

The fix seems easy enough, just need to skip the method when there are no fields.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.