Move entity-type specific storage logic into entity classes

+1, most def.

+1 also - the storage controllers right now really take care of storage logic.
Also related #1497374: Switch from Field-based storage to Entity-based storage.

So getting the names right is the hardest part here imo. What about using "storage engine" for the storage operations? For the other one I'm not so sure, CRUD does not map too well as we have not exactly the CRUD methods on it.

hm, logic isn't very descriptive - it could mean a lot. What about having EntityStorageController + EntityStorageEngine?

The only real storage-related method I was able to find is UserStorageController::save() — since that code is strictly tied to a Database/SQL storage.

There is also buildQuery() or so and delete() which goes with SQL/DB.

public function getFieldDefinitions(array $constraints);

Actually, I think this should move into the EntityManager now - opened #1893820: Manage entity field definitions in the entity manager.

:D I'd not say BusinessController is more descriptive? What about StorageLogicController?

So what's the decision here, how should we proceed.

And yes I am willing to work on this.

ok, so here a proposal to move on:

- Go with "EntityStorageController" and "EntityStorageEngine" for now, thus we postpone the bikeshed of rename EntityStorageController and focus on separating EntityStorageEngine out.
- Fix entity-info to be consistent with storage controller also, i.e. call it "storage_controller_class" there instead of "controller_class" - that's inline with what we do with other controllers.
- Add "storage_engine_class" for the new storage engine.

If we want to rename EntityStorageController we can bikeshed on that in another issue, but move on here.

buildQuery is a tricky one. How do we deal with that? it's definitely SQL bound and yet it is needed.

Hm.

We currently have three overrides of that. Comment should IMHO go away, joins to other entities are bad. Term should probably go away too, load should not do access control* and the translatable tag is useless, I think we should remove that completely.

The only problematic one seems to be node then, which is doing some trickery with the revision table. If we want to decouple sql storage from the storage controller then we need to somehow get rid of that.

* why are we doing that ?!

Well, if we have customizations of the sql-storage it should be in its own StorageEngine class, e.g. NodeSQLStorageEngine ?

* why are we doing that ?!

I think it would
* make it more obvious that the storage controller contains the storage logic, hooks, etc. - see issue summary.
* ease customizing the storage of existing entities only, e.g. move nodes to mongo
* ease doing entities with remote storage as you could re-use the storage-logic controller and concentrate on storage

Well, if we have customizations of the sql-storage it should be in its own StorageEngine class, e.g. NodeSQLStorageEngine ?

Yes, I think that would make sense for now. It's SQL specific handling that might not be necessary for MongoDB. And secondary, we should try to get rid of most of it as I said above.

@fago: I think you misunderstood me. That note was related to the term_access check in the term storage controller. This is at odds with every other entity type. We check access when querying for entities, not when loading them. It was probably added when the entity loading mechanism was introduced back in 7.x to make the conversion easier but I'm quite sure that this should go away.

>@fago: I think you misunderstood me.

Oh sry. I totally agree that loading should not - or better - must not run any access checks.

And secondary, we should try to get rid of most of it as I said above.

Yep!

Opened #1919022: Remove translatable tag and #1919016: Remove term_access tag in TermStorageController::buildQuery() for the discussed tags in the term storage controller.

loading should not - or better - must not run any access checks.

So now to implement node_access?

#1982880: Refactor the entity system to be evented

Reopening because I think the discussion of how this is split is beyond chx's issue.

Regarding name, EntityLogicController would work for me, too — I like how that would finally bring in the terminology of business logic, which is the core of entity types in the first place... Essentially, two entity types that have the same LogicController really shouldn't be two different entity types. ;)

I agree with this thinking, but I dont think it suggests we need another controller. Business logic goes on the object itself. Node's business logic goes on Node, because that is 100% Node's responsibility, not something to be delegated to another object. We'll never come up with a good name for an additional controller, because having one makes no sense.

Related: #1937266: BlockContent entities uses delete method instead of pre/postdelete

Business logic goes on the object itself. Node's business logic goes on Node, because that is 100% Node's responsibility, not something to be delegated to another object. We'll never come up with a good name for an additional controller, because having one makes no sense.

