Convert node properties to methods.

As this was RTBC in #1939994: Complete conversion of nodes to the new Entity Field API setting to RTBC here again. This is straight conversion and ready to go.

(+1 on separating the search/replace things from other changes, so those are easier to review)

Needs a reroll...

git ac https://drupal.org/files/node-methods-1939994-65_0.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  161k  100  161k    0     0  23941      0  0:00:06  0:00:06 --:--:-- 39692
error: patch failed: core/modules/comment/lib/Drupal/comment/Tests/CommentTokenReplaceTest.php:67
error: core/modules/comment/lib/Drupal/comment/Tests/CommentTokenReplaceTest.php: patch does not apply
error: patch failed: core/modules/node/lib/Drupal/node/Tests/PageEditTest.php:128
error: core/modules/node/lib/Drupal/node/Tests/PageEditTest.php: patch does not apply
error: patch failed: core/modules/node/node.pages.inc:107
error: core/modules/node/node.pages.inc: patch does not apply
error: patch failed: core/modules/system/lib/Drupal/system/Tests/ParamConverter/UpcastingTest.php:72
error: core/modules/system/lib/Drupal/system/Tests/ParamConverter/UpcastingTest.php: patch does not apply
error: patch failed: core/modules/system/lib/Drupal/system/Tests/System/TokenReplaceTest.php:43
error: core/modules/system/lib/Drupal/system/Tests/System/TokenReplaceTest.php: patch does not apply
error: patch failed: core/modules/user/lib/Drupal/user/Tests/UserCancelTest.php:212
error: core/modules/user/lib/Drupal/user/Tests/UserCancelTest.php: patch does not apply

I haven't reviewed the initial patch, but that was already RTBC'd in #1. The interdiffs since then look good, so back to RTBC.

Also, at this point in the D8 cycle, I think subitems of critical metas should also be critical (with the possible exception of routing conversions until we get those down to a smaller amount).

#11: node-methods-2049039-11.patch queued for re-testing.

Went through the patch line by line and could only find these small nitpicks. Nice job! :)

+++ b/core/modules/forum/forum.module
@@ -509,7 +509,7 @@ function forum_field_storage_pre_insert(EntityInterface $entity, &$skip_fields)
+        'sticky' => (int)$entity->isSticky(),

@@ -544,7 +544,7 @@ function forum_field_storage_pre_update(EntityInterface $entity, &$skip_fields)
+            'sticky' => (int)$entity->isSticky(),

