SQL error on revision export from view

I created a patch to fix this issue.

The same error on 8.3.7
Patch work!

So this seems like a pretty major bug (we cannot serve a revisions endpoint in REST w/o this). Is anything being done to fix/incorporate this? Or is there a parent issue somewhere covering this? Why is it even "npr" in the first place?

Any updates?

#11 (#4) Worked here. I thought I had messed up my contextual filter. After looking into the logs I came across the error that led me here.

Patch #11 passes on the latest build. Can we get this RTBC soon?

What are the next steps to get this into the next patch release?

Nice bug find! Yep the query is obviously wrong but I think we need to ask ourselves what is this code really doing?

  /**
   * Override the behavior of title(). Get the title of the revision.
   */
  public function titleQuery() {
    $titles = [];

    $results = $this->database->query('SELECT nr.vid, nr.nid, npr.title FROM {node_revision} nr WHERE nr.vid IN ( :vids[] )', [':vids[]' => $this->value])->fetchAllAssoc('vid', PDO::FETCH_ASSOC);
    $nids = [];
    foreach ($results as $result) {
      $nids[] = $result['nid'];
    }

    $nodes = $this->nodeStorage->loadMultiple(array_unique($nids));

    foreach ($results as $result) {
      $nodes[$result['nid']]->set('title', $result['title']);
      $titles[] = $nodes[$result['nid']]->label();
    }

    return $titles;
  }

Why are we loading the default revisions of a node and then setting the title to the revision's title and returning an array with the result of ->label(). This code looks super odd.

This is what the equivalent code in Drupal 7 is doing:

  /**
   * Override the behavior of title(). Get the title of the revision.
   */
  public function title_query() {
    $titles = array();

    $result = db_query("SELECT n.title FROM {node_revision} n WHERE n.vid IN (:vids)", array(':vids' => $this->value));
    foreach ($result as $term) {
      $titles[] = check_plain($term->title);
    }
    return $titles;
  }

I think the correct fix here is to using the entity query for node and query against all revisions. I think you can use the aggregate entity query trick to get it to return all the titles without fully loading them.

This is something of interest for my application, any update on when this will be released will be helpful.

Great work on this patch.

1) The patch works.

2) Has tests to prove that the patch is indeed fixing something.

I just applied it to Drupal core v8.8.5 and it fixes the same issue that was documented above.

still using this patch, now on 8.8.8

Addressing #18 and resolving failed test cases from #11 for latest dev branch