*clap* *clap* .. totally agreed with putting that logic where it belongs, in entity type classes.

Business logic goes on the object itself. Node's business logic goes on Node, because that is 100% Node's responsibility, not something to be delegated to another object. We'll never come up with a good name for an additional controller, because having one makes no sense.

We did also discuss that several times during drupalcon, and we overally agree that this is a good approach. So let's do it.

This is being worked on in http://drupal.org/node/1857558/ branch 1893772

#23: 1893772_23.patch queued for re-testing.

Testbot failures, failed to write config files and stuff like that.

Quick review shows that some methods needs doc-block comments

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -516,4 +516,36 @@ public function isTranslatable() {
+  public function preSave(EntityStorageControllerInterface $storage_controller) {
...
+  public function postSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
...
+  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
...
+  public function postCreate(EntityStorageControllerInterface $storage_controller) {
...
+  public static function preDelete(EntityStorageControllerInterface $storage_controller, $entities) {
...
+  public static function postDelete(EntityStorageControllerInterface $storage_controller, $entities) {
...
+  public static function postLoad(EntityStorageControllerInterface $storage_controller, $entities) {
...
+  public function preSaveRevision(EntityStorageControllerInterface $storage_controller, \stdClass $record) {

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -525,4 +525,36 @@ public function onChange($property_name) {
+  public function preSave(EntityStorageControllerInterface $storage_controller) {
...
+  public function preSaveRevision(EntityStorageControllerInterface $storage_controller, \stdClass $record) {
...
+  public function postSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
...
+  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
...
+  public function postCreate(EntityStorageControllerInterface $storage_controller) {
...
+  public static function preDelete(EntityStorageControllerInterface $storage_controller, $entities) {
...
+
+  public static function postDelete(EntityStorageControllerInterface $storage_controller, $entities) {
...
+  public static function postLoad(EntityStorageControllerInterface $storage_controller, $entities) {

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -1134,4 +1135,36 @@ public function setContext($name = NULL, TypedDataInterface $parent = NULL) {
+  public function preSave(EntityStorageControllerInterface $storage_controller) {
...
+  public function postSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
...
+  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
...
+  public function postCreate(EntityStorageControllerInterface $storage_controller) {
...
+  public static function preDelete(EntityStorageControllerInterface $storage_controller, $entities) {
...
+  public static function postDelete(EntityStorageControllerInterface $storage_controller, $entities) {
...
+  public static function postLoad(EntityStorageControllerInterface $storage_controller, $entities) {
...
+  public function preSaveRevision(EntityStorageControllerInterface $storage_controller, \stdClass $record) {
...
+  public function mergeDefaultDisplaysOptions() {

Once they are defined in interface so just {@inheritdoc} needed

Tons and tons of doxygen fixes.

Notes for reviewers:

1. Function calls in classes: this does not belong to the issue, there is another patch to wean entities off plugins and make them injectable.
2. The ping-pong between entity and controller in save and create. That might not be pretty but it's much more complicated than it looks to move save to entity -- check DatabaseStorageControllerNG for example of how interspersed the seemingly storage-independent logic is with the storage logic. I think we can attack this in a followup if at all.
3. Passing the storage controller to every method instead of setting it ... somewhere ... and then using $this->storageController. There are a ton of static methods (preCreate, preDelete, postDelete, postLoad) which can't be ordinary methods to be able to deal with a multitude of entities and those definitely can't use $this. So, I believe this is what it is.
4. Speaking of postLoad, load is not converted in this issue. The method on the interface is introduced but I would rather not convert until NG is complete. The ordering of load makes me want to run screaming.

#28: 1893772_28.patch queued for re-testing.

Long list of documentation nitpicks. A few things about method names.

Noticed that you haven't touched baseFieldDefinitions() yet. That's tricky because that in fact means that it's still not possible to replace storage controllers in a generic way as every entity type will need it. Probably needs a follow-up for that.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageControllerInterface.phpundefined
@@ -0,0 +1,33 @@
+  public function loadCategories($entities);

Missing documentation.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageControllerInterface.phpundefined
@@ -0,0 +1,33 @@
+   * @param $feed
+   *   The feed object.
...
+  public function saveCategories($feed, array $categories);

Should have a type hint I guess?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.phpundefined
@@ -120,4 +80,36 @@ public function baseFieldDefinitions() {
+      $item->categories = db_query('SELECT c.title, c.cid FROM {aggregator_category_item} ci LEFT JOIN {aggregator_category} c ON ci.cid = c.cid WHERE ci.iid = :iid ORDER BY c.title', array(':iid' => $item->id()))->fetchAll();

This should use the injected database connection as the functions below.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageControllerInterface.phpundefined
@@ -0,0 +1,19 @@
+interface ItemStorageControllerInterface extends EntityStorageControllerInterface {
...
+  public function loadCategories($entities);
+
+  public function deleteCategories($entities);
+
+  public function saveCategories(Item $item);

Also, documentation.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Core/Entity/Feed.phpundefined
@@ -173,4 +175,72 @@ public function id() {
+   * @param \Drupal\Core\Entity\EntityStorageControllerInterface $storage_controller
+   * @param array $values
+   * @return mixed|void
+   */
+  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {

Generated param stuff should be removed.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Core/Entity/Feed.phpundefined
@@ -173,4 +175,72 @@ public function id() {
+  public function preSave(EntityStorageControllerInterface $storage_controller) {
...
+  public function postSave(EntityStorageControllerInterface $storage_controller, $update = FALSE) {

Here it needs to be added.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockStorageControllerInterface.phpundefined
@@ -0,0 +1,14 @@
+interface CustomBlockStorageControllerInterface extends EntityStorageControllerInterface {
+

Empty (undocumented) interface, what is this used for?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.phpundefined
@@ -180,4 +181,36 @@ public function uri() {
+    drupal_container()->get('plugin.manager.block')->clearCachedDefinitions();

Should use Drupal::service()

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
@@ -356,4 +199,23 @@ public function baseFieldDefinitions() {
+  public function getMaxThread(EntityInterface $comment) {
...
+  public function getMaxThreadPerThread(EntityInterface $comment) {
...
+  public function getChildCids(array $comments) {

And... more @inheritdocs :)

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageControllerInterface.phpundefined
@@ -0,0 +1,40 @@
+interface CommentStorageControllerInterface extends EntityStorageControllerInterface {
+
+  public function getMaxThread(EntityInterface $comment);
+
+  public function getMaxThreadPerThread(EntityInterface $comment);
+
+  public function getChildCids(array $comments);

And more documentation necessary here :)

+++ b/core/modules/file/lib/Drupal/file/FileStorageControllerInterface.phpundefined
@@ -0,0 +1,37 @@
+   * @param int $uid
+   *   Optional. A user id, specifying NULL returns the total space used by all
...
+   * @param $status
+   *   Optional. The file status to consider. The default is to only

Missing type on status and Optional. should be (optional). I know this is just moved but we can just as well fix it here, it distracts when reviewing ;)

+++ b/core/modules/file/lib/Drupal/file/FileStorageControllerInterface.phpundefined
@@ -0,0 +1,37 @@
+   *  @return

Same here.

+++ b/core/modules/file/lib/Drupal/file/Plugin/Core/Entity/File.phpundefined
@@ -118,4 +119,44 @@ public function id() {
+  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
...
+  public function preSave(EntityStorageControllerInterface $storage_controller) {
...
+  public static function preDelete(EntityStorageControllerInterface $storage_controller, $entities) {

And more @inheritdoc.

+++ b/core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.phpundefined
@@ -68,4 +69,67 @@ public function id() {
+  public function postSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
...
+  public static function postDelete(EntityStorageControllerInterface $storage_controller, $entities) {

Ditto.

+++ b/core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.phpundefined
@@ -68,4 +69,67 @@ public function id() {
+   * @param ImageStyle $style

Non-namespaced type

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Plugin/Core/Entity/Shortcut.phpundefined
@@ -79,4 +80,59 @@ public function uri() {
+  public function postSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
...
+  public static function preDelete(EntityStorageControllerInterface $storage_controller, $entities) {

And more ;)

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutStorageControllerInterface.phpundefined
@@ -0,0 +1,69 @@
+interface ShortcutStorageControllerInterface extends EntityStorageControllerInterface {

No docs on interface.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutStorageControllerInterface.phpundefined
@@ -0,0 +1,69 @@
+   * @param Shortcut $entity

Missing namespace.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutStorageControllerInterface.phpundefined
@@ -0,0 +1,69 @@
+   * @return
+   *   The name of the shortcut set assigned to this user.

Missing string.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutStorageControllerInterface.phpundefined
@@ -0,0 +1,69 @@
+   * @return
+   *   The number of users who have this set assigned to them.

Missing int

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Vocabulary.phpundefined
@@ -95,4 +96,79 @@ public function uri() {
+  public function postSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
+    if (!$update) {
+      entity_invoke_bundle_hook('create', 'taxonomy_term', $this->id());

Hm, I had the idea of having a default storage controller for entity types that are bundles for another entity type which would call this functions/hooks by default. This makes that a bit weird, would have to be a BundleEntityBase class or something.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.phpundefined
@@ -167,4 +112,30 @@ public function baseFieldDefinitions() {
+    db_delete('taxonomy_term_hierarchy')
...
+    $query = db_insert('taxonomy_term_hierarchy')

Should use $this->database.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageControllerInterface.phpundefined
@@ -0,0 +1,30 @@
+   *   Term object that needs to be added to term hierarchy information.

We usually use "Term entity" not "Term object" I think.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -179,4 +182,93 @@ class User extends Entity implements UserInterface {
+  static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
...
+  function postSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
...
+  public static function postDelete(EntityStorageControllerInterface $storage_controller, $entities) {

@inheritdoc.

+++ b/core/modules/user/lib/Drupal/user/RoleStorageControllerInterface.phpundefined
@@ -0,0 +1,21 @@
+  /**
+   * Delete roles.
+   *
+   * @param array $rids
+   *   The list of role IDs to delete.
+   */
+  function deleteRoles(array $rids);

This doesn't delete roles, it deletes permission/role and user/role assignments.

+++ b/core/modules/user/lib/Drupal/user/UserStorageControllerInterface.phpundefined
@@ -0,0 +1,36 @@
+interface UserStorageControllerInterface extends EntityStorageControllerInterface {

No docs on interface.

+++ b/core/modules/user/lib/Drupal/user/UserStorageControllerInterface.phpundefined
@@ -0,0 +1,36 @@
+  public function addRoles(array $users);
...
+  public function saveRoles(EntityInterface $user);
...
+  public function deleteUserRoles(array $uids);

Similar, the documentation her is correct but not sure about the names.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.phpundefined
@@ -361,4 +362,66 @@ public function getExportProperties() {
+   * @param \Drupal\Core\Entity\EntityStorageControllerInterface $storage_controller
+   * @param array $values
+   * @return mixed|void
+   */
+  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {

Another left-over to remove.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -1134,4 +1135,36 @@ public function setContext($name = NULL, TypedDataInterface $parent = NULL) {
+  public function preSave(EntityStorageControllerInterface $storage_controller) {

Methods in this class are also missing @inheritdoc.

I've not been in this code in a long time, so it's hard for me to get into much detail. Some comments below, but I'm afraid they're rather superficial. It looks like most of this patch is just moving code around between entities and storage controllers and breaking it up into more methods, which I generally support.

The main concern I have is the static methods chx mentioned. He and I talked in IRC, and he pointed out that they need to operate on multiple objects at once since "everything is a set operation". I hate that we're using static methods here, but since we can't put them on the storage controller (since that needs to be swappable but the logic here is bound to the entity, not the storage mechanism for the entity), I don't have a better solution.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageController.php
@@ -191,4 +105,39 @@ public function baseFieldDefinitions() {
+      $item->categories = db_query('SELECT c.cid, c.title FROM {aggregator_category} c JOIN {aggregator_category_feed} f ON c.cid = f.cid AND f.fid = :fid ORDER BY title', array(':fid' => $item->id()))->fetchAllKeyed();

I know you said to ignore the function calls for now since injection is being handled elsewhere, but the very next method has a $this->database in it, meaning the DB is already injected. We can go ahead and convert this one since no new injection is needed for it.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.php
@@ -120,4 +80,36 @@ public function baseFieldDefinitions() {
+      $item->categories = db_query('SELECT c.title, c.cid FROM {aggregator_category_item} ci LEFT JOIN {aggregator_category} c ON ci.cid = c.cid WHERE ci.iid = :iid ORDER BY c.title', array(':iid' => $item->id()))->fetchAll();

Ibid.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.php
@@ -180,4 +181,36 @@ public function uri() {
+    drupal_container()->get('plugin.manager.block')->clearCachedDefinitions();

If we're adding this line, at the very least it should be Drupal::, not drupal_container(), as the latter has been deprecated for months.

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.php
@@ -356,4 +199,23 @@ public function baseFieldDefinitions() {
+    return db_query('SELECT MAX(thread) FROM {comment} WHERE nid = :nid', array(':nid' => $comment->nid->target_id))->fetchField();

As above.

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageControllerInterface.php
@@ -0,0 +1,40 @@
+  public function getMaxThread(EntityInterface $comment);
+
+  public function getMaxThreadPerThread(EntityInterface $comment);
+
+  public function getChildCids(array $comments);

Need docblocks.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.php
@@ -219,4 +220,132 @@ protected function init() {
+  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
+    if (empty($values['node_type']) && !empty($values['nid'])) {
+      $node = node_load(is_object($values['nid']) ? $values['nid']->value : $values['nid']);
+      $values['node_type'] = 'comment_node_' . $node->type;
+    }
+  }

This makes me very concerned about race conditions. I've run into a *lot* of race conditions lately at work around entity A being saved, which requires entity B to be updated, and entity B is updated based on entity A. Oh, wait, you got entity A via node_load()? Guess what, because of the DB transactions the node load will get the pre-edit entity A, and so you're operating on stale data.

Having a node_load() (function or method, doesn't matter) here raises a red flag in my mind for that reason.

Great work!

Patch looks good to me, it's mostly doxygen stuff that is missing or needs work, but I'm not repeating that as berdir pointed that already out.

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -155,6 +155,96 @@ public function save();
+   * @param \stdClass $record
+   *   The revision object.

Could be explained a bit better - which object is it? Maybe just describe it as "The revision record to be saved."

This makes me very concerned about race conditions. I've run into a *lot* of race conditions lately at work around entity A being saved, which requires entity B to be updated, and entity B is updated based on entity A. Oh, wait, you got entity A via node_load()? Guess what, because of the DB transactions the node load will get the pre-edit entity A, and so you're operating on stale data.

Oh yes, we should really do #1729812: Separate storage operations from reactions. This patch doesn't change the likelihood for that issues though.

Keeping up with HEAD (User NG, Actions are config entities). berdir's review is addressed.

Boy, these two fine English gentlemen certainly are moving the drop fast.

Whoops... Turns out that #2017657: Node admin test broken wasn't actually broken. Kinda weird anyways.

Forgot the interdiff.

It failed and passed due to the commit and rollback of #2012916: Implement access controller for the menu and menu link entity. Thanks fubhy, much nicer fix than I was able to come up with. Note that the test is broken, but differently, #2018315: NodeAdminTest is broken here has been filed.

Chasing HEAD.

Just fixing some nitpicks. I know that some of these have not been introduced here, but since we are moving it all around anyways this is a good chance to fix them.

Added a few missing 'public' to some functions.

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
@@ -224,7 +224,7 @@ public function getMaxThreadPerThread(EntityInterface $comment) {
   public function getChildCids(array $comments) {
     return $this->database->select('comment', 'c')
       ->fields('c', array('cid'))
-      ->condition('pid', array(array_keys($comments)))
+      ->condition('pid', array_keys($comments))
       ->execute()
       ->fetchCol();
   }

Given that previous tests were green too this probably indicates that we are missing test coverage for this method.

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
@@ -52,156 +53,9 @@ protected function attachLoad(&$records, $load_revision = FALSE) {
-  protected function postDelete($comments) {
-    // Delete the comments' replies.
-    $query = db_select('comment', 'c')
-      ->fields('c', array('cid'))
-      ->condition('pid', array(array_keys($comments)), 'IN');
-    $child_cids = $query->execute()->fetchCol();
-    entity_delete_multiple('comment', $child_cids);

This is what we had before... Does not seem right, or? So yeah, looks like we are missing a 'comment cleanup' test.

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
@@ -224,7 +224,7 @@ public function getMaxThreadPerThread(EntityInterface $comment) {
   public function getChildCids(array $comments) {
     return $this->database->select('comment', 'c')
       ->fields('c', array('cid'))
-      ->condition('pid', array(array_keys($comments)))
+      ->condition('pid', array_keys($comments))
       ->execute()
       ->fetchCol();
   }

Given that previous tests were green too this probably indicates that we are missing test coverage for this method.

We are not adding new tests for existing code here; please file a separate issue. The issue would never ever end if we were to do that.

Re fail, bot fluke.

Read through the entire patch once more. Found a few minor things and fixed them in this patch. @see interdiff. I think (even though it looks ugly) we should use "unassign" instead of "unAssign" as it's one word.

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageControllerInterface.phpundefined
@@ -0,0 +1,66 @@
+  /**
+   * @todo Document this.
+   *
+   * @param EntityInterface $comment
+   *   A comment entity.
+   * @return string
+   *   A thread string.
+   */
+  public function getMaxThread(EntityInterface $comment);
+
+  /**
+   * @todo Document this.
+   *
+   * @param EntityInterface $comment
+   *   A comment entity.
+   * @return string
+   *   A thread string.
+   */
+  public function getMaxThreadPerThread(EntityInterface $comment);

This is the only thing left for this patch to be RTBC imho. I would've written the documentation but I seriously have no clue how to document these methods/what to write.

We are not adding new tests for existing code here; please file a separate issue. The issue would never ever end if we were to do that.

Oh yeah, I wasn't implying that we should fix it here. That comment was rather a reminder for me to open a follow-up later.

Resolved @todo from #57

refined one of @return in #59

Cool, RTBC if green (which it will be).

Keeping up with HEAD (FileNG)

Updated issue summary.

Updated issue summary.

Committed 1648a47 and pushed to 8.x. Thanks!

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Plugin/Core/Entity/Shortcut.phpundefined
@@ -79,4 +80,65 @@ public function uri() {
+  public function preSAve(EntityStorageControllerInterface $storage_controller) {

Fixed capitalisation on commit...

We definitely need a change notice here but I also think we need to scan existing D8 change notices...

I do not see a need for new change notice because of 1. and 2.:

1. Content entity classes's interfaces should implement ContentEntityInterface. So entity classes should implementContentEntityInterface, EntityInterface, ConfigEntityInterface -- but the methods are not listed so this doesn't need an update, it's still true you need to implement these interfaces. (I will file a novice task for this to be upgraded with links api.drupal.org links to the interfaces as 2. is because that's nice.)
2. New Entity Field API based upon the Typed Data API downright links to api.drupal.org when talking of the specific interfaces and those are auto updated.
3. Updated Support for saving and deleting revisions in the default storage controller and entity classes for the moving of preSaveRevision.

Awesome!

I noticed NodeStorageController postDelete() has been forgotten, so I've opened #2020837: Node access table is not cleaned up after delete.

berdir found Entities are now classed objects using a defined interface, and that concludes our journey in change notices. Still no need for a new one.

When you update an existing change notice, PLEASE also add the issue you're updating it for to the list of issues on the change notice. Please please please. This is the only way we can keep track of where changes are coming from, and the way that the change notices are auto-linked in the issue summaries. I updated https://drupal.org/node/1400186.

Edit: Also, a summary of the API changes actually made by this issue, in the summary, would be nice for everyone whose patches broke on account of it. :)