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

<?php
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.

Files: 
CommentFileSizeAuthor
#9 test-2068891-9.patch1.7 KBsaranraj.G
FAILED: [[SimpleTest]]: [MySQL] 58,405 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#9 test-fix-2068891-9.patch2.76 KBsaranraj.G
PASSED: [[SimpleTest]]: [MySQL] 58,425 pass(es).
[ View ]
#9 interdiff.txt1.44 KBsaranraj.G
#7 2068891-7.patch2.58 KBsaranraj.G
PASSED: [[SimpleTest]]: [MySQL] 58,075 pass(es).
[ View ]
#5 2068891-5.patch2.84 KBsaranraj.G
PASSED: [[SimpleTest]]: [MySQL] 58,449 pass(es).
[ View ]
#2 2068891-2.patch2.88 KBsaranraj.G
FAILED: [[SimpleTest]]: [MySQL] 58,604 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Comments

Issue tags:+Needs tests

Looks valid, needs an actual patch and a test.

Assigned:Unassigned» saranraj.G
Status:Active» Needs review
StatusFileSize
new2.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,604 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

I have attached patch with test case.

Status:Needs review» Needs work

The last submitted patch, 2068891-2.patch, failed testing.

Thanks for working on this!

  1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,62 @@
    +class NodeLastChangedTest extends NodeTestBase {
    +  ¶
    +   /**

    Some trailing spaces here and in other places in tha file.

  2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,62 @@
    +      'uid' => $this->web_user->uid,

    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().

  3. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,62 @@
    +    $node->changed = 280299600;

    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().

  4. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,62 @@
    +    //Test node last changed timestamp
    +    $changed_timestamp = node_last_changed($node->nid);
    +    $this->assertEqual($changed_timestamp, $node->changed, 'Function node_last_changed() returned expected timestamp.');

    And here id() for nid, getChangedTime() for ->changed.

Status:Needs work» Needs review
StatusFileSize
new2.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,449 pass(es).
[ View ]

Thanks for your review. Re-rolled

Status:Needs review» Needs work
  1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,60 @@
    + * Definition of Drupal\node\Tests\NodeLastChangedTest.

    This should be Contains \Drupal\... now, with a leading \ in front of Drupal.

  2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,60 @@
    +
    ...
    +class NodeLastChangedTest extends NodeTestBase {

    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()).

  3. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,60 @@
    +    entity_create('node', $edit)->save();
    +    $node = $this->drupalGetNodeByTitle($edit['title']);

    $node = entity_create();
    $node->save();

    Then you can continue to use $node.

  4. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,60 @@
    +    // Update the created timestamps.
    +    $node->created = 979534800;
    +    $node->save();

    Is this actually necessary?

  5. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,60 @@
    +    //Test node last changed timestamp

    Space after // and . at the end.

Status:Needs work» Needs review
StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,075 pass(es).
[ View ]

I have made the changes.

Status:Needs review» Needs work

Getting 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-...

  1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,50 @@
    + * Tests the node_last_changed($nid) function.

    Without the $nid was correct.

  2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,50 @@
    +    $changed_timestamp = node_last_changed($node->id());

    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.

  3. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeLastChangedTest.php
    @@ -0,0 +1,50 @@
    +    $this->assertEqual($changed_timestamp, $node->getChangedTime(), 'Function node_last_changed() working as expected.');

    I think we can do better on the assertion message, maybe just "Expected last changed timestamp returned" ?

Status:Needs work» Needs review
StatusFileSize
new1.44 KB
new2.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,425 pass(es).
[ View ]
new1.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,405 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

I have attached patches with test and fix.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

Looks 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.

Status:Reviewed & tested by the community» Fixed

Committed ad2ec22 and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.