+++ b/core/modules/node/src/Plugin/views/argument/Vid.php
@@ -37,15 +29,11 @@ class Vid extends NumericArgument {
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, Connection $database, NodeStorageInterface $node_storage) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, NodeStorageInterface $node_storage) {

I'm curious how (if?) we want to handle BC for this? Anything extending this and injecting extra services will throw a fatal error.
Maybe make $database optional, set this to NULL in create and check if its not NULL in the constructor and if it's not NULL we trigger_error()?

Other than that, this looks good.

1. Added The \Drupal\node\Plugin\views\argument\Vid constructor parameter $database is deprecated
2. Reorder parameters. Optional parameter should be provided after required.

+++ b/core/modules/node/src/Plugin/views/argument/Vid.php
@@ -37,15 +37,18 @@ class Vid extends NumericArgument {
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, Connection $database, NodeStorageInterface $node_storage) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, NodeStorageInterface $node_storage, Connection $database = NULL) {

Since anything extending this will pass the variables in the other order, won't this still lead to a fatal error?

Improved BC.

Added follow-up #3178818: Remove deprecation layer and add argument type to the \Drupal\node\Plugin\views\argument\Vid constructor.

needs work for todo code style

1. +++ b/core/modules/node/src/Plugin/views/argument/Vid.php
   @@ -41,20 +41,18 @@ class Vid extends NumericArgument {
   +   * @todo https://www.drupal.org/project/drupal/issues/3178818 Remove
   +   *   deprecation layer and add argument type to $node_storage.
   

   needs to be inline with https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

2. +++ b/core/modules/node/src/Plugin/views/argument/Vid.php
   @@ -41,20 +41,18 @@ class Vid extends NumericArgument {
   +      @trigger_error('Calling ' . __CLASS__ . '::__construct() with the $database parameter is deprecated in drupal:9.2.0 and will be removed in drupal:10.0.0. See https://www.drupal.org/node/3178412', E_USER_DEPRECATED);
   

   could use __METHOD__ . '()

Worked on comment #35

#36 Works for me! Fixed my issue when trying to use a node's revision ID as a contextual filter on a node revision based view.

Tested #36 on 9.1
Its working fine for me as well
RTBC

Tested #36 on 8.9.6 and it works fine for me too! I used in addition to this patch that provides the Contextual filter "Revision ID from URL".
RTBC

1. +++ b/core/modules/node/src/Plugin/views/argument/Vid.php
   @@ -37,15 +36,28 @@ class Vid extends NumericArgument {
   +      @trigger_error('Calling ' . __METHOD__ . '()::__construct() with the $database parameter is deprecated in drupal:9.2.0 and will be removed in drupal:10.0.0. See https://www.drupal.org/node/3178412', E_USER_DEPRECATED);
   

   This isn't in line with what @andypost meant, ::__construct() can be removed when using __METHOD__

Yes, let's fix that

Nice, looks ready now.

On the project page for safedelete I am currently recommending the usage of this core patch, in some situations some warning messages are thrown by core on unpatched systems.

FYI: I just released a new version of the safedelete module supporting checks for referenced embedded media entities.

1. +++ b/core/modules/node/src/Plugin/views/argument/Vid.php
   @@ -37,15 +36,28 @@ class Vid extends NumericArgument {
   +   * @param \Drupal\Core\Database\Connection|null $database
   +   *   Database Service Object.
   

   This will never be a $database object. I'm this argument should be removed.

2. +++ b/core/modules/node/src/Plugin/views/argument/Vid.php
   @@ -37,15 +36,28 @@ class Vid extends NumericArgument {
   +    if ($node_storage instanceof Connection) {
   

   We also need to ensure that $node_storage is an instance of NodeStorageInterface and error if (once the args are sorted out) it is not. That will allow us to introduce the type hint in D10.

   Also given there is special logic to deal with the argument deprecation thisw does need testing.

3. +++ b/core/modules/node/src/Plugin/views/argument/Vid.php
   @@ -37,15 +36,28 @@ class Vid extends NumericArgument {
   +      if ($database) {
   +        $this->database = $database;
   +      }
   

   We should use the \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait to provide BC for $this->database.

#47 is right, reverted for now.

Discussed the issue briefly with @alexpott in slack due to the cross-post, and we both think we should split the bugfix from the deprecation.

The deprecation adds most of the complexity to the patch, and it also makes it harder to backport to 9.1.x and 8.9.x - if we do a bugfix-only patch here and backport it, we could then handle the deprecation in 9.2.x-only in a follow-up.

#50 sounds good, here we go

Per #50 it needs follow-up.

I created #3187435: Deprecate $database argument in \Drupal\node\Plugin\views\argument\Vid::__construct() - @Krzysztof Domański in order to rtbc this patch that really needs to be done. Adding "needs followup" and rtbc in the same comment means that someone else has to do the work.

Committed and pushed 4823b803ea to 9.2.x and 0753e71955 to 9.1.x. Thanks!

No mention of 8.9.x since #51. The test there fails due to visibility on a property in the test class.

I can confirm that the commit to 9.1.x applies successfully to the current 8.9.x release (8.9.18) and works as expected. https://git.drupalcode.org/project/drupal/-/commit/0753e71955013bec8080e...

Wrong code in this patch.

Adding a condition to limit by vid. We have a large site and the current implementation is producing a slow query that returns all titles of all node revisions.

Created a new issue here https://www.drupal.org/project/drupal/issues/3259671