These two could use a small space there.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.php
@@ -263,7 +263,12 @@ public function setRevisionCreationTime($timestamp) {
+    if (!$entity) {
+      return user_load(0)->getBCentity();
+    }
+    return $entity->getBCEntity();

Didn't we remove the BC decorator for users a few hours ago?

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeSaveTest.php
@@ -114,17 +114,17 @@ function testTimestamps() {
     $node->changed = 280299600;

I think this needs to be setChangedTime().

+++ b/core/modules/node/node.api.php
@@ -257,15 +257,15 @@ function hook_node_grants($account, $op) {
+    // to all users. If we fail to check $node->isPublished(), all users would be able

Comment needs to be rewrapped.

+++ b/core/modules/node/node.module
@@ -537,8 +537,8 @@ function node_load_multiple(array $nids = NULL, $reset = FALSE) {
+ * @return \Drupal\node\NodeInterface|false
+ *   A fully-populated node entity, or NULL if the node is not found.

One of these two lines is lying. My bet is on the first one since we just changed loaders to return NULL instead of FALSE.

+++ b/core/modules/node/node.module
@@ -551,8 +551,8 @@ function node_load($nid = NULL, $reset = FALSE) {
+ * @return \Drupal\node\NodeInterface|null
+ *   A fully-populated node entity, or FALSE if the node is not found.

Same problem, but reverted :)

+++ b/core/modules/node/tests/modules/node_test/node_test.module
@@ -129,15 +130,15 @@ function node_test_node_grants_alter(&$grants, $account, $op) {
     $node->changed = 979534800;

setChangedTime()?

+++ b/core/modules/taxonomy/taxonomy.module
@@ -1122,22 +1122,14 @@ function taxonomy_build_node_index($node) {
+    $sticky = (int)$node->isSticky();

Missing space.

+++ b/core/modules/tracker/tracker.module
@@ -302,13 +303,13 @@ function _tracker_add($nid, $uid, $changed) {
- * Determines the max timestamp between $node->changed and the last comment.
+ * Determines the max timestamp between $node->getChangedTime() and the last comment.
  *
  * @param $nid
  *   A node ID.
  *
  * @return
- *  The $node->changed timestamp, or most recent comment timestamp, whichever
+ *  The $node->getChangedTime() timestamp, or most recent comment timestamp, whichever

Needs rewrapping.

+++ b/core/themes/bartik/templates/node.html.twig
@@ -5,15 +5,16 @@
+ *   - createdtime: Formatted creation date. Preprocess functions can reformat it by

Same here.

+++ b/core/themes/seven/seven.theme
@@ -173,20 +173,20 @@ function seven_form_node_form_alter(&$form, &$form_state) {
-      '#markup' => user_format_name(user_load($node->uid)),
+      '#markup' => $node->getAuthor()->label(),

Afaik, $user->label() is not the same thing as user_format_name()..

Looks good. I raised the weirdness of getAuthorId() but that's a performance shortcut to avoid loading a typeddata object and then getRevisionId() (vs id() and bundle()) but that is not introduced in here so I ran out of complaints

+++ b/core/modules/book/book.module
@@ -120,13 +120,13 @@ function book_permission() {
-function book_node_view_link(EntityInterface $node, $view_mode) {
+function book_node_view_link(NodeInterface $node, $view_mode) {

Yayyyy! :)

+++ b/core/includes/menu.inc
@@ -1005,7 +1005,7 @@ function menu_item_route_access(Route $route, $href, &$map) {
- * $story = $node->type == 'story';
+ * $story = $node->bundle() == 'story';

->bundle() makes sense for generic entity code, but we know we're dealing with nodes here. This is removing important semantic information and introducing a DX regression.

We should add a wrapper type() method on NodeInterface that calls bundle() to keep the code understandable.

+++ b/core/modules/node/templates/node.html.twig
@@ -5,15 +5,16 @@
+ *   - bundle: The type of the node, for example, "page" or "article".

Yeah, no. We're not introducing a variable "bundle" and exposing that to themers. :P This needs to be "type" as well.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTokenReplaceTest.php
@@ -67,7 +67,7 @@ function testCommentTokenReplacement() {
-    $tests['[comment:node:title]'] = check_plain($node->title);
+    $tests['[comment:node:title]'] = check_plain($node->label());

Same deal here. This is called a "title" in the user interface and elsewhere. We need a title() wrapper method on NodeInterface and call it here.

+++ b/core/modules/forum/forum.module
@@ -528,26 +528,27 @@ function forum_field_storage_pre_update(EntityInterface $entity, &$skip_fields)
-            'sticky' => $entity->sticky,
...
+            'sticky' => (int) $entity->isSticky(),

Why is that randomly cast to an int?

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.php
@@ -263,7 +263,12 @@ public function setRevisionCreationTime($timestamp) {
+    $entity = $this->get('revision_uid')->entity;
+    // If no user is given, default to the anonymous user.
+    if (!$entity) {
+      return user_load(0);
+    }

This doesn't seem right; I thought we were removing calls to procedural functions like user_load()?

Also, this should be using the DRUPAL_ANONYMOUS_USER constant.

This doesn't seem right; I thought we were removing calls to procedural functions like user_load()?

To clarify, this couples the Node class to the whole of user.module.

Discussed with @Berdir in IRC. I think not subjecting themers to the word "bundle" is important to do in this issue so that we're not causing a TX regression. So, my recommendation is to use the method name getContentType(), which would yield node.contenttype in the theme layer. I think that's a better method name anyway. They're not just "types"; they're "content types".

@Berdir pointed out that we usually use node_type rather than content_type in the codebase, so I'd also be okay with getNodeType()/node.nodetype.

I also still want to see the death of TypedDataInterface, and I think at least renaming that method to getDataType() is potentially an allowable API change given that it's not a BC break to D7 and it solves some serious DX ick. We'd already considered that some of the typed sanity API changes might not land until after July 1. So I'd suggest filing/linking a followup for that (though we'll want a committer to weigh in on the API change).

Related issue: #2056721: Remove TypedDataInterface::getType()

The int cast is because in the database query we need an integer 0/1, not a boolean, db queries explode when you try to do that. It's necessary now because isSticky(), as an is*() method, returns a boolean.

Let's add an inline comment explaining that bit, then. And then we just need the method wrappers... I think I prefer the more explicit getNodeType() even if #2056721: Remove TypedDataInterface::getType() goes in, because it's more explicit and readable, especially with as vague a word as "type".

Oh, and the followup bits are taken care of now.

TypedData needs the rename, not Node. THis is a very clear example of a parent class getting in the way of the child, which is always the parent's fault. TypeData should not have methods as generic as "getType".

And while getNodeType is more explicit, it isn't explicit in a useful way. $node->getNodeType() is just silly.

I disagree. There's still ambiguity between node types (page) and entity types (node), and when we can do such a simple thing to alleviate that confusion, why wouldn't we?

There's no ambiguity in the code. "type" has a very clear meaning in the domain of node. You shouldnt have to keep a class' inheritance chain in mind to understand it's interface.

I'm more concerned about the terminology we use in actual real verbal conversations with actual real people trying to learn D8, and "node type" is the noun. There is a NodeTypeInterface and that's the object getNodeType() should return.

@msonnabaum and I have agreed to disagree, though, so I'll let that be my last remark on the subject. :)

I agree with Mark, sorry. $node->getType() is just fine, and we should fixed TypedData API to quit being a jerk with its over-ambiguous names.

If that is the decision, that would make #2056721: Remove TypedDataInterface::getType() a soft blocker for this issue rather than just a good idea, so I've bumped its priority.

From #33:

There's no ambiguity in the code. "type" has a very clear meaning in the domain of node.

From #35:

I agree with Mark, sorry. $node->getType() is just fine, and we should fixed TypedData API to quit being a jerk with its over-ambiguous names.

I just want to point out that there's an implicit bias and an implicit assumption in this line of thinking.

The bias is that "the domain of the node" is the more important domain, presumably because it is the most specific. Within the domain of "entity", $entity->getType() (returning 'node', not 'article') would be equally sensible, but we made EntityInterface call that method entityType(), in order to retain clarity when working with subclasses. For the same reason, so long as we have a TypedDataInterface, I agree with changing its getType() to getDataType() (#2056721-14: Remove TypedDataInterface::getType()).

The assumption is that nodes won't be subclassed any further. Which is true in D8 core, and possibly true for the duration of D8 contrib. However, suppose in D9, we do support further subclassing in some meaningful way. For example, maybe on a DrupalCon website, we want "Session" to be a node type, and then subclass that further into "Presentation", "Discussion", and "Lab". Would we then still consider it just fine for $session->getType() to return the node type, 'session', or would we then want to change NodeInterface::getType() to NodeInterface::getNodeType()?

So, back to #33:

You shouldnt have to keep a class' inheritance chain in mind to understand it's interface.

IMO, based on the above, that's actually an argument for calling it getNodeType(). But, I'm fine with calling it getType() in D8, since we actually *do* have the inheritance chain in mind, and therefore, know that node types are the end of that chain.

I apologize for continuing this digression, but I feel it's necessary to clarify my thoughts on this.

The bias is that "the domain of the node" is the more important domain, presumably because it is the most specific.

Specific isn't how I would put it. It's about root-level concepts.

Within the domain of "entity", $entity->getType() (returning 'node', not 'article') would be equally sensible, but we made EntityInterface call that method entityType(), in order to retain clarity when working with subclasses.

This is because Entity's role is that of a framework superclass. It's job is to provide shared functionality through inheritance, because that is currently our primary method of code sharing. A framework superclass has the responsibility to provide this while also staying out of the way of the subclass, because that subclass will act as the root of it's own conceptual hierarchy.

The assumption is that nodes won't be subclassed any further. Which is true in D8 core, and possibly true for the duration of D8 contrib. However, suppose in D9, we do support further subclassing in some meaningful way.

There are essentially two types of inheritance:

1. Code sharing
2. Specialization

Entities are 1. Node is not a specialization of Entity, it is simply an implementation of one (we didn't name it NodeEntity for a reason). If we treat Entities like 2, we're polluting our domain with implementation details. We have a tendency to do that (Block plugins), but we should try to fight it, because it's a big part of what makes Drupal so difficult to learn.

If there was a reason to subclass Node, it would likely be 2, where your specialization of Node could be used polymorphically wherever any code was working with Nodes. The name of a class like this would contain Node, and a prefix or suffix that communicated how it different.

For example, maybe on a DrupalCon website, we want "Session" to be a node type, and then subclass that further into "Presentation", "Discussion", and "Lab". Would we then still consider it just fine for $session->getType() to return the node type, 'session', or would we then want to change NodeInterface::getType() to NodeInterface::getNodeType()?

Having another root hierarchy based on Node makes no sense. Nodes are a thing you can make new implementations of in a UI, and then specialize further using fields. If you want to create a new root hierarchy in code, you make a new entity type. If there's code you want to share with Node that you dont get from Entity, then we just found a new abstraction, which is a totally separate topic.

We have to be able to make these distinctions if we ever hope to have a comprehensible data model.

Looks great to me.

Great API clean-up. Code looked perfect. Committed to 8.x.