Make the node entity a classed object

Recategorizing per #1346204-11: [meta] Drupal 8 Entity API improvements.

Will we get this?

class forum extends node() { 
  function save() {
    $this->parent->save();
     // Forum node specific stuff here
  }
}

Has anyone started anything on this yet? I'd be up for giving it a shot.

If the issue is still marked 'unassigned', go for it !

Alright, I'll give this a shot.

I saw that it was unassigned, but I'm terrible at remembering to assign things to myself when I look at them, so figured it couldn't hurt to post first since this is a big issue.

Quick post for testing. There is still lots to do. I know the node revision test isn't working, but I want to see what else fails outside of Node module's tests.

Another shot at testing. I think the basics should all be working.

One issue that I have run into, that I think still may break sometimes in the current code, is the new behavior for updating values. Previously if you left a node property empty on a node that already existed, then drupal_write_record will be smart enough to exclude the value from the UPDATE query. Now though, with all node properties defined in a class, drupal_write_record will ALWAYS try to update all columns in the table. That means that to maintain current behavior, the original values need to be manually copied from node->original in the presave.

While this isn't the end of the world, it is a change in the API and required some changes in a few tests to make them pass. Several places rely on being able to essentially grab nid and vid, and construct whole new object. That will no longer work because doing so will set all other node properties back to their default values.

One more time, this time based on most recent HEAD instead of last week.

I rly HATE the fact that we have to COPY entire functions (and edit add 3 lines of code) just to support revisions :( *sigh*.
But I guess this is they way to go as long we don't have revision support in the base entity class.

ps: I'm talking about Save, Delete and invokHook in node.entity.inc

I entirely agree. I kept everything in the Node controller because that's what the issue body said to do, but I'm not a big fan either. I may very well try to just move the revision logic to Entity module. I don't think it makes sense to have revision loading support built in but not revision saving/deleting.

no don't move it to entity yet. There is a seperate issue for that.

I'm uploading a patch withtype hinting (I think I covered everything).
Would like to see what testbot thinks. This is probably to big to do at once. So I suggest reviewing #11 and fixing the type hinting in a followup.

Needs review for #11...
Will leave this for a followup...

@aspilicious Thanks for the type hinting. Unfortunately a lot of places have two types so there is too much hinting. For example node_access can take either $node for 'view', 'update', 'delete' or $node_type if op is 'create'.

@fago Thanks for the initial review. Lots left to do, but getting there.

Fixed the stuff fago mentioned and included the type hinting.

That failed, it just didn't recognize it properly. That's what I get for changing "one more quick thing" :P

* Forum was treating the results of a query on the node table as a node object.
* Comment's documentation was missing that $node can be NULL
* Node's indexing says pass a node, but all you need is to pass an object containing a nid.
** I guess this is kind of a change in the API, but seemed like the thing to do. Thoughts?

+++ b/core/modules/node/node.entity.inc
@@ -0,0 +1,384 @@
+ * This extends the DrupalDefaultEntityController class, adding required
+ * special handling for node objects.
+ */
+class NodeStorageController extends EntityDatabaseStorageController {

Comment is not correct, DrupalDefaultEntityController?

+++ b/core/modules/node/node.entity.inc
@@ -0,0 +1,384 @@
+  public function delete($ids) {

Doc block missing, it should state that it overrides a parent method. Also: why did you have to override the default method?

+++ b/core/modules/node/node.entity.inc
@@ -0,0 +1,384 @@
+  public function save(EntityInterface $entity) {

Same here and on the other methods on this controller.

+++ b/core/modules/node/node.entity.inc
@@ -0,0 +1,384 @@
+      // 'delete' is triggered in 'predelete' is here to preserve hook ordering from D7.

Comment exceeds 80 characters.

+++ b/core/modules/node/node.pages.inc
@@ -87,7 +87,12 @@ function node_add($type) {
+    'language' => LANGUAGE_NONE

missing comma at the end.

+++ b/core/modules/node/node.test
@@ -226,14 +226,12 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+    $updated_node = clone $node;

That looks wrong. Shouldn't you use $node->createDuplicate()?

+++ b/core/modules/node/node.test
@@ -515,7 +511,7 @@ class NodeCreationTestCase extends DrupalWebTestCase {
+      node_save(entity_create('node', $edit));

node_save() is a legacy function and should not be used anymore. Try entity_create('node', $edit)->save() instead.

- fixed all problems from #26
* the delete() method is overriden because we need to delete node revisions as well.
- cleaned-up some unneeded newlines from node.entity.inc
- replaced all occurences of node_save() with $node->save()
- replaced all occurences of clone $node with $node->createDuplicate()

"the delete() method is overriden because we need to delete node revisions as well"

That should be in the patch, not only in your comment :-P
Seriously, please document why any particular method is overridden.

node_save() is a legacy function and should not be used anymore.

Not really sure I understand that.... We have node_load(); why would we not want to use node_save() also?

It seems to me that for basic API consistency reasons, if we use one we should use the other too.

I agree with David, is getting rid of node_save really happening?

'clone $node' and '$node->createDuplicate()' do two different things. CreateDuplicate sets nid and vid to null make duplicating the node easier. The places I used 'clone' preserve nid and vid. Previously the tests were creating new node objects with the same nid by casting a new array to an object and setting different properties on that new object. I figured cloning the original and changing the properties on the clone made more sense with classes.

@David_Rothstein: because node_save() has no benefit really. It just adds one more function call and is longer to type; node_load() is different because I don't have to specify an entity type and I consider it a convenience function.

node_load() and node_save() are fundamentally different concepts. The first retrieves an object from an id, while the second executes an operation on an object I already have at hand. So I would also argue for API consistency: whenever we have methods on entities that accomplish the same as a function, we should use the method. In the end we should remove node_save() from Drupal 9.

@loganfsmyth: urgs, so I guess I was wrong about clone, sorry about that.

I don't have any more time to make progress on this, sorry.

Looks like what this issue needs in the short term is to pass the failed tests. I'll try to work on that tonight.

To start with, a reroll

I'd like to see how many tests still fail after the reroll.

yea, that's what I thought. I dissected the way this patch is applying and was able to discern this to be best merging of the old patch with HEAD. Let's see how it performs.

Oh man that patch is filled with fail. Can anyone reroll this patch better?

I'll see if I can fix my patch since the old one doesn't apply cleanly.

I'll attempt to reroll this patch against HEAD

Rerolled patch against head. It seems like a lot of the problems were due to #965300: Change LANGUAGE_NONE to LANGUAGE_NOT_SPECIFIED; add LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE, mainly $node->language is now $node->langcode.

Some notes:

• Changed $node->language to $node->langcode (per #9655300)
• Removed $node->save() changes to generate-d*-content.sh
• In #27, an unrelated change looks like it got in:
  

  --- a/core/modules/node/node.admin.inc
  +++ b/core/modules/node/node.admin.inc
  @@ -104,18 +104,14 @@ function node_filters() {
       ) + node_type_get_names(),
     );
   
  -  // Language filter if the site is multilingual.
  -  if (language_multilingual()) {
  -    $languages = language_list(TRUE);
  -    $language_options = array(LANGUAGE_NONE => t('Language neutral'));
  -    foreach ($languages as $langcode => $language) {
  -      $language_options[$langcode] = $language->name;
  -    }
  +  // Language filter if there is a list of languages
  +  if ($languages = module_invoke('locale', 'language_list')) {
  +    $languages = array(LANGUAGE_NONE => t('Language neutral')) + $languages;
       $filters['language'] = array(
         'title' => t('language'),
         'options' => array(
           '[any]' => t('any'),
  -      ) + $language_options,
  +      ) + $languages,
       );
     }
     return $filters;
  

  Removed, unless there's a specific reason why that's in there

• There's 5 changes from clone $node to $node->createDuplicate() in the patch in #27. Given the comment in #30, these should be reviewed and verified that they should actually be using createDupicate() instead of clone

diff --git a/core/modules/node/node.entity.inc b/core/modules/node/node.entity.inc
new file mode 100644
index 0000000..5e4cf0e

index 0000000..5e4cf0e
--- /dev/null

--- /dev/null
+++ b/core/modules/node/node.entity.incundefined

+++ b/core/modules/node/node.entity.incundefined
@@ -0,0 +1,394 @@
+class Node extends Entity {

We could move this to modules/node/lib/Drupal/node/Node.php . Doesn't need to be part of this issue though! Looks pretty good to me! If that language issue goes through, I'd consider this pretty good. Changing to needs review to see what the bot says.

There's a problem with with CommentHelperCase::postComment() (the first parameter can either be a $node or NULL). Removed type hinting for that method.

Alright, let's try this again. I did remove one test from node.test:

    $this->assertFalse(_node_revision_access(FALSE, 'view', $admin_account), '_node_revision_access() returns FALSE with an invalid node.');

Since the first argument to _node_revision_access() is being type cast, this throws a Recoverable Error, which I think is enough of a test.

I also removed a lot of the changes from clone $node to $node->createDuplicate(). Most of these were in tests, and were causing tests to fail.

There's still a major test that's failing: Language upgrade test. It looks like it's having trouble finding the NodeStorageController class. Here's the error:

Fatal error: Class 'NodeStorageController' not found in core/modules/entity/entity.module on line 298

Rob Loach suggested moving the classes to modules/node/lib/Drupal/node/Node.php (and NodeStorageController.php), but that didn't work with the upgrade test to autoload the necessary classes. Anyone have any direction on how to fix this?

We'll see what testbot says about the other tests

It looks like autoloader isn't registered in UpgradePathTest (wtf). If add manually call the autoloader, everything works fine:

    drupal_autoload_class('Entity');
    drupal_autoload_class('NodeStorageController');

Obviously this won't fly. Calling spl_autoload_functions() shows that only Symfony\Component\ClassLoader\UniversalClassLoader is the only autoload function that's registered.

I can create a new issue for this (since it seems unrelated), but the patch in #45 won't apply until this is fixed

Created issue for UpgradePathTest bug: #1495024: Convert the entity system to PSR-0

Per fago's suggestion here: http://drupal.org/node/1495024#comment-5773606, manually adding in the autoloaders to UpgradePathTest in order for tests to pass. The same method is being used in #1361232: Make the taxonomy entities classed objects. Once the entity system is converted to PSR-0, these can be removed.

Overall the patch looks pretty good. Some problems:

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -218,31 +220,12 @@ class DatabaseBackend implements CacheBackendInterface {
-    if ($cache_flush && ($cache_flush + $cache_lifetime <= REQUEST_TIME)) {
+    if ($cache_flush && ($cache_flush + variable_get('cache_lifetime', 0) <= REQUEST_TIME)) {

This should use config() instead of variable_get(), right?

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -7,7 +7,6 @@
-use Drupal\Core\Database\Database;
 use Drupal\Core\Database\Install\Tasks as InstallTasks;

unreleated change? Git merge problems? Also elsewhere.

+++ b/core/modules/entity/tests/entity_crud_hook_test.test
@@ -70,11 +70,11 @@ class EntityCrudHookTestCase extends DrupalWebTestCase {
-      'language' => LANGUAGE_NOT_SPECIFIED,
+      'langcode' => LANGUAGE_NOT_SPECIFIED,

unrelated change to this issue? Also elsewhere.

+++ b/core/modules/forum/forum.module
@@ -915,7 +916,16 @@ function forum_get_topics($tid, $sortby, $forum_per_page) {
-    $result = $query->execute();
+    $result = array();
+    foreach ($query->execute() as $row) {
+      $topic = $nodes[$row->nid];
+      $topic->comment_mode = $topic->comment;
+
+      foreach ($row as $key => $value) {
+        $topic->{$key} = $value;
+      }
+      $result[] = $topic;
+    }

unrelated change?

+++ b/core/modules/node/node.entity.inc
@@ -0,0 +1,394 @@
+  /**
+   * The node content_type (bundle).
+   *

why "content_type" and not "content type"?

+++ b/core/modules/node/node.module
@@ -314,10 +318,10 @@ function node_title_list($result, $title = NULL) {
- * @param $node
+ * @param Node $node
  *   A node object.

Description should be "A node entity."

+++ b/core/modules/simpletest/tests/cache.test
@@ -336,40 +336,6 @@ class CacheClearCase extends CacheTestCase {
-
-  /**
-   * Test minimum cache lifetime.
-   */
-  function testMinimumCacheLifetime() {

why is this test function removed?

+++ b/core/modules/system/system.api.php
@@ -1824,10 +1824,6 @@ function hook_theme_registry_alter(&$theme_registry) {
- * Note that returning different themes for the same path may not work with page
- * caching. This is most likely to be a problem if an anonymous user on a given
- * path could have different themes returned under different conditions.
- *

unrelated change, again. There are also other cache related changes that do not belong into this patch.

I apologize, git mishap. Pulled in the latest into my 8.x branch, and forgot to merge it into this issue branch. Here's a new patch without all the unrelated changes. Thanks for the catch klausi.

Now that all the cruft is removed, here's a cleanup from #50 (for items related to this issue):

Changed "content_type" to "content type"

+++ b/core/modules/entity/tests/entity_crud_hook_test.test
@@ -70,11 +70,11 @@ class EntityCrudHookTestCase extends DrupalWebTestCase {
- 'language' => LANGUAGE_NOT_SPECIFIED,
+ 'langcode' => LANGUAGE_NOT_SPECIFIED,

unrelated change to this issue? Also elsewhere.

This is a cleanup from #965300: Change LANGUAGE_NONE to LANGUAGE_NOT_SPECIFIED; add LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE, where language was changed to langcode for nodes.

+++ b/core/modules/node/node.module
@@ -314,10 +318,10 @@ function node_title_list($result, $title = NULL) {
- * @param $node
+ * @param Node $node
* A node object.

Description should be "A node entity."

Should we standardize on referring to "node objects" as "node entities"? There are quite a few places where "node object" is used. Might be a separate issue, since it looks like "comment object" is used over "comment entity".

Ok, it looks like the "object" -> "entity is being standardized here: #1480866: Add type-hinting and parameter type docmentation for comment objects. Updated patch to refer to $node params as "node entity"

If you are adding type hinting to a @param or @return directive for a function, please add it all of them in that docblock. It looks really strange to have it on only one of the parameters. Thanks!

Some very minor stuff

+++ b/core/modules/node/node.entity.incundefined
@@ -0,0 +1,394 @@
+  /**
+   * Saves a node revision.

This needs an @param for the entity. Is revision saving entity based at this moment? If not why are we typehinting with EntityInterface and not Node?

+++ b/core/modules/node/node.entity.incundefined
@@ -0,0 +1,394 @@
+    //  Delete values from other tables also referencing this node.

Only one space needed after '//'

+++ b/core/modules/node/tests/node_access_test.moduleundefined
@@ -199,28 +199,28 @@ function node_access_test_node_load($nodes, $types) {
  * Implements hook_nodeapi_update().

Out of scope but LOL.

+++ b/core/modules/node/node.entity.inc
@@ -0,0 +1,394 @@
+   * 0 => no comments
+   * 1 => comments are read-only
+   * 2 => open (read/write)

Looks like there are constants for this in comment.module, COMMENT_NODE_HIDDEN and the like. (Yeah node.module and comment.module are nicely interwangled)

+++ b/core/modules/node/node.entity.inc
@@ -0,0 +1,394 @@
+  /**
+   * The node translation set ID.
+   *
+   * Translations sets are based on the ID of the node containing the source
+   * text for the translation set.
+   */
+  public $tnid;
+
+  /**
+   * The node 'translation needed' status.
+   *
+   * @var integer
+   */
+  public $translate;
+
+  /**
+   * The node revision creation timestamp.
+   */
+  public $revision_timestamp;
+
+  /**
+   * The node revision author's user ID.
+   */
+  public $revision_uid;

Most of these seem to be missing @var.

Note sure if the referenced upgrade path related issue should be dealt with first, as most upgrade tests are affected by this. It can be activated again now that taxonomy entity classes are in.

Fixed issues from #55 and #56. Also removed the upgrade path related fixes, since they are now in HEAD thanks to the taxonomy entity commit.

@Lars Toomre: type hinting all of the parameters where $node appears is probably outside the scope of this patch.

Ok, it looks like some things changed when the taxonomy patch went in. This should fix most of the tests.

Is there any reason why NodeStorageController is overriding save() and delete() just to handle revisions? Why not use postSave() and postDelete(), so there's not so much duplicated code?

+++ b/core/modules/node/node.entity.incundefined
@@ -0,0 +1,403 @@
+
+      if ($this->revisionKey) {
+        db_delete($this->revisionTable)
+          ->condition($this->idKey, $ids, 'IN')
+          ->execute();
+      }
+
+      // Reset the cache as soon as the changes have been applied.
+      $this->resetCache($ids);
+

If we do the revision stuff in postDelete the caches will be cleared alrdy.

+++ b/core/modules/node/node.entity.incundefined
@@ -0,0 +1,403 @@
+
+        $entity->enforceIsNew(FALSE);
+        $this->postSave($entity, FALSE);
+        $this->invokeHook('insert', $entity);
+      }
+
+      if ($this->revisionKey) {
+        $this->saveRevision($entity);
+      }

If we handle the revisions in postSave other modules can't alter it before it gets saved as a revision

aspilicious is right, I accidentally switched the order of revision execution, which is where many of the errors from #59 came from. Restored proper revision handling in NodeStorageController::save().

Also moved alot of the code form NodeStorageController::invokeHook() to the appropriate methods calls.

One of the test is probably failing because

    elseif (!isset($node->log) || $node->log === '') {
      // If we are updating an existing node without adding a new revision, we
      // need to make sure $node->log is unset whenever it is empty. As long as
      // $node->log is unset, drupal_write_record() will not attempt to update
      // the existing database column when re-saving the revision; therefore,
      // this code allows us to avoid clobbering an existing log entry with an
      // empty one.
      $node->log = $node->original->log;
    }

$node->original isn't set somehow

The comment suggest to just unset the $node->log ....

EDIT: this is added in #9 but I think it's wrong

Ok, looks like moving code out of NodeStorageController::invokeHook() broke some code order, so moving back. Tests should pass now.

#68: node-class-1361234-67.patch queued for re-testing.

#1512564: Remove entity info from the Entity classes landed, so you need to implement the id() and bundle() method's in the Node class.

Also, just like the User patch already has been, it probably makes sense to convert this to PSR-0 already.

Yup, working on converting it to PSR-0. Thanks for the update, I was wondering why tests were failing now.

For reference, #1495024: Convert the entity system to PSR-0. PSR-0 doesn't really have to be part of this patch, could easily be done in a follow up.

I went ahead and converted Node and NodeStorageController to PSR-0, and added in the id() and bundle() methods to Node. Let's see what testbot says.

To fix the exceptions, you need to add a use statement to all files with Node type hints.

Also, the class name needs to be fully qualified in doc blocks AFAIK (someone confirm please), which means a search and replace for "@param Node" with "@param Drupal\node\Node" and the same for @return.

I can confirm everything Berdir says

@Berdir: It looks like the class names only need to be fully qualified for API docs: http://drupal.org/node/1353118 ( see http://drupal.org/node/1495024#comment-5855632). Let me know if I'm wrong, and I can update the doc blocks.

Thanks for the suggestion to fix the exception tests. There was also a broken test due to the way the caches were being reset in Node::save().

IDEs won't properly auto-complete or read class names in docblocks unless they're fully qualified. If the docs don't say to always use fully qualified names in docblocks, they should. :-)

Ok, changed Node class names to be fully qualified in all docblocks.

FYI... The recent issue adding type hinting to comment module did not use fully qualified paths. Either this patch or that one needs to be changed so that they match.

Edit: That was issue #1480866: Add type-hinting and parameter type docmentation for comment objects.

@Lars I think that can be taken care of in #1533020: Convert comment.module entity classes to PSR-0

@Lars: That's because Comment is not ported to PSR-0 and does not use namespaces yet, which essentially means that "Comment" *is* the fully qualified name. This will be changed in the linked issue where the docblocks will need to be updated.

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.phpundefined
@@ -0,0 +1,287 @@
+        // If we are updating an existing node without adding a new revision, we
+        // need to make sure $node->log is unset whenever it is empty. As long as
+        // $node->log is unset, drupal_write_record() will not attempt to update
+        // the existing database column when re-saving the revision; therefore,
+        // this code allows us to avoid clobbering an existing log entry with an
+        // empty one.
+        $node->log = $node->original->log;
+      }

The comment and the code doesn't match anymore. So we have to rewrite the comment or the code. If we rewrite the comment we should know WHY it's safe to change the comment.

-12 days to next Drupal core point release.

Looks like that was done as a work around with the integration of entity classes and drupal_write_record (http://drupal.org/node/1361234#comment-5455070).

This was fixed in the taxonomy entity patch: http://drupal.org/node/1361232#comment-5437548, so I reverted the change back to the original code. Let's see if testbot complains.

Looks like $node->log is currently not defined as a property on the Node class, I'm not sure if it should be. It's not part of the main node table but node_revision (which is always present), no idea what the pattern there is. There is similar stuff e.g. in Comment, where we define a number of things in attachLoad() and also fetch certain things from the user table (which should IMHO be removed in the long-term as it breaks entity caching completely).

Do we have a rule what needs to be documented and what not?

If it is a property defined as public $log, then "unset($node->log);" would feel wrong to me.

unset($node->log) if log is a defined property is wrong, I agree. That said, log is not, AFAIK, an actual property of the node. Rather, it's a weird control-y variable that's more form element than entity. It's a hold-over from the way that nodes are sometimes not nodes but form_state relabeled as a node, which is a legacy from Drupal 3 and a source of all sorts of grotesque bugs. :-) It should be redeveloped into something else.

Should log be converted to a field? That's probably a debate for a different issue. I think it could be resolved following the conversion of nodes to classed objects.

I'd strongly suggest to perform any functional changes to node revisions in a dedicated separate issue. It needs a proper discussion and also needs to be verified that we're not breaking any assumptions, which may or may not be covered by tests currently.

Thus, for this issue and patch, retain the old unset() code.

Thanks sun, agreed. Follow up issue at #1537214: Clean up node revision log message handling or convert it to a field

Rerolled, can we please commit this? To many files affected...

Reviewed all changes. This patch is RTBC after resolving the remaining test failure.

Let's try this one

And what if we "Use" the Node class...

In fact I just made a typo

Alright, I just did a "quick" (45 min) review. Two things popped out at me but it turns out they are already discussed in the issue:

+++ b/core/modules/entity/tests/entity_crud_hook_test.testundefined
@@ -70,11 +70,11 @@ class EntityCrudHookTestCase extends DrupalWebTestCase {
-      'language' => LANGUAGE_NOT_SPECIFIED,
+      'langcode' => LANGUAGE_NOT_SPECIFIED,

@@ -86,7 +86,7 @@ class EntityCrudHookTestCase extends DrupalWebTestCase {
-      'language' => LANGUAGE_NOT_SPECIFIED,
+      'langcode' => LANGUAGE_NOT_SPECIFIED,

@@ -197,12 +197,12 @@ class EntityCrudHookTestCase extends DrupalWebTestCase {
-      'language' => LANGUAGE_NOT_SPECIFIED,
+      'langcode' => LANGUAGE_NOT_SPECIFIED,

I was going to ask what the deal is here but it seems to be answered in #52. Seems like it should have been a followup for the original issue, though. I guess it is needed before we can use Entity?

+++ b/core/modules/node/node.testundefined
@@ -2414,7 +2410,6 @@ class NodeRevisionPermissionsTestCase extends NodeWebTestCase {
-    $this->assertFalse(_node_revision_access(FALSE, 'view', $admin_account), '_node_revision_access() returns FALSE with an invalid node.');

A justification for removing this is in #45. It was added in #1064954: _node_revision_access() static usage to codify the expected behavior of _node_revision_access().

Aside, I definitely prefer the type-hinting as a followup like we are doing with User, but too late now. :)

Ok, sun stated in #93 that this is RTBC once the tests pass, xjm did not find anything in #98, neither do I.

RTBC.

#97: node-class-1361234-97.patch queued for re-testing.

Re-rolled against HEAD.

#102: drupal8.node-class.102.patch queued for re-testing.

#105: d8-entity-node.patch queued for re-testing.

1) Git apply --reject IS AWESOME
2) Can we get this is soonish prety please?

#109: node-class-1361234-109.patch queued for re-testing.

#109: node-class-1361234-109.patch queued for re-testing.

The test failures are caused by #1541792: Enable dynamic allowed list values function with additional context, you can ignore them.

Ow yeah!

#114: d8-entity-node.patch queued for re-testing.

待てました.

Doesn't seem to be pushed yet: http://drupal.org/node/3060/commits

It's there now. :)
http://drupalcode.org/project/drupal.git/commit/51c004a

Actually, I think this needs to be added to the change notification:
http://drupal.org/node/1400186

Added the new Classes like we did with taxonomy and comments. Don't think there are other API changes.

I think we also need to document the PSR-0 namespacing of these classes under a subheader on http://drupal.org/node/1479568, preferably in a nice pretty table like we have for the others. This applies to the other entity conversions as well. Edit: Only Node and User are namespaced so far.

It is possible we introduced some random test failures: http://drupal.org/node/1431634#comment-5923178

:( sad sad

Ok, we did mess up a bit. Luckily, just a bit and I got extremely lucky and got the test failure right on the first time I tried to execute it.

Due to fago's rename from $node->timestamp to $node->revision_timestamp, we did update the existing property, which was used by the submit callback *after* calling save() so the confirmation message used the new timestamp where it should have used to old one. This failed when the difference was enough to be in a different minute.

See patch for details.

To test this, we need to fake a node revision entry far enough into the past to to be sure that the old and new revision time is different enough to trigger this. I haven't yet found time to do this. We might want to commit this asap, I guess...

In this case I agree; let's fix the bot first, and then add the test as followup.

OK, Committed and pushed that hotfix to 8.x to un-screw-up testbot. Thanks, Berdir!

Moving back to ... active?

Updated http://drupal.org/node/1400186 to use a table, please review. I think one problem will be that this table will not have enough space once the entity classes have been renamed, so maybe the third column should be changed to "Type" that is either "Entity" or "StorageController", without namespaces.

The change notification looks good to me. Now it's back to test coverage for #126?

Yeah we need the coverage

This turned out to be easier than I thought. Patch simply updates the timestamp to yesterday and then does another revert to make sure that we consistently get a test fail with the old code.

Two patches , the tests + a revert of the commited bugfixed and the tests only patch. The tests-only patch is the one that needs to be commited.

+++ b/core/modules/node/node.test
@@ -230,6 +230,20 @@ class NodeRevisionsTestCase extends NodeWebTestCase {
+    $this->assertRaw(t('@type %title has been reverted back to the revision from %revision-date.',
+                        array('@type' => 'Basic page', '%title' => $nodes[1]->title,
+                              '%revision-date' => format_date($old_revision_date))), 'Revision reverted.');

In any case, what kind of coding style is that? ;)

Also, the assertion message can be shortened, and $group is reserved for whatever, but not this.

I.e.:

+    $this->assertRaw(t('@type %title was reverted to revision from %revision-date.', array(
+      '@type' => 'Basic page',
+      '%title' => $nodes[1]->title,
+      '%revision-date' => format_date($old_revision_date),
+    ));

Erm, that's not the assertion message, that's the text we're looking for on the page ;) We can't shorten that ;) And the group wasn't the group, that was the assertion message :p

Agreed that one's not necessary either, though. Let's try this.

I'm marking this rtbc, others have 3 days to proof me wrong ;)

Looks good to me too. Committed/pushed to 8.x.