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.
Comment | File | Size | Author |
---|---|---|---|
#20 | entity_merge.patch | 64.42 KB | catch |
#17 | merge-controller-1615236-17.patch | 43.86 KB | aspilicious |
#13 | merge-controller-1615236-13.patch | 50.26 KB | Berdir |
#7 | merge-controller-1615236-7.patch | 43.86 KB | aspilicious |
#5 | merge-controller-1615236-5.patch | 43.76 KB | aspilicious |
Comments
Comment #1
BerdirComment #2
fubhy CreditAttribution: fubhy commentedEntityDatabaseStorageController makes sense because of possible alternative storage implementations.
Comment #3
BerdirWell, 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.
Comment #4
aspilicious CreditAttribution: aspilicious commentedI prefer DatabaseStorageController and EntityStorageControllerInterface for the reasons Berdir mentioned.
Comment #5
aspilicious CreditAttribution: aspilicious commentedThis should implement #3
Comment #6
aspilicious CreditAttribution: aspilicious commentedSelf review, going to change the following in the next patch
Perhapse this should be MyCustomNodeStorageController for clarity
And DatabaseStorageController isn't an interface, EntityStorageControllerInterface is the correct interface for the docs.
Should be on one line now
23 days to next Drupal core point release.
Comment #7
aspilicious CreditAttribution: aspilicious commentedComment #9
aspilicious CreditAttribution: aspilicious commentedWTF...
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).
Comment #10
aspilicious CreditAttribution: aspilicious commented#7: merge-controller-1615236-7.patch queued for re-testing.
Comment #12
aspilicious CreditAttribution: aspilicious commented#5: merge-controller-1615236-5.patch queued for re-testing.
Comment #13
BerdirWeird, 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.
Comment #14
BerdirSo, it passes with the test runner changes reverted. I'm not sure if I like that or not...
Comment #15
Berdir#7: merge-controller-1615236-7.patch queued for re-testing.
Comment #16
fagoPatch looks good to me. I just found one glitch:
Needs replace NodeController by NodeStorageController.
Comment #17
aspilicious CreditAttribution: aspilicious commentedComment #18
catchTagging with avoid commit conflicts, this blocks a lot of other work.
Comment #19
catchCan't see anything to complain about so I'm marking this RTBC. Will commit in a couple of days if there's no objections.
Comment #20
catchRe-rolled after entity.test PSR-0 conversion.
Comment #21
aspilicious CreditAttribution: aspilicious commentedLooks good, applied locally to see if there was something missing. Couldn't find anything.
Comment #22
catch#20: entity_merge.patch queued for re-testing.
Comment #23
catchOK committed/pushed to 8.x. Thanks!
Comment #25
chx CreditAttribution: chx commentedremoving tag