Problem
I would export a revision. So i created a new view of type "content revision" with a filter on "revision id".
I acces to this view with the url "revision/%" where % is a vid
And i had this error :
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'npr.title' in 'field list': SELECT nr.vid, nr.nid, npr.title FROM {node_revision} nr WHERE nr.vid IN ( :vids__0 ); Array ( [:vids__0] => 60 ) in Drupal\node\Plugin\views\argument\Vid->titleQuery() (line 76 of core/modules/node/src/Plugin/views/argument/Vid.php).
Proposed resolution
The current line 76 of vid.php is :
<?php
$results = $this->database->query('SELECT nr.vid, nr.nid, npr.title FROM {node_revision} nr WHERE nr.vid IN ( :vids[] )', array(':vids[]' => $this->value))->fetchAllAssoc('vid', PDO::FETCH_ASSOC);
?>
There is no npr table in the WHERE clause
The first thing to do is to add a "\" before PDO::FETCH_ASSOC
The second is to remove the reference to "npr" on the select
Just for testing I replaced the line 76 by this one :
<?php
$results = $this->database->query('SELECT nr.vid, nr.nid, nr.nid as title FROM {node_revision} nr WHERE nr.vid IN ( :vids[] )', array(':vids[]' => $this->value))->fetchAllAssoc('vid', \PDO::FETCH_ASSOC);
?>
And now my export work well. Of course it's not a correct solution. It's just a dirty and quick fix for remove the PHP fatal error.
Comment | File | Size | Author |
---|---|---|---|
#60 | limitbyvid-2628130-60.patch | 527 bytes | _KASH_ |
#6 | sql_error_on_revision_export_from_view.patch | 1.05 KB | lamp5 |
#11 | 2628130-11-test-only.patch | 7.25 KB | Anonymous (not verified) |
#11 | 2628130-11.patch | 8.19 KB | Anonymous (not verified) |
#24 | 2628130-24-test-only.patch | 7.24 KB | raman.b |
Comments
Comment #2
cmarti CreditAttribution: cmarti commentedComment #3
cmarti CreditAttribution: cmarti commentedComment #4
cgoffin CreditAttribution: cgoffin at Sopra Steria commentedI created a patch to fix this issue.
Comment #5
cgoffin CreditAttribution: cgoffin at Sopra Steria commentedComment #6
lamp5The same error on 8.3.7
Patch work!
Comment #7
amaisano CreditAttribution: amaisano commentedSo 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?
Comment #8
amaisano CreditAttribution: amaisano commentedComment #9
amaisano CreditAttribution: amaisano commentedAny updates?
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commented#7:
Just an unsuccessful replacement in a large issue #2057401: Make the node entity database schema sensible:
But if you're wondering why "npr" instead of "nfr" (
node_field_revision
table), so just an unsuccessful replacement in an even larger issue #1498674: Refactor node properties to multilingual, where would initially usenode_property_revision
, but then changed the decision.#9: It would be great to unite forces with #2945298: Typo in SQL select, where we have a fresh patch.
But it seems to me better to transfer it here (with the addition of credit to @Pasqualle). Because here better IS, and it was created before.
Also we need test for prevent regression. I can help with it, if there is no other who want to.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commented#10: Oh, my bad! This was not an unsuccessful renaming,
node_revision
table has not title column. So, #4 nice fix, @cgoffin! Here #4 patch + quick test.Comment #13
wmike7 CreditAttribution: wmike7 at Monarch Digital commented#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.
Comment #15
amaisano CreditAttribution: amaisano commentedPatch #11 passes on the latest build. Can we get this RTBC soon?
Comment #16
amaisano CreditAttribution: amaisano commentedWhat are the next steps to get this into the next patch release?
Comment #17
cgoffin CreditAttribution: cgoffin at Sopra Steria commentedComment #18
alexpottNice bug find! Yep the query is obviously wrong but I think we need to ask ourselves what is this code really doing?
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:
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.
Comment #19
bstan CreditAttribution: bstan at Liberty Mutual Group commentedThis is something of interest for my application, any update on when this will be released will be helpful.
Comment #21
joseph.olstadGreat 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.
Comment #22
joseph.olstadstill using this patch, now on 8.8.8
Comment #24
raman.b CreditAttribution: raman.b at OpenSense Labs commentedAddressing #18 and resolving failed test cases from #11 for latest dev branch
Comment #26
LendudeI'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.
Comment #27
Krzysztof Domański1. Added The \Drupal\node\Plugin\views\argument\Vid constructor parameter $database is deprecated
2. Reorder parameters. Optional parameter should be provided after required.
Comment #28
LendudeSince anything extending this will pass the variables in the other order, won't this still lead to a fatal error?
Comment #29
Krzysztof DomańskiImproved BC.
Comment #31
Krzysztof DomańskiComment #33
raman.b CreditAttribution: raman.b at OpenSense Labs commentedComment #34
Krzysztof DomańskiAdded follow-up #3178818: Remove deprecation layer and add argument type to the \Drupal\node\Plugin\views\argument\Vid constructor.
Comment #35
andypostneeds work for todo code style
needs to be inline with https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...
could use
__METHOD__ . '()
Comment #36
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedWorked on comment #35
Comment #37
acrazyanimal CreditAttribution: acrazyanimal as a volunteer and commented#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.
Comment #38
tanubansal CreditAttribution: tanubansal at Salsa Digital commentedTested #36 on 9.1
Its working fine for me as well
RTBC
Comment #39
elgandoz CreditAttribution: elgandoz as a volunteer commentedTested #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
Comment #40
elgandoz CreditAttribution: elgandoz as a volunteer commentedComment #41
LendudeThis isn't in line with what @andypost meant, ::__construct() can be removed when using __METHOD__
Comment #42
raman.b CreditAttribution: raman.b at OpenSense Labs commentedYes, let's fix that
Comment #43
LendudeNice, looks ready now.
Comment #44
joseph.olstadOn 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.
Comment #47
alexpottThis will never be a $database object. I'm this argument should be removed.
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.
We should use the \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait to provide BC for $this->database.
Comment #49
catch#47 is right, reverted for now.
Comment #50
catchDiscussed 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.
Comment #51
Lendude#50 sounds good, here we go
Comment #52
Krzysztof DomańskiPer #50 it needs follow-up.
Comment #53
alexpottI 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.
Comment #54
alexpottCommitted and pushed 4823b803ea to 9.2.x and 0753e71955 to 9.1.x. Thanks!
Comment #58
kerasai CreditAttribution: kerasai commentedNo 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 current8.9.x
release (8.9.18
) and works as expected. https://git.drupalcode.org/project/drupal/-/commit/0753e71955013bec8080e...Comment #59
_KASH_ CreditAttribution: _KASH_ commentedWrong code in this patch.
Comment #60
_KASH_ CreditAttribution: _KASH_ commentedAdding 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.
Comment #61
_KASH_ CreditAttribution: _KASH_ commentedCreated a new issue here https://www.drupal.org/project/drupal/issues/3259671