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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Issue tags: +Needs tests

Looks valid, needs an actual patch and a test.

saranraj.G’s picture

Assigned: Unassigned » saranraj.G
Status: Active » Needs review
FileSize
2.88 KB

I have attached patch with test case.

Status: Needs review » Needs work

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

Berdir’s picture

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.

saranraj.G’s picture

Status: Needs work » Needs review
FileSize
2.84 KB

Thanks for your review. Re-rolled

Berdir’s picture

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.

saranraj.G’s picture

Status: Needs work » Needs review
FileSize
2.58 KB

I have made the changes.

Berdir’s picture

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" ?

saranraj.G’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
2.76 KB
1.7 KB

I have attached patches with test and fix.

Berdir’s picture

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.

alexpott’s picture

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.