- @joachim opened merge request.
- Status changed to Needs work
over 1 year ago 9:57pm 18 March 2023 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.
- First commit to issue fork.
- π¬π§United Kingdom joachim
It looks like β¨ Allow EntityQuery to be converted to the underlying SQL query Needs work is not going to get in.
We can maybe do this by decorating the entity query factory instead, the way workspaces module does it?
- π¬π§United Kingdom joachim
Unfortunately, doing it the way Workspaces module does it isn't going to be possible, because of the way Workspaces module does it: π Workspace QueryFactory alters queries in a way that's not compatible with any other module doing the same Active
- π¬π§United Kingdom joachim
New plan is to use a query tag and hook_query_alter().
- Status changed to Needs review
over 1 year ago 10:29pm 26 May 2023 - last update
over 1 year ago Custom Commands Failed - π¨πSwitzerland berdir Switzerland
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 need 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).
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,441 pass - π¨πSwitzerland berdir Switzerland
Hm, missed cspell, kinda annoying, using the same comment as in SqlBase for that.
- π¬π§United Kingdom joachim
> 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
Is that related to my comment here: #2942948: Add allTranslations() and allTranslations()->count to entity query β ?
- last update
over 1 year ago Custom Commands Failed - π¨πSwitzerland berdir Switzerland
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.
- Status changed to Needs work
over 1 year ago 10:24am 8 June 2023 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.