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.

Rolled #10 into an MR without the change to NodeViewsData as per #11.1

Can the IS be updated to include the attention of a new trait?

Will there be a scenario anyone would use this trait outside node? Just wonder if a CR is needed

Also since this is in the views folder of node plugin should view be in the name?

Will there be a scenario anyone would use this trait outside node

No, the query hardcodes node tables

Also since this is in the views folder of node plugin should view be in the name?

I don't think so, it matches the class names of the argument and filter.

Ok I’ll leave in review for others as not 100% on the name

been a few weeks with no additional eyes. Going to mark as the only comment I had left was the name but not a hill to die on for me.