Our real entities are now converted, time for some clean up!

This patch does:

- Merge EntityControllerInterface into EntityStorageControllerInterface
- Merge EntityController into EntityDatabaseStorageController (I find the "Database" in there weird, but haven't changed it. removing it is trivial with a search & replace in a follow-up).
- Default to entity class => Entity and controller class => EntityDatabaseStorageController in entity_get_info(), add documentation for entity class (was missing completely) to hook_entity_info().
- Update all references to EntityControllerInterface and EntityController.

Marking as major as it blocks further progress on the entity API, like removing entity_label() and entity_url().

Patch follows in a second.

CommentFileSizeAuthor
#20 entity_merge.patch64.42 KBcatch
#17 merge-controller-1615236-17.patch43.86 KBaspilicious
#13 merge-controller-1615236-13.patch50.26 KBBerdir
#7 merge-controller-1615236-7.patch43.86 KBaspilicious
#5 merge-controller-1615236-5.patch43.76 KBaspilicious
#1 merge-controller-1615236-1.patch40.61 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
merge-controller-1615236-1.patch40.61 KB
fubhy’s picture

fubhy CreditAttribution: fubhy commented

EntityDatabaseStorageController makes sense because of possible alternative storage implementations.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Merge entity controller interfaces, document and add default entity class definition. » Merge entity controller interfaces, document and add default entity class definition

Well, nothing would stop someone else from creating another controller that doesn't use the database if it were named EntityStorageController.

Also, given that argumentation, "DatabaseStorageController" or "DatabaseEntityStorageController" would actually make more sense to me. We aren't forced to the Entity prefix anymore, it lives within Drupal\entity\ now anyway. I think it still makes sense to have EntityStorageControllerInterface instead of just StorageControllerInterface, however, not because of a pseudo-namespace prefix, but because it's a "entity storage controller". Just like "Drupal\Core\Cache\CacheBackendInterface".

We're not exactly consistent with stuff like that. For example, the default reliable queue is currently named "System" (Drupal\Core\Queue\System), which is completely nonsense, the default cache backend is DatabaseBackend (Drupal\Core\Cache\DatabaseBackend), which actually isn't that bad.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

I prefer DatabaseStorageController and EntityStorageControllerInterface for the reasons Berdir mentioned.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
FileSize
merge-controller-1615236-5.patch43.76 KB

This should implement #3

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

Self review, going to change the following in the next patch

+++ b/core/modules/entity/entity.api.phpundefined
@@ -211,7 +216,7 @@ function hook_entity_info() {
+  // Drupal\entity\DatabaseStorageController interface.
   $entity_info['node']['controller class'] = 'Drupal\mymodule\MyCustomNodeController';

Perhapse this should be MyCustomNodeStorageController for clarity
And DatabaseStorageController isn't an interface, EntityStorageControllerInterface is the correct interface for the docs.

+++ b/core/modules/entity/lib/Drupal/entity/DatabaseStorageController.phpundefined
@@ -12,12 +12,13 @@ use PDO;
- * Default implementation of Drupal\entity\EntityControllerInterface.
+ * Default implementation of
+ * Drupal\entity\DatabaseStorageControllerInterface.

Should be on one line now

23 days to next Drupal core point release.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
FileSize
merge-controller-1615236-7.patch43.86 KB

Status: Needs review » Needs work

The last submitted patch, merge-controller-1615236-7.patch, failed testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review

WTF...

Uncaught exception thrown in shutdown function.
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest88592cache_bootstrap' doesn't exist: DELETE FROM {cache_bootstrap}
WHERE (cid = :db_condition_placeholder_0) ; Array
(
[:db_condition_placeholder_0] => variables
)
in Drupal\Core\Database\Connection->query() (line 532 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php).

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

#7: merge-controller-1615236-7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, merge-controller-1615236-7.patch, failed testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review

#5: merge-controller-1615236-5.patch queued for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
merge-controller-1615236-13.patch50.26 KB

Weird, can't reproduce these locally, but the test bot seems to do so, consistently, on the same tests.

Wondering how this looks without the recently commited test runner patch. This patch is the same as #7 except it reverts the changes in run-tests.sh.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

So, it passes with the test runner changes reverted. I'm not sure if I like that or not...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

#7: merge-controller-1615236-7.patch queued for re-testing.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs review » Needs work

Patch looks good to me. I just found one glitch:

+++ b/core/modules/entity/entity.module
@@ -223,13 +224,14 @@ function entity_load($entity_type, $id, $reset = FALSE) {
+ * class. See node_entity_info() and the NodeController in node.module as an
+ * example.

Needs replace NodeController by NodeStorageController.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review
FileSize
merge-controller-1615236-17.patch43.86 KB
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +Avoid commit conflicts

Tagging with avoid commit conflicts, this blocks a lot of other work.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Can't see anything to complain about so I'm marking this RTBC. Will commit in a couple of days if there's no objections.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review
FileSize
entity_merge.patch64.42 KB

Re-rolled after entity.test PSR-0 conversion.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community

Looks good, applied locally to see if there was something missing. Couldn't find anything.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#20: entity_merge.patch queued for re-testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

OK committed/pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

chx’s picture

chx CreditAttribution: chx commented
Issue tags: -Avoid commit conflicts

removing tag