What are the steps required to reproduce the bug?
Call api node_last_changed($nid);
What behavior were you expecting?
unix timestamp indicating the last time the node was changed.
What happened instead?
The following uncaught exception message was thrown,
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens: SELECT changed FROM {node_field_data} WHERE nid = :nid AND default_langcode = 1; Array ( [:nid] => 4 [:langcode] => ) in node_last_changed() (line 1764 of /home/saranraj/Desktop/drupal-8/core/modules/node/node.module).
The website has encountered an error. Please try again later.
Fix worked for me
function node_last_changed($nid, $langcode = NULL) {
if (isset($langcode)) {
$result = db_query('SELECT changed FROM {node_field_data} WHERE nid = :nid AND langcode = :langcode', array(':nid' => $nid, ':langcode' => $langcode))->fetch();
}
else {
$result = db_query('SELECT changed FROM {node_field_data} WHERE nid = :nid AND default_langcode = :default_langcode', array(':nid' => $nid, ':default_langcode' => 1))->fetch();
}
return is_object($result) ? $result->changed : FALSE;
}
Reporting this on behalf of my colleague Saranraj G. He couldn't make it through because of spam policy.
Comment | File | Size | Author |
---|---|---|---|
#9 | test-2068891-9.patch | 1.7 KB | saranraj.G |
#9 | test-fix-2068891-9.patch | 2.76 KB | saranraj.G |
#9 | interdiff.txt | 1.44 KB | saranraj.G |
#7 | 2068891-7.patch | 2.58 KB | saranraj.G |
#5 | 2068891-5.patch | 2.84 KB | saranraj.G |
Comments
Comment #1
BerdirLooks valid, needs an actual patch and a test.
Comment #2
saranraj.G CreditAttribution: saranraj.G commentedI have attached patch with test case.
Comment #4
BerdirThanks for working on this!
Some trailing spaces here and in other places in tha file.
The BC decorator of nodes and users have been removed, so ->uid and so on is now, so, for web_user, you need to use ->id() now,instead of uid().
This doesn't work, changed is always set to REQUEST_TIME when the node is saved. The only way to make it work is using a trick through a presave hook, see NodeSaveTest::testTimestamps().
And here id() for nid, getChangedTime() for ->changed.
Comment #5
saranraj.G CreditAttribution: saranraj.G commentedThanks for your review. Re-rolled
Comment #6
BerdirThis should be Contains \Drupal\... now, with a leading \ in front of Drupal.
Would be great if we could make this a DrupalUnitTestBase test.
That is much faster, as it doesn't have to do a full installation. However, you have a completely empty environment, and need to enable all modules (includig dependencies), tables (with installSchema()).
$node = entity_create();
$node->save();
Then you can continue to use $node.
Is this actually necessary?
Space after // and . at the end.
Comment #7
saranraj.G CreditAttribution: saranraj.G commentedI have made the changes.
Comment #8
BerdirGetting there :) Please provide interdiff in the future, so that it's possible to see what you changed in comparison to the last patch.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff | Microbranching workflow: http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...
Without the $nid was correct.
Now we're missing a test for the actual bug here, that only occurs when a language is passed in.
So you should test with second call, where you pass in the node language ($node->language()-id) as second argument and verify the result again.
Then upload two patches, one that only contains the test and one with test and fix. That should prove that the test really covers the bug.
I think we can do better on the assertion message, maybe just "Expected last changed timestamp returned" ?
Comment #9
saranraj.G CreditAttribution: saranraj.G commentedI have attached patches with test and fix.
Comment #10
BerdirLooks good to me!
The function will eventually move to the storage controller or be replaced or whatever, either way, having explicit test coverage makes sense and we obviously didn't have this yet.
Comment #11
alexpottCommitted ad2ec22 and pushed to 8.x. Thanks!