Replace node_load() with Node::load()

Hi,

Please find the patch attached for this issue.Any comments or suggestions are appreciated.

My first review on D8 ;)

It looks good nripeshtrivedi

+++ b/scheduler.module
@@ -1,5 +1,7 @@
+use \Drupal\node\Entity\Node;
+

This is not needed though ;)

Hi,

Going by https://www.drupal.org/node/2266845, it says it does. How could we use an entity class without that

I disagree with #3. It's necessary to add "use Drupal\node\Entity\Node;" in order to use the Node class. After a review of the patch, I noticed that there is still some work to do.

Too many parameters added when using ::load

In the code below, you're calling Node::load($node->nid, NULL, TRUE). The load method accepts only one parameter: nid. You're providing these additional parameters in every Node::load call in both scheduler.test and scheduler_api.test.

/**
  * @file
  * Scheduler module test case file.
@@ -131,7 +133,7 @@
     $this->assertRaw(t('@type %title has been updated.', array('@type' => t('Basic page'), '%title' => check_plain($edit['title']))), 'The node is saved successfully when the publication date is in the past and the "publish" behavior is chosen.');

     // Reload the changed node and check that it is published.
-    $node = node_load($node->nid, NULL, TRUE);
+    $node = Node::load($node->nid, NULL, TRUE);

Saving a node.

Changing a node is done by calling the save() method of the Node class. The following code;
$node->save($node);
Needs to be replaced with;
$node->save();

Note: I'm not 100% sure wether this in scope in this issue.

Changing the value of a node.

In your patch, you're changing the value of a field of a node as following;
$node->field_scheduler_test_approved[$node->language][0]['value'] = TRUE;
However, in Drupal 8, this should be done something like this;
$node->set('field_scheduler_test_approved' , TRUE);
For more information, check class Node.

Another example that needs to be refactored;
$node->{$action . '_on'} = strtotime('-1 day');

Note: I'm not 100% sure wether this in scope in this issue.

Wouldnt it be better to use entity_load here ??

Hello,

Thank you legovaer for the review ;)

I created a new patch fixing the following:
- Fix node_load() to Node::load()
- Fix too many parameters added when using Node::load
- Use Node::loadMultiple() instead of Node::load() when possible
- Fix doc and type when passing $node objects as parameters in functions
- Fix node accessors and setters - Change to $node->id(), $node->getType(), $node->set...

$node->save($node) was fixed in #2418535: Replace node_save() with $node->save()
Also I didn't change how we access or set the publish_on or unpublish_on properties... I think it's ok like this those properties are injected during scheduler_node_load(). WHat do you think?

@Swarnendu-Dutta: We know we have nodes here so I think the best is to stick with Node::load(). Maybe later on we could change the code so scheduler could apply on entities and not only nodes but this is a big change to be discussed I guess...

Let me know,
Hervé

Thanks. Sure here it is :)
I might have slighly changed the scope of this issue with such a big patch... sorry for that and for the conflicts.
I hope it won't affect other issues that much.

Let me know,
Hervé

Hi,

Thanks a lot pfrenssen. Great job on the split up too ;)

I'm rerolling the remaining patch to take into account the 3rd parameter of node_load() that I missed.
D7:
$node = node_load($nid, NULL, TRUE);
D8:
Used in module files:

    \Drupal::entityManager()->getStorage('node')->resetCache(array($nid));
    $node = Node::load($nid);
    

OR (used in tests - we can retrieve the EntityManager from a TestBase class):

    $node_storage = $this->container->get('entity.manager')->getStorage('node');
    $node_storage->resetCache(array($nid));
    $node = $node_storage->load($nid);
    

So I used the 2nd notation but for this to work we would need to finish: #2418543: Relocate automated tests in PSR-4 namespaces
Having the tests working would be nice so I'll probably spend some time on it this week then.

Hervé