Clean up the node_uid_revision handlers

Completely wrong patch.

The last submitted patch, 4: 3341387_3.patch, failed testing. View results →

Better comments.

Great improvements! Only minor concerns:

1. +++ b/core/modules/node/src/NodeViewsData.php
   @@ -183,8 +183,8 @@ public function getViewsData() {
   -    $data['node_field_data']['uid_revision']['title'] = $this->t('User has a revision');
   -    $data['node_field_data']['uid_revision']['help'] = $this->t('All nodes where a certain user has a revision');
   +    $data['node_field_data']['uid_revision']['title'] = $this->t('Edited by');
   +    $data['node_field_data']['uid_revision']['help'] = $this->t('All nodes where a certain user authored a revision');
   

   I appreciate the appeal of "fixing" this while we're at it, but I fear:

   • This opens us up to needing usability review and all the delays that can entail.
   • This now becomes a translation-breaking change, which limits backportability.

   So I vote we move this hunk to a follow-up to avoid delaying the fixes / improvements provided by the rest of the patch.

2. +++ b/core/modules/node/src/Plugin/views/UidRevisionTrait.php
   @@ -0,0 +1,36 @@
   +   * @param array $uids
   +   *   A list of user ids.
   ...
   +    $args = array_values($uids);
   

   If $uids is already an array of user ids, why do we need the array_values() here?

3. +++ b/core/modules/node/src/Plugin/views/UidRevisionTrait.php
   @@ -0,0 +1,36 @@
   +   * @param $group
   

   Don't we need a type here?

4. +++ b/core/modules/node/src/Plugin/views/UidRevisionTrait.php
   @@ -0,0 +1,36 @@
   +   *   See \Drupal\views\Plugin\views\query\Sql::addWhereExpression() $group.
   

   I'm not sure that's gonna fly. ;) Definitely an elegant solution, and better than duplicating docs. But I think we're supposed to format @see a specific way, etc...

5. +++ b/core/modules/node/src/Plugin/views/UidRevisionTrait.php
   @@ -0,0 +1,36 @@
   +  public function uidRevisionQuery(array $uids, $group = 0) {
   

   Probably wants a : void return type. Can't we typehint the $group param, too?

6. +++ b/core/modules/node/src/Plugin/views/UidRevisionTrait.php
   @@ -0,0 +1,36 @@
   +    $this->query->addWhereExpression($group, "$this->tableAlias.uid IN ($placeholder_1) OR
   +      EXISTS (SELECT 1 FROM {node_revision} nr WHERE nr.revision_uid IN ($placeholder_2) AND nr.nid = $this->tableAlias.nid)", [
   +        $placeholder_1 => $args,
   +        $placeholder_2 => $args,
   +      ]);
   
   +++ b/core/modules/node/src/Plugin/views/argument/UidRevision.php
   @@ -13,10 +14,10 @@
   -    $this->query->addWhereExpression(0, "$this->tableAlias.uid = $placeholder OR ((SELECT COUNT(DISTINCT vid) FROM {node_revision} nr WHERE nr.revision_uid = $placeholder AND nr.nid = $this->tableAlias.nid) > 0)", [$placeholder => $this->argument]);
   

   OMG yes. This is so much better of a query!

That said, I'm not convinced this is a bug per se. Two strong clues this is really a task:

1. Title starts with "clean up"
2. Summary includes "nothing is broken as we have passing tests"

That said, the original query is so non-performant, that we could consider this a performance bug, even if what we have is technically correct. At least tagging for performance, since that's the main point.

Since this is tweaking low-level DB queries, queued up test runs on pgsql + sqlite, too.

Thanks again!
-Derek

The initial fails ( pgsql 10.12 → and sqlite → ) were random fails in MediaLibraryTest, requeued.

Nonetheless, this patch needs work for at least points 1 and 5, if not 2-4 from above.

Thanks again,
-Derek

Great, the re-tests passed on all DBs. So, just some minor cleanup here and we should be RTBC.

Sorry, forgot to tag this when I first saw it as a bug...

To be completely fair, I don't have the slightest idea how on earth does this not blow up: the repeated placeholder usage should, by all means, blow it up. That's why I marked it as a bug. That we have a passing test is a mystery.