Posted by sun on January 20, 2013 at 12:56pm
29 followers
Jump to:
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | entity system |
| Category: | task |
| Priority: | normal |
| Assigned: | chx |
| Status: | active |
| Issue tags: | API clean-up, Entity system |
Issue Summary
Problem
- Most entity storage controllers in core right now do NOT override the storage. They apply custom CRUD/business logic for creating/saving/deleting entities.
- That custom CRUD business logic is completely independent from the storage. Storage has to mean Database vs. Remote vs. Config vs. File vs. Whatever only.
Goal
- Limit StorageController to actual storage operations only.
- Introduce a new CRUDController (or similar) for storage-agnostic CRUD operations.
Comments
#1
+1, most def.
#2
+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.
#3
Looking at EntityStorageControllerInterface, there are only two methods that don't really belong there:
<?php
public function create(array $values);
public function getFieldDefinitions(array $constraints);
?>
And looking through all storage controller overrides we currently have in core (except ConfigStorageController, of course), they are overriding these methods:
<?php
public function create(array $values) {
public function load(array $ids = NULL) {
protected function attachLoad(&$queried_entities, $load_revision = FALSE) {
protected function buildQuery($ids, $revision_id = FALSE) {
protected function invokeHook($hook, EntityInterface $entity) {
protected function preSave(EntityInterface $entity) {
protected function preSaveRevision(\stdClass $record, EntityInterface $entity) {
protected function postSave(EntityInterface $entity, $update) {
protected function preDelete($entities) {
protected function postDelete($entities) {
?>
Whereas
load()andattachLoad()are commonly used to prepare/apply default values - i.e., not actually manipulating the load storage operation.That said, base
attachLoad()method on its own is interesting: It loads and attaches field data, and also invokeshook_entity_load()implementations. Both have their own + independent storages, so that logic shouldn't be part of the StorageController.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.
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. ;)
#4
hm, logic isn't very descriptive - it could mean a lot. What about having EntityStorageController + EntityStorageEngine?
There is also buildQuery() or so and delete() which goes with SQL/DB.
Actually, I think this should move into the EntityManager now - opened #1893820: Manage entity field definitions in the entity manager.
#5
Hm, if I can't get a LogicController, can I get a BusinessController? :P
#6
:D I'd not say BusinessController is more descriptive? What about StorageLogicController?
#7
So what's the decision here, how should we proceed.
#8
And yes I am willing to work on this.
#9
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.
#10
buildQuery is a tricky one. How do we deal with that? it's definitely SQL bound and yet it is needed.
#11
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 ?!
#12
Well, if we have customizations of the sql-storage it should be in its own StorageEngine class, e.g. NodeSQLStorageEngine ?
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
#13
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.
#14
>@fago: I think you misunderstood me.
Oh sry. I totally agree that loading should not - or better - must not run any access checks.
Yep!
#15
Opened #1919022: Remove translatable tag and #1919016: Remove term_access tag in TermStorageController::buildQuery() for the discussed tags in the term storage controller.
#16
So now to implement node_access?
#17
#1982880: Refactor the entity system to be evented
#18
Reopening because I think the discussion of how this is split is beyond chx's issue.
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.
#19
Related: #1937266: entity_delete_multiple() bypasses Entity::delete()
#20
*clap* *clap* .. totally agreed with putting that logic where it belongs, in entity type classes.