Motivation
Make entities classed objects based on a defined interface: EntityInterface
. Introduce CRUD support.
Proposed resolution
This issue is about adding the interface and providing a entity-controller which implements full-CRUD support, but excludes revisions for now.
It comes with a test-implementation and migrates the comment entity to be based upon the new system, so there is also a real-life example.
Other entity types keep working with the old system and should be converted in follow-up issues.
Work is done in the CMI sandbox repo, see here.
Example API usage:
// Create a new comment entity.
$comment = entity_construct('comment', array(
'nid' => $node->nid,
'subject' => 'New comment',
));
$comment->save();
// Update a comment entity.
$comment = comment_load(1);
$comment->subject = 'Updated';
$comment->save();
// Delete a loaded comment entity.
$comment->delete();
// Or delete multiple comments.
entity_delete_multiple('comment', array(1, 2));
Remaining tasks
See the full roadmap of followups in:
User interface changes
- None.
API changes
- Only additions for now.
Original report by fago
Let's add basic CRUD support to the entity API.
To start, I'm adding the basic CRUD functionality of the entity API module - which has no revision support yet. So we can refactor and move on based on that, whereas revision support should be handled in the next issue.
Work is done in the CMI sandbox repo, see here.
Attached is a first version, based upon #1018602: Move entity system to a module. It just adds the controller and some entity API functions, as well as basic tests for CRUD, viewing and export.
Apart from that, I've already converted comment module CRUD (but not viewing), what seems to work fine.
TODO:
* Merge core and entity API controllers.
* Overhaul the controller, split it into multiple controllers, e.g. crud, view & export.
* Convert more entity types?
I'm attaching only for this issue + a full one, which includes #1018602: Move entity system to a module so the bot can test it.
Comment | File | Size | Author |
---|---|---|---|
#184 | entity-changelog-1184944-183.patch | 553 bytes | Berdir |
#152 | drupal-8-entity-crud-5315182-152.patch | 53.31 KB | Tor Arne Thune |
#152 | interdiff-138-152.txt | 2.9 KB | Tor Arne Thune |
#138 | drupal_8_entity_crud.patch | 53.33 KB | fago |
#135 | drupal_8_entity_crud-1184944-135.patch | 53.33 KB | xjm |
Comments
Comment #2
aspilicious CreditAttribution: aspilicious commentedOk, I reviewed the doc blocks. You will hate me for now but it has to be done, if not now than later ;)
I pointed every single "error" in the attached text file. I know it is a lot, but I can summarize it:
- some files don't have a newline in the end
- place a newline between @param and @return
- Start with 3th person verbs like Creates and Deletes
- Try to place the "permanently" to the end of the sentence, that way the sentence can start with a 3th person verb
- 1 function was missing docs
- Some constructor docblocks needs a rewrite
- A lot of functions used the same summarize sentence: Implements entity....
It would be better to show what the function actually does in that sentence.
Comment #3
catchSubscribing.
Comment #4
mikeryanSubscribe.
Comment #5
casey CreditAttribution: casey commentedI think we full CRUD shouldn't be required. There are lots of usecases for entity types that'll only support loading (e.g. external sources).
I suggest we have multiple interfaces and a default controller class that implements them all.
Comment #6
xjmTracking.
Subscribers, see also: http://groups.drupal.org/node/133579
Comment #7
fagoad #5:
Initially, I was thinking that too. However, we might want to store additional data even for external sources in fields or modules could do so, in which case we need at least the save() possibility while delete() could delete the local data only.
That means in the long-term we should probably note on the property-level whether something is writeable or not. Once the id-property is read-only, the entity won't really be deleted but only the local additions (=all writeable properties). Still, it would be weird to be able to delete a remote entity while it won't go away..
Given that, I'm not sure whether we need to differentiate read-only entity-types vs not on the controller level. Opinions?
It's a bit similar with the view() and export() functionalities which we might not need for some entities either. However I don't see an overhead in them once we move the implementation out of the storage-controller - so why not have them by default?
If someone calls entity_view() on an entity whose entity-type is not using view() at all, one just gets the default which could be a default-view of the stored data or just nothing. I do not see any overhead in that, while it would enable us to provide a handy $entity->view() method by default.
ad #2:
Puh.. but thanks! :-) I've gone through the re-marks and fixed them, except for the one concerning the EntityAPIController as we have to merge the class first anyway.
>- A lot of functions used the same summarize sentence: Implements entity....
It would be better to show what the function actually does in that sentence.
Well, it is already described at the interface, so I don't think we need to describe it again unless it is something more special. Probably we should fix them to be
Implements Interface::method().
though, so it can be turned into a link by the API module?Comment #8
fagook, I've done some more work. I've changed it to use per entity-type class for common customizations for the CRUD stuff, as well as for import/export. That way modules can extend the default-class to add-in their customizations while they (storage)controller should stay as-far-as-possible as is, so it could be pluggable.
In detail, I've
* moved the create(), save() / delete() customizations to the Comment class
* moved invoke() into the entity-class and changed it to per-hook methods, what should ease specific customizations like do something before invoking hook-presave.
* moved import/export from the controller into the entity-class
* TODO, and not yet moved is view() and buildContent() - it should be fine to have single-view(), and buildContent() in the entity-class too though.
Once, buildContent() and view() is moved out too - the controller would do deal with storage only. While we'd have then a single-controller again, I still think we want to separate a cache-controller from the storage-controllers + support multiple types of controllers so modules can add their own.
Comments about the approach taken?
Comment #10
fagoupdated Git repo to the latest 8.x branch + created new patches
Comment #12
xjmLooks like it is getting a fatal error when it tries to do
EntityCrudHookTestCase->testCommentHooks()
.OT suggestion: Adding a number at the end of of the non-applicable patch (with the isssue-relevant changes only) will make testbot ignore it and maybe save a little time testing. :)
Comment #13
fagoyep, there was another stdclass comment creation.. fixed that.
Comment #14
amitaibusubscribe
Comment #16
tstoecklerI think the newly introduced hooks make sense, but I think the following names would be more consistent / self-documenting:
- hook_modules_pre_install
- hook_modules_post_install
- hook_modules_pre_enable
- hook_modules_post_enable
Comment #17
catchThose new hooks aren't part of this patch, they're from #1018602: Move entity system to a module. Better to review the 'not full' one.
There's a few things in this patch that I'm not sure about:
- I think we should have separate controllers for load, save etc. to make them simpler to mix and match. D7 has a single class just for loading.
- it's a shame comment_save() is so far off the parent controller we have to completely override it.
- why do the extra deletion etc. in the invoke* methods? Wouldn't it be better to override the delete() method and call parent::delete()? Or is the timing bad for that? If the timing is bad we should call those something other than invoke.
Comment #18
dixon_sub.
Comment #19
catchTo be a bit clearer on controllers, I've been thinking something like this, although not sure where the separation should lie exactly:
The idea being:
- usually we will only be loading the EntityStorage class for most requests, sometimes EntityRender too. It seems pointless to load the form for entity types when some may not even use them. If we separated out Load from Storage this would be even more true although that gets tricker.
- When swapping controllers for an entity type (say SQL storage for MongoDB) this can be a bit more targeted - you could leave EntityForm and EntityRender completely alone, or swap EntityRender for a completely different controller that has nothing to do with which storage you're using.
- to enable granular sub classes we need to have quite a lot of little methods on the controllers, having separate controllers means there is a bit more room for all these methods.
Comment #20
fagoYes, there is quite some custom stuff in there. It's not completely overridden though - it calls the parent implementation.
The point is deletion is currently only handled by the controller, but doesn't go via the entity class (in the patch except for the invoke method). We cannot always use Entity::delete() as deletion also has "multiple handling". Probably we should streamline that in some point in the future and make the hook_entity_delete() multiple only just as load.
Still, we need an easy way for the implementing module to do stuff before any other module does stuff. Using the Entity class for such modifications is imho a good pattern as it helps keeping all this stuff together + separates this custom stuff from the storage controller, which should be re-used as is to allow swapping it out (if possible).
Thus consequently, we should add a "static public function invokeHookLoad() {}" method too. Thoughts?
@controllers in #19:
Very much agreed, that's the way I'd envision it too :)
As written in #8:
Thus, I think we should have Entity::buildContent() - which can be easily overridden and customized. Then entity_view() remains, what does not really require per-type customization. That way we won't even need the render controller. Thoughts?
I still think that's the way to go. Supporting multiple controllers is fine for implementing further stuff for entities, e.g. an access system or contrib stuff like views integration. So we have a standard way of implementing default stuff that allows for en(/dis)abling/customizing/swapping implementations.
I'm not sure about that. Usually one would want to swap all storage-related stuff out together, not? E.g., replace the sql controller with a mongo-db controller?
@swapping-storage:
Well, the swappable-storage controller is just half the battle. What about schema-upgrades or adding/deleting indexes? If this should work we need to make them storage back-end agnostic, so e.g. mongodb can ignore schema upgrades but apply indexes. Still, we are already far if anyone can easily build a new entity type with storage backend XY - we should reach that once we are done with this issue!
Comment #21
catchRight I think the storage controller we'll want to go for a single instance per entity-type that can handle multiple entities. Deletion is interesting though since we only require an array of $ids for that. $node->deleteMultiple($ids); feels a bit weird. Definitely hook_$entity_delete() should be multiple whatever we do though - it not being multiple makes things like migration rollbacks (or any bulk deletion) very slow. I think there's already an issue for that somewhere.
I think it makes sense to do this in the class, I would just rename the methods - for example to preLoad() and postLoad() or anything that's more generic than just invoking the hooks
Why should it be a static function?
I'd like to be able to swap out view controllers. I worked on some very rudimentary render caching for nodes in http://drupal.org/project/performance_hacks and that really, really suffers from a hard-coded rendering pipeline. Also feels like it would be strange to have some logic inside controllers and some directly in the Entity class, this is where it starts to get tricky :)
However we should maybe tackle rendering in another issue?
I'm not sure about this either. I'd quite like it if we could keep code that handles deletion and saving separate from code that just loads - since we can save loading a tonne of code that way for 99.9% of requests, but in terms of swapping out controllers it makes sense to keep them together. There are some minor things like replacing straight static caching with an LRU cache etc. in the load controller which does not need to worry about save/delete/update etc. but I'm not sure that should be done at the level of storage controllers anyway.
Yeah we will need something similar to hook_field_schema() if we do this, and that is not without its problems.
Comment #22
fagoAgreed.
>Why should it be a static function?
Because the hook is a *multiple hook*. I thought of having methods that directly map to the available hooks. That way it is consistent with what people already know + more efficient than invoking many many methods in case of mass-loading or deletion.
So they are very much like the hooks, but allow the providing module to ensure they come first or last.
Well, I think everything that usually needs to provided or customized by the implementing entity-type module, should be in the entity class. So you just need to do your own custom entity class and you are done. That way you easily get an overview of all the methods you might want to customize.
buildContent() really is what the entity-providing module needs to provide (except for some automated fields stuff). So I'd move buildContent just into the entity class, and keep view() implemented out-side as it can be implemented generically.
I think your use-case for having view() in the controller is very valid, so let's do it that way.
>However we should maybe tackle rendering in another issue?
Agreed.
Well, usually save() would be handled by the controller and does not need to be overridden. Thus, we'd have it only once per storage controller, so I don't think loading this little code really is an issue.
I'd like have cache controllers though, so we could not only simply plugin in/out different caching strategies but also this would help a lot to make implementing new storage controllers much simpler.
Comment #23
fagook, I realized this is not going to fly as we would have to apply this rule to other controllers to, what is not possible as they need to be extendable.
Entities are representing your data in Drupal, so the class should only contain methods related do the data model itself. Stuff that makes use of the that should live outside (in controllers) so viewing.
Comment #24
catchFrom the lazy load issue it feels like we're moving towards having a single controller instance per entity type, that can handle loading multiple entities - with each individual entity instance mapping to that one controller instance. If we go that way would that remove the need for the static method?
Yeah I'm leaning towards this as well. I started work on #1199866: Add an in-memory LRU cache a little while back with the intention of allowing that to be used by the current controller. However it'd be much cleaner to have a cache controller, specify an interface, then just swap the class used for that. We could have at least the following:
- null cache (for comments/files that aren't statically cached now due to memory - this has potentially issues with PHP overhead since a class that does nothing won't be completely free compared to actually doing nothing).
- static cache - same logic as now.
- LRU.
- I have my eye on weak references http://pecl.php.net/package/Weakref
- any combination of the above + cache_set() / cache_get() per http://drupal.org/project/entitycache
It'd be really, really great to have all of this potential logic outside of the storage controller (or at worst in methods in that controller which won't need to be overridden most of the time).
Comment #25
rlmumfordsub
Comment #26
yched CreditAttribution: yched commentedStaying out of the Entity class / Controller class architectural debate for now ;-), a couple down-to-earth remarks after a visual review of #13 :
I don't think I get why there is an $entityType param here. __construct() can have different signatures along the inheritance stack, right ?
Similarly, it seems odd that Entity::_construct() accepts an optional $entityType param only to raise an exception if the param is missing.
Could this be :
A separate copy of the entity type info in each actual entity - memory bloat ? Does this really bring significant CPU gains over directly calling entity_get_info() in Entity::entityInfo() ?
'full' is no longer a required view mode for all entity types (it used to be at some point in the D7 dev cycle, so most/all core entities still have one), so strictly speaking, we can't provide a default view mode that makes sense for all entity types.
I'd suggest that the default value is only provided by Node::view(), Comment::view(), etc - as is currently the case in core, IIRC, i.e. $view_mode optional and defaults to 'full' in node_view(), but is required in field_attach_view().
(also applies to Entity::buildContent(), and the corresponding methods in EntityAPIControllerInterface)
It has been suggested in #1018602: Move entity system to a module that field_attach_OP() get turned into field.module implementations of hook_entity_OP().
If so, then probably the 'generic' hooks should be invoked before the 'entity-type specific' hooks, so that all can consistently run *after* field ops have run (provided field.module gets an extra-low {system}.weight) ?
'bundle of' ? Do we really need this at this point ? I think I remember the concept is a bit controversial in contrib Entity API D7 ?
Shouldn't that be a prerequisite on the incoming $entities array ? That's extra CPU if the incoming array is correctly keyed.
Do we intend to support StdClass entities as well, or is this just a temporary BC layer ? (If the latter, maybe we should add a @todo ?)
Does the import() / export() part belong in this initial patch ? It seems it would be more easily reviewed in a followup ?
Same for template_preprocess_entity() ? (+ I don't see the corresponding theme_entity() or entity.tpl ?)
24 days to next Drupal core point release.
Comment #27
plachComment #28
gddsubscribing
Comment #29
fagoUpdate:
As discussed and suggested above I've removed all view and import/export related code for now. Then, I've added in the EntityInterface which we defined in London and implemented it with the Entity class.
Yep, that would work. However we need an interface for construct, so storage controllers and entity-classes work together based on the interface. Thus, we have to define the parameter of construct. I've changed it to always get $entity_type passed, as we need it for the "Entity" class + it doesn't hurt for the others as developers should use entity_create() for object creation anyway. Then, I've done away with the check and the exception as we cannot account for any misuse of the code we write anyway.
That was a left-over, thanks removed. I also removed some other unrelated entity-api.module improvements, i.e. load().
I'm not sure. It would be an API change. Anyway, I think we should deal with cleaning up the entity api <-> field API integration in a separate issue afterwards. Makes sense?
Nope, everything has to be an instance of EntityInterface now. For now this applies only to the converted entity-types though. Fixed that.
Thinking more about this, this would lead to add method names like Entity:prePresave() or Entity::preInsert(), while pre-insert actually means after insertion but before hook invocation. Perhaps we should call them pre/postHook$name, i.e. e.g. preHookInsert(), postHookInsert()?
Comment #30
fagoComment #32
klausishouldn't we type hint the $entity parameter here? we don't want that arrays or stdClass objects are passed in here, so maybe:
is better?
Comment #33
fago#1018602: Move entity system to a module was not applying any more. I've fixed that, merged banches and re-rolled the patches.
ad #32:
Agreed - I've added typehinting + added the interface to the doxygen.
Updated patches attached.
Comment #35
fagoI was not able to reproduce the System-Info-Alter test fail locally. Anyway, re-rolled the patches with the latest #1018602: Move entity system to a module patch. Maybe they pass this time.
Comment #36
fagoAs seen in the @todos the patch misses proper language handling. I've opened #1277776: Add generic field/property getters/setters (with optional language support) for entities to discuss that.
Comment #37
catchOK did not review every line of the patch but here's a few thoughts reading through. Mainly nitpicks, not up to much else today.
Do we still need 'entity keys' etc? This feels like it could just be defined by the classes, and put some sane defaults in the Entity class itself.
It's not related to this patch really but we should consider calling this EntityDatabaseStorageController rather than default. Would match caching API and Field API more.
These should all be on their own lines with their own docblock.
Maybe just preSave()? This does a lot more than invoke the hook. Similarly with the other methods.
How can we get here without a proper bundle name? Or why would it vary?
If we moved this to a protected class method it might possibly mean less work for extending storage engines, that's probably wishful thinking at this point but you never know. Either way hopefully entity_tree can replace that logic with some nice API functions.
Change _comment_update_node_statistics() to a method?
I think we should leave uuid for a followup - no core entities have uuid support yet.
Is all of this needed in the entity class as a copy? Essentials like idKey etc. should never need to be altered (and if for some reason they did could subclass). Is the rest of it used at all?
This feels like potentially a regression. It's possible to set the ID of an incoming node in Drupal 7, and also set is_new.
Why not move that logic into the class? Or not have the method on the class at all? Feels like duplication at the moment.
Could we switch to entity($entity_type)->delete($ids); for the public API? Would save all the wrappers.
The naming here confuses me a bit, maybe because we have field_create_field() which actually saves the field?
Comment #38
plachtagging
Comment #39
bojanz CreditAttribution: bojanz commentedSubscribing.
Comment #40
plachWhile working on #1277776: Add generic field/property getters/setters (with optional language support) for entities, I realized it would be really cool for DX to exploit the
__call
magic method to provide "real" property accessors. Consider the following scenario:instead of
Providing defaults for
$delta
and$column
parameters could be possible also in the latter case, but the first one lets us override the accessor through a class extending the base entity class. I think this is very valuable from an architectural perspective.I'm not saying we should drop the
get()
/set()
methods since they allow us to deal with dynamic properties, but when dealing with defined properties/known fields this would greatly increase DX.I can see two problems with this approach:
_call
invocation would add an overhead for each property access, which should be added to the one already introduced by the actualget()
/set()
pair. A__call
method implementation might look like this:parent::getBody()
call from within an overriding method.Overall I ain't sure this is a viable solution, but I definitely think it's worth a try.
Comment #41
chx CreditAttribution: chx commentedI do not like this name. I know relation uses it too but isn't create kinda reserved from CRUD for creating the entity in a permanent way?
Why is a function called invoke* do all sorts of stuff? That's for implementing a hook and not invoking it.
Insert rant against settter/getters. I realize it's necessary due to multilanguage. Allow me to hate it nonetheless and then move on.
See above -- save() and CRUD kind of does not mesh. What about
entity_construct
instead of create, hm?*blink* so this is not the constructor. It's not even called from the constructor. The doxygen says " Set up the object instance on construction". Color me confused, please.
These need to be final IMO. Quite in league what I said above about invoke* implementing.
You seriously write down
array(array(), $this->entityType)
without a nick of a comment :) ? What's that first empty array, huh?EntityAPIwhateverinterface? I know most of this code comes from Entity API project but we got rid of API even in hook_nodeapi.
14 days to next Drupal core point release.
Comment #42
Crell CreditAttribution: Crell commentedI am honestly not disposed toward using __call() at all if we can avoid it. It can easily obfuscate the real logic under the hood in difficult to comprehend ways. We use it in the DB layer for extenders only because we had no other viable option, and even there it's rarely called.
Comment #43
plach@Crell:
Ok, but besides that, what is being planned here is trying to unfiy field and property handling. If I'm not mistaken you don't like this approach, right? The main idea behind this (@fago correct me if I'm wrong) is having the ability to reuse concepts like widgets and/or formatters at a lower level. In this scenario property would be implemented by a Property class and fields would extend it (or something like that, not really got into the implementation details).
Comment #44
catchI have wanted to use __get() in #1237636: Entity lazy multiple front loading since that would allow viable entity objects without having to actually load all the data, but that's postponed at least until patches like this are in. Using __call() is a similar conversation to have and it'd be good to avoid it here.
For language, could we do something like $entity->setPreferredLanguage($langcode); rather than passing an argument to get()? that would mean you only have to set things once then you can just access properties as usual, and if you don't set it you get the default. It'd also feels a bit easier to move that to depending on context later on.
Comment #45
plachYes, this might be a good idea, I think D7 Entity API already does something like this. But probably we should move this dicussion to #1277776: Add generic field/property getters/setters (with optional language support) for entities.
Comment #46
catchWhile there's more in this patch than I'm completely comfortable with, it's also adding stuff that probably needs to go in here but that's not reflected in the issue title, updating it to look a bit sexier ;)
Two things with what is currently called entity_create():
We need to allow people to construct the entity object with the entity ID - there are several use cases for this:
1. Migrations where you want to retain the ID. node_save() supports this via $node->is_new (to differentiate between new entities with IDs vs. entities being updated).
2. Potentially allowing updates of entities without loading the original entity. Again migrate module currently supports that, possibly web services might want it to. If we drop it, we need to do it consciously.
3. per #1237636: Entity lazy multiple front loading wanted to do this for consistency, and for fun tricks.
That leads to whether entity_load() should instantiate the class from the database, or instantiate the class then call a load() method that fills in from the db.
Comment #47
fagoI agree, though we have to keep it as long as APIs (old+new) co-exist. We should remove it from the Entity class though. However, I think we still need to have the possibility to know which property is the bundle property. I'd move that part over the not yet existing entity-property-info stuff - that's another issue though.
@entity_label()
As not all entity types are converted yet, we need to keep the old API working too for now. So should we dupcliate the code now by copying it over, or clean it up in follow-up? I'd tend to the latter, as removing the 'label' entity-key again has more impact: it'd would remove the possibility to query by label.
Agreed. I'm not so happy with that definition anyway, but for now it's imho the best possibility to define default values for properties. It does not work for module defined properties though, so it would be more consistent to have no defined class properties at all. Then, for setting default values we would have to come up with something new like defining those in that hook_entity_property_info()... Any thoughts on having entity properties as defined class properties?
@invokeHookPreSave()
ok, I've discussed that issue with sun this weekend. We come up with the following:
* The entity storage controller should be implementing storage logic, but not the actual "storage engine". Thus its not the right place of implementing swappable storage. If we want that we should do a separate layer, similar as the proposed document-orient-storage of damien. Anyway, we are far from really having swappable storage, so I think it makes much sense to *not* restrict ourself by that far away vision now. (Still, entity types could easily use the controllers to implement custom storage, remote storage or whatever, but it's not swappable.)
Consequently, there is no reason to move this stuff to the entity class, so we can keep it in the storage controller.
Agreed. Me and sun come up with the idea of adding preSave() and postSave() methods to the storage controller, which are called right before hook presave and insert/update. That should suffice to implement 90% of the need save() customizations without having to override the save() logic.
I like that idea. I've discussed that + the whole multiple-controller idea in detail with fubhy and sun this weekend in Berlin. fubhy is working on a summary of that.
@entity_create()
Looks like you are not the only one ;) I'm already used to entity_create(), but I can see the potential confusion with it. Thus, I'd vote for entity_construct() (as chx came up with).
oh, it's called by __construct()?
Right.
Oh yes, let's not do that. I've tried going that way with faces and I can say it sucks out of experience (at least for PHP).
Yep, but let's postpone that for now, so we can concentrate on the more foundational issues now.
pwolanin suggested in London to not support it for all entites, as it's hack. I agree it's hacky and I'm not sure it's still needed once we have proper UUID support... ?
But we would drop it *if* we would make entity_construct() setting is_new. However if we don't, you still could set the id-property manually afterwards - so it should still work. Again hacky, but not more as than it is now.
I'd prefer removing the is_new property, as it makes the behaviour of the API sane: there is no identifier <=> it is new.
Comment #48
bojanz CreditAttribution: bojanz commentedI find entity($entity_type)->delete($ids); several orders of magnitude more ugly than entity_delete($entity_type, $ids);
But then again, I never got completely used to the "it's a function, it's an object, it's a bird, it's a plane!" way of doing this that appeared with dbtng.
Can you give me an example of "module defined properties"? I find it pretty logical that if node is a class, I can see its properties as class properties.
+1 on entity_construct() and avoiding __call(). And I think it's fine to take "is there an id" as a way of determining if the entity is new. I've written migrate handlers and plugins, and that's the way that made sense to me.
Comment #49
chx CreditAttribution: chx commentedAs for " The entity storage controller should be implementing storage logic, but not the actual "storage engine" if it makes no sense to you it doesnt to me either. Here's what I came up with today: remember node hooks? hook_insert? Which is not a hook? Right. So these, because they are not hooks become methods. The rest, those are hooks and stay hooks. fago said this is in line with what he thinks.
So, can we make the invoke* methods final to indicate no override?
Comment #50
Crell CreditAttribution: Crell commentedWhy do we need the invoke method to be final, exactly? I missed that part.
I agree with chx that the hook_load et al pseudo-hooks map to methods more naturally. However, those are per-node-type (bundle) at present. I don't believe bundles having their own classes is on the table. So that doesn't really map to the new structure.
The controller for an entity type having a load() method makes sense, but I don't think hook_load has any equivalent anymore, nor should it. hook_node_load() can do everything hook_load() can do, only better. hook_load and friends should just die completely.
Comment #51
catchThat works for me.
Sounds fine but either a @todo or @deprecated on entity_label() or something would be good to track this.
I realised this and completely agree - this also at least for now sidesteps the fact that the entity controller supports hook_schema_alter() and adding columns to base tables etc., for which we can't add defined properties (but which are otherwise indistinguishable). Also if we move things like {user}.init and {user}.picture out we won't need to change class definitions.
Hmm what kind of default values? I'd think the schema definition is enough?
@invokeHookPreSave()
I'm fine with punting on this. Very simple entities should have no special logic in the class. Core entities that have legacy crap you could still subclass it and handle the crap yourself.
Sounds great!
That's a good change yeah.
Well we don't have that yet and it's a bug in core that you can't do this for other entities, there are at least three scenarios we need to support:
1. Entity does not exist anywhere, ID generated by the system.
2. Entity exists somewhere else (if we use migrate for major version upgrades this could even be a D7 site) and we want to insert it but specify the ID.
3. Entity exists and we want to update it.
As long as we can support those three use cases that works for me, but just the presence of an ID or not can by definition only support two at the moment.
Do you mean entity_construct(), then once you have the ID (which is internally tracking is_new()) set the ID on the object before saving? If so that seems fine - I care about the use case more than the property.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe.
Comment #53
moshe weitzman CreditAttribution: moshe weitzman commentedFYI, user_save() supports it too.
Alas, UUIDs don't help enough. For the D7 => D8 upgrade, we need to migrate with specified IDs because we need URLs to not break. This problem exists when migrating from other systems into Drupal as well. ID preservation vitally simplifies data import.
Comment #54
mikeryan+1 on ID preservation, for the sake of #1052692: New import API for major version upgrades (was migrate module).
My personal goal for this effort is that Migrate no longer will need node/user/comment/etc. destination handlers - one entity destination handler should be sufficient for all properly-implemented entities. That'll be a huge win, not only in Migrate's own core entity support but by making it unnecessary to write specialized destination handlers for many contrib modules.
Comment #55
chx CreditAttribution: chx commentedWhy do we need the invoke method to be final, exactly? I missed that part.
because invoke , well, you know, invokes hooks. There's nothing more to be done there.
Comment #56
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #57
fagook, I've updated the patch and included the pre/postSave methods. Please review!
As always I've pushed the changes also to the Git repo.
I tried removing it, but it's still used and needed by the storage controller. So, I think for now it's best to keep the entity-keys as is. Maybe we can find a good way to re-factor that in a follow-up.
Makes sense, changed that.
Fixed that.
It wouldn't vary and it looks like it doesn't happen. So I've removed that check.
As noted above pluggable storage engines are probably best implemented in a lower-level. However, we could still ease implementing extended comment storage controllers that take care of everything storage specific "manually", e.g. convert needed queries to another storage system. To ease that comment module should probably use efq queries as far as possible and move all others in their own storage controller methods, so one could easily override that.
Still, if we want to do so I think this deserves its own issue though.
Makes sense to me, I've moved it to the storage controller.
I've done so.
It's not really needed as it could just directly access $this->entityInfo or even do entity_get_info() each time. Still, they are handy shortcuts as we have them in the controllers + they are used.
Copied the code over for now and added an @todo to remove entity_label() once everything is converted.
Added a comment.
Right, done so.
ok, renamed entity_create() to entity_construct().
ok, I've added support for the is_new flag in $entity->isNew(). That should do it.
Indeed, still I don't see a point in forcing the methods to be final. It's not part of the interface in any way so anyone could still implement it different in a new class. Next, I'd say customizing hook invocations is totally valid, e.g. to add further arguments to an entity-type specific hook - as we have it already for hook_node_load().
Comment #58
bojanz CreditAttribution: bojanz commentedI like what I see. So I'm gonna skip the nitpicking and go straight to the tricky stuff: class VS controller ( + storage concerns).
I see no reason to have the "storage engine" (MySQL, MongoDB, Document Storage) be separate from the storage controller, in another layer of its own. Seems like we'd add another layer of complexity for no good reason.
Making that possible on the storage controller level would then necessitate the following rule: if it touches storage (like updateNodeStatistics()) it goes into the controller. If it's generic entity stuff, it goes into the class. Of course, we try to hit the metal as little as possible, so the overriden controllers can be as small as possible.
I see the following code in the comments controller and see no reason why it shouldn't be in the class:
(Why not in the entity class constructor?)
or
and
from the controller preSave() method. Which could easily be in the entity class save() method (overriding the method, and calling the parent, like Entity API does in contrib currently).
It also makes for better DX. If I'm implementing a new entity type ("product", for example), I just create a custom entity class, and I'm done.
With the current approach, I would need to extend the storage controller as well, just to add the three lines of code that handle $entity->created and $entity->change (any reason why we don't require at least "created" on all entity types?). So instead of extending one class, I'm extending two. And of course, this makes doing alternate storage through the controllers impossible, because a new storage engine would need to override every single storage controller, which is impossible if every entity type needs one.
So it comes down to:
- One class per custom entity type, and custom storage engines overriding the controller
VS
- Two classes per custom entity type, and another layer of classes just for storage engines.
The first option is clearly more simple. Now let's hear your objections :)
Comment #59
fagoGood choice ;)
Initially I thought this is a good idea too (see some of the earlier comments). Still, having pluggable entity-storage controllers implementing different storage engine doesn't fly. I cannot really see how it should work:
So, I don't think this is going to work properly unless we have an underlying API that cares about the stuff like indexes and allows for queries being executed regardless of the storage engine. The document orient storage Damien suggested would provide us with exactly that.
Then, how would the two-class approach look like? I shorty tried implementing it in a sane, consistent way, but that requires you to support operations on load() and deleteMultiple() too. That would be static and not going to be nice. Also, if we leave db-querying stuff in the controller we end up with parts of the code in the controller and in the entity class.
So implementation wise it's easier and cleaner to just have everything in the storage controller. And as we have no concrete vision or plan how the pluggable-storage-controller stuff should work, I don't think we should perform any contortions to obtain somehow, in theory pluggable storage ;)
Still, we might want to think about moving all queries to functions in the storage controller, as at least it would allow pluggable storage provided by customized controllers (write your own no-sql CommentStorageController). There are more considerations for that we'd have to make (how can modules add their queries?), so it's questionable whether this is a good direction to pursue and out-of-scope for this issue.
It doesn't make much sense to have it in the constructor as it is dealing with the custom stuff that has been loaded via adapting buildQuery(), which won't be available when you invoke entity_construct(). This really is about customizing loading, so attachLoad() should be fine.
(That's the way it already works in D7 I've not changed that code).
Yes, but there is also stuff in preSave() that does db-queries, so it makes sense to have that in the storage controller. I agree that just setting some simple defaults would be fine in the entity-class, but it would require the code-flow jumping back and forth between the two classes and so make it unnecessarily hard to follow. And for end-developers (credits for that expression go to fubhy) it should be simpler to have just a single place to do customizations, as there is no need to think about which of the options is the right one for a certain case.
Any reason to do so? ;)
Here is one to don't: We might want to integrate remote data that doesn't have it. The less assumptions we make, the better. So if there is no good reason to do so, let's don't.
Comment #60
klausitype hinting with EntityInterface $comment would be nice here
type hinting again, and several other places where $entity is used.
you could use class_exists() here. Also please add the entity type to the exception message, so that I know which of my entity info is misconfigured.
Comment #61
bojanz CreditAttribution: bojanz commentedI agree that just setting some simple defaults would be fine in the entity-class, but it would require the code-flow jumping back and forth between the two classes and so make it unnecessarily hard to follow. And for end-developers (credits for that expression go to fubhy) it should be simpler to have just a single place to do customizations, as there is no need to think about which of the options is the right one for a certain case.
I'm thinking about contrib entity types here. The many "small" examples that don't require additional DB queries in the controller, just a bit of wrangling on default values, and cache clears (og, for example).
Would be great if they could just create an entity class and use the default controller, instead of having to create both a class and a controller. Of course, there's nothing in the current patch that prevents that, it's just a matter of setting and communicating best practices.
Comment #62
mikeryanActually, I'd like to require "changed" on all entity types, which would make it simple to incrementally pull content as it changes: #1052692: New import API for major version upgrades (was migrate module). Pulling remote data that doesn't have created/changed? Just default to time() (but PLEASE don't hard-code them, as changed is on nodes now - we need to be able to set them explicitly in migration scenarios).
Comment #63
fagoI see. Adding custom "calculated" defaults to the entity-class should be fine in __construct() anyway.
Still, stuff like custom cache-clears and so on imho makes sense to be in the controllers. Also, in case of multiple operations this is the only sensible place for them - consider deletion. Else we'd have to go with static methods in the entity class, which imho does not help to makes things easier/nicer. I think we can agree upon that?
Then, what remains is preSave(), postSave() which could live at both places. For consistency reasons I'd vote for going with the controller too, though.
As commented above I don't think it's a good idea to require it - that does not mean though we can ahead and add it to all our entity types. Anyway, this would come with quite some (schema) changes and deserve it's own issue.
Comment #64
fagofixed that.
Well, I don't think it's necessary to check it all. It's required so let's assume it's there. If not, developers will get an error message that the class could not be found anyway. That does it without any custom error handling code. -> I've removed the check.
Also, I've re-rolled the patch and fixed merge conflicts. Updated patch attached.
Comment #65
tstoecklerAbout requiring "created", "changed", etc. please see #1295148: Maintain common properties/statistics about entities.
Comment #66
Crell CreditAttribution: Crell commented"changed" on all core entities to establish a convention I would support. (The lack of that on user entities was a problem when writing the DrupalCon Mobile app.) As a required part of the API, though, no.
As for the entity/controller split, remember:
- The job of an entity is to "be" something.
- The job of a controller is to "do" something.
The CRAP logic belongs in the controller. The entity can/should have convenience methods, but $entity->save() and $entity->delete() shouldn't do anything more than call $this->controller->whatever().
Comment #67
Crell CreditAttribution: Crell commentedOops, crossposted.
Comment #68
bojanz CreditAttribution: bojanz commentedNice explanation, Crell.
And your point about cache clears makes sense, fago.
I have no more points about the class / controller split. Makes sense.
Comment #68.0
fagoUpdated issue summary.
Comment #68.1
fagoUpdated issue summary.
Comment #68.2
fagoUpdated issue summary.
Comment #69
fago@bojanz :)
I've added an issue summary.
Comment #70
chx CreditAttribution: chx commentedAs now all the invokeHook* methods literally are the very same, I would merge them down into a
final invokeHook($op, $entity)
method which also would fire$this->$op
to save further code instead of all this$this->preSave; $this->invokeHookpreSave
fago says postSave is not a hook, well, bummer, just create such a hook :) it's great we are hiding the insert/update details because eventually I want to move from CRUD to CRAP anyways.Comment #71
aspilicious CreditAttribution: aspilicious commented@chx
I openened an issues a few months ago to standarize the way we handle hooks. That issue tries to remove the last '$op' parts from our api's as I was confused why we removed most $op arguments but not all. I don't think it's a good thing to introduce new '$op' arguments.
Maybe I'm just wrong and this isn't related at all but I felt I had to share this info :)
Comment #72
klausianother case for type hinting to EntityInterface I guess. This also applies to all other comment_*() functions in comment.module that have a $comment object as parameter. Would be a good test to see if we might have still some stdClass comment objects somewhere.
Comment #73
klausiSame patch as in #64, only added more type hinting. Want to see what the testbot thinks.
Comment #75
klausiA-ha! Ok, comment_publish_action() and comment_unpublish_action() was not a good idea, the comment parameter is optional there and can be NULL. Reverted them.
Fixed the occurrence of a stdClass comment object in comment.pages.inc with entity_construct().
Comment #76
klausifago pointed out that type hinting can be used with NULL default variables, so i fixed that for comment_publish_action() and comment_unpublish_action(). Also added an interdiff to fago's last patch for reviewing convenience.
Comment #77
Crell CreditAttribution: Crell commentedPerhaps a smaller issue, but this function name is completely confusing to me. The Docblock doesn't answer either. Is this for creating a new, not-yet-saved entity out of thin air, or for loading one out of storage?
The "construct" name to me implies "uses the constructor", which still tells me nothing. This is not self-documenting and I would argue poor DX.
If the goal here is to create a new instance of an entity out of whole cloth (which I think is the case, but I'm not actually sure), then it should be called entity_create(). That makes it clearer that you're creating a new object, not loading an existing one (a process that could very easily involve a constructor or two).
Comment #78
fagoThanks klausi, doing that makes sense to me. Here a review:
This seems unrelated. It belongs to the implementation though, so I've moved it over into the controller.
This is still not a valid entity object, as it by-passes entity-load for that given comment. Use comment_load() or just work with your database $values.
The invoked methods are rather different than the hooks now, e.g. the methods for deletion are working with multiple entity objects already, but the hook is still single. Yes, we want to work on improved entity hooks too, but this is out of scope for this issue.
As discussed in IRC, I agree with moving to a centralized invokeHook() method though (back to the start!). I've improved the patch to use such a centralized method + kept the method calls to $this->preSave(), postSave(), .. out of it - as they are different and no hooks anyway. Maybe we can re-factor that later on once we've improved entity hooks though.
Updated patch attached. For interdiffs check the Git repos as always.
Comment #79
fagoComment #80
fago@entity_create() vs entity_construct()
Short discussion history:
#7:
I tend to agree with crell. As klausi's patch has shown entity_construct() makes it appear as it would be fine to pass in property values sucked out of the database to get your entity object - but it is not. You *have* to use entity_load().
So entity_create() makes clear it's just for creating new entities + is already used in D7. That + adding the documentation "it does not save" should make clear how it works.
Comment #81
indytechcook CreditAttribution: indytechcook commentedFrom Entity::label(). This will never get called since $entity is undefined. Should be replaced with $this.
Comment #82
bojanz CreditAttribution: bojanz commented#78 no longer applied (failed hunks in simpletest.info and entity.info, because of entity_crud_hook_test.test)
Rerolled, addressed #81 (not included in the interdiff because I'm clumsy), renamed entity_construct to entity_create, and added a comment that explains the saving:
The note about saving could go in a new row as well, it was short enough that I didn't think it deserved a new paragraph.
Looking at it now, "Construct a new entity object without saving it." might be the best option (why assume the database...)
Comment #83
indytechcook CreditAttribution: indytechcook commentedOn the create/construct line of comments. It's very common in other ORM's (Active Record in Rails, Django, Lithium, etc) create is used to initialize the object without saving it so this make the most sense.
Comment #84
fagook, let's go back to entity_create() then!
#82 looks good - I've added it to the Git repo (it already included the entity label fix too) and re-rolled the patch.
Comment #85
fagore-rolled the patch
Comment #87
fago#85: drupal_8_entity_crud.patch queued for re-testing.
Comment #89
indytechcook CreditAttribution: indytechcook commentedCreating files in modules instead of core/modules
Creating files in modules instead of core/modules
Creating files in modules instead of core/modules
Comment #90
fagoindeed, thanks. Looks like Git is not able to magically resolve everything..
Comment #91
xjm@fago -- It probably could; we just don't know the esoteric commands we need. ;)
Comment #92
xjmAttached replaces lowercase 'id' with 'ID' in the new comments added by the patch. (We had it both ways in #90, and the uppercase 'ID' is preferred in documentation.)
Comment #93
catchBumping this to major. Lack of full CRUD support for entities is a major deficiency of the Drupal 7 entity API (without the contrib module), and this also blocks some CMI work.
Comment #94
aspilicious CreditAttribution: aspilicious commentedShouldn't we do this or is it for a followup
Functions without any docblock, many of them in this patch. I think if we are overriding a function we have to write *something*
3th person verbs needed
Better docs needed
After the entity is deleted? needs more explenation, we were confused in irc. So other will be to.
These are only a few points, we need to get the docs correct before this gets into core.
20 days to next Drupal core point release.
Comment #95
xjmIn general, all the classes, functions, and methods should have summaries that begin with a third-person verb and explain what each does. See http://drupal.org/node/1354#functions and http://drupal.org/node/1354#classes.
Comment #96
xjmI'll work on polishing the docs a bit.
Comment #97
xjmComment #98
xjmFirst pass at making sure all the doxygen meets our standards as defined in http://drupal.org/node/1354#classes. Please review the interdiff for changes.
Comment #99
Dave ReidNot sure if anyone has noticed but this changes the order of delete hooks to run after the item has been deleted rather than before (comment seems to be the exception in D7 but it's also wrong by being the exception).
Comment #100
xjm@Dave Reid -- if I understand correctly, there are both preDelete() and postDelete() methods which run on either side of entity deletion. Does the current implementation use the wrong one relative to delete hooks? (Also see the added comments on the methods near the end of my interdiff; could you check if they are correct?)
Comment #101
Dave Reidhook_node_delete() and hook_entity_delete() have always run prior to the node record actually being removed. Same with users and files. Comments and taxonomy terms invoke hooks after deletion. Ugh, as long as we are consistent with this I guess - but it will be a functional change for several entity types in hook execution order.
Comment #102
xjmComment #103
xjmIf someone who knows this patch well could look over its @todos and either clarify them or open followup issues, I think that would also help this move forward.
Comment #104
xjmAlright, I looked over all the @todos in the patch:
These relate to default language handling.
This seems kind of important?
These are related to functionality that should be removed once all entities are using EntityInterface.
There's also Dave Reid's outstanding question as to deletion order. (Personally I found the fact that taxonomy hooks ran after deletion to be a significant pain in the backside.)
Comment #105
Gábor Hojtsy@xjm: As written above fago opened #1277776: Add generic field/property getters/setters (with optional language support) for entities to discuss the language question. I think conclusions from there should be brought over :) I'm very excited that this patch progresses :)
Comment #106
Dave ReidYeah the property and language handling as of the recent patch is really confusing. It's going to cause problems the moment someone does set('property_name', 'value', 'en') and then someone else does get('property_name') as it will not return 'value' but rather array('en' => 'value').
Comment #107
RobLoachThis is pretty awesome... Quick note that we now have the Symfony auto-loader in Drupal core, so although it might be out of scope for this issue, we could register Symfony namespaces for the entities and then load class files and create new instances of those classes via... $comment = new \Drupal\modules\Comment();
Again, probably WAY out of scope for this issue.
Comment #108
Crell CreditAttribution: Crell commentedRob Loach: Eventually yes, but we haven't figured out yet how autoloading will work for modules. Please stay tuned. :-)
Comment #109
webchickTagging.
Comment #110
xjmRegarding deletion...
Maybe we should just add a hook_predelete() to the API? That way modules can pick which they need. That addresses Dave Reid's concern and spares some of the horrible things I've had to do with taxonomy submit handlers in the past. ;)
Hmm, why does the type-specific hook come before the entity-level hook?
Comment #111
xjmxpost :P
Comment #112
Gábor Hojtsy@Dave Reid: agreed.
Comment #113
fagoGreat to see so much movement here! :)
#92 and #98 look good to me, I've added them to the Git repository.
Yes, that's pretty inconsistent for D7 :(
I think entity_delete() should be actually post-delete though. Other hooks are post-operation hooks too if not mentioned otherwise (think load, insert, update). Thus, having predelete() + delete() would imho fit best.
That makes much sense to me, but probably better we postpone it to a follow-up? We should convert delete() to be a multiple hook too, so maybe we could just do a "fix deletion hooks" follow-up.
Ouch. I've removed that language specific code for now. Let's work on fixing it in #1277776: Add generic field/property getters/setters (with optional language support) for entities.
* @todo Add invokeHook*() methods.
That's already solved by the preSave(), postSave() methods. I've removed the deprecated todo.
That's our usual ordering.
I've updated the Git repo and re-rolled the patch. Please review the changes.
Comment #114
moshe weitzman CreditAttribution: moshe weitzman commentedI'm sure that that comment_delete has to run before node does its delete or else bad things happen - #890790: deleting nodes does not delete their comments.. So it needs that 'preDelete' hook
Comment #115
fagoad #114: yep, but we don't need it *now* for comments - that applies to node_delete_multiple(). In #113 I meant to postpone this to a follow-up, or do you think that should be already covered by this one?
Comment #116
fago@language: Talked to Gabor, catch and xjm on IRC and agreed upon postponing property language handling to a follow-up, so we can move on with this one faster. Thus, I've just removed the property getter/setters for now - let's deal with that in #1277776: Add generic field/property getters/setters (with optional language support) for entities.
@deletion: I've created #1346032: Ordering of hook_entity_delete() is inconsistent to fix this.
Updated patch attached.
Comment #117
xjmI read #116 closely and it looks fantastic. I added the remaining
@todos
from the patch to the roadmap in #1346204: [meta] Drupal 8 Entity API improvements, under step two. I think we should also leave them in the codebase for now, though.I'm working now on clarifying a few comments I found confusing and a couple other (mostly grammatical) fixes. I'll upload that shortly.
Comment #118
xjmAttached:
Comment #119
xjmAlso, I think #1346204: [meta] Drupal 8 Entity API improvements addresses all the outstanding questions above, so let's get some final reviews? :D
Comment #121
xjmBot goof. (Opened #1346772: StatisticsLoggingTestCase->testLogging() fails with clean URLs in some environments.)
Comment #122
xjmaspilicious reviewed for me in IRC and found a few more things to clean up:
Comment #123
xjmAlright, the attached clean up the issues mentioned in #122. However, the exceptions are still a little weird:
interface EntityStorageControllerInterface extends DrupalEntityControllerInterface
is defined inentity.controller.inc
.class EntityStorageException extends Exception
is defined inentity.class.inc
, notentity.controller.inc
, and is not (yet?) used anywhere.EntityInterface::save()
andEntityInterface::delete()
claim they throwEntityStorageException
, but this isn't the case anywhere (yet?).EntityStorageControllerInterface::save()
andEntityStorageControllerInterface::delete()
throw a plainException
. Doesn't it seem likeEntityStorageException
would have been intended for this, not the base interface?Can anyone clarify what's going on here?
Comment #124
fagoThanks, I've incorporated #118
Not sure what you mean here, could you clarify? Using two lines for the statement makes sense to me though, as it's better readable.
@Exceptions: Indeed, that part wasn't really completed (should have been marked todo). We should make sure to only throw storage exceptions. Please, review the attached interdiff. It also contains a comment improvement for entity_test_load().
Comment #125
fago@$build['comment_form']: Oh got it, it's just about the two lines :)
-> I've added those changes too.
Updated patch attached, which contains all of xjm's improvements as well as the interdiff from above (#124).
Comment #126
aspilicious CreditAttribution: aspilicious commentedOn one line (pid before nid)
On two lines (nide before pid)
On two lines but different than the previous one
I would like us to be consistent:
For the first one
Second one can stay the same
Last one
Nice work Xjm and fago!
9 days to next Drupal core point release.
Comment #127
fago@aspilicious: Thanks for clarifying - so that should work now in #125? It has those changes included.
Comment #128
aspilicious CreditAttribution: aspilicious commentedWell...
Is on three lines but I can live with that ;)
I'll try to go over the lines again to see if I can find something but I don't think I will.
Comment #129
aspilicious CreditAttribution: aspilicious commentedI changed those lines and a comment. I hope this is rdy to go now :)
Comment #130
xjm@aspilicious doesn't want us to care about
hook_comment_publish()
. Poor hook!I'll review this again later today. Yay!
Edit: Also, for the question of deletion order, I have a patch in #1346032: Ordering of hook_entity_delete() is inconsistent. I'd love it if some of the folks who've addressed that question above could review that patch. If it goes in after this one, it will need a few changes, but it also could go in before, depending on what folks think is best.
Comment #131
fagoI'd argue that a comment that just says what I can read anyway is useless. Should we better remove it?
However please, let's don't turn this issue into "fix comment.module" but concentrate on the entity api related stuff.
I've added in the
$additions['comment_form']
hunk from above. Updated patch attached.Comment #132
fagogrml
Comment #133
xjmI read the patch hopefully for the last time. :) Here's a few things I will fix in a quick reroll:
This comment should be on the preceding line.
I agree; let's just kill this comment.
Another ids vs. IDs.
"Look up" should be two words.
Comment #134
xjmNow I'm going to hassle some folks and ask them to review. :)
Comment #135
xjmAhh whitespace!
Fixed:
Comment #137
tstoeckler#135: drupal_8_entity_crud-1184944-135.patch queued for re-testing.
Comment #138
fagoChanges of #135 look good to me - added those to the Git repo + re-rolled the patch based upon the repo. I also looked through all the changes another time and have not found anything left/wrong.
Thus, it should be ready now!
Comment #139
pounardI'm all in for this kind of change, but why don't you call you
EntityInterface
accessors as accessors? Seeing a function just namedEntityInterface::id()
makes gnash my teeth. Methods should at least carry a verb in them, I don't want to identify my node, I want to get its id, I'd naturally look for aEntityInterface::getSomething()
method instead (hereEntityInterface::getId()
).That was my small contribution to this patch.
EDIT: Short name look like simplicity, but definitely just look like: the more the API will grow the more keeping short names will be confusing. And not naming accessors get/set something makes them hard to find when using autocompletion features of IDE's (for example) or even when searching through code with a good'old grep. Don't misplace simplicity and shortcut, this may look like it but it's not: IMHO explicit things are better than short ones.
Comment #140
tstoeckler#139--
I'm all for explicitness and against shortcuts, but that isn't the case here.
id() is just the abstraction of the $id property that all entities have anyway. We just can't enforce $id in the Interface, because that would not allow remote entities or non-Drupal entities in general who just might not have that $id property.
But the 80% use-case is $id, so you think $entity->id, while writing $entity->id(). It's not a shortcut.
Comment #141
Crell CreditAttribution: Crell commentedI also have to disagree with #139. Not every method needs get* in front of it to indicate that it's a read method. Sometimes that helps, but not always. jQuery, for instance, rarely has a get* prefix and it works fine.
Comment #142
pounardIt's only a matter of standard, what happens is that if we start using get*() method names over the WSCCI initiative (and we do) this should be normalized accross the framework. I will follow the majority, but my opinion will remain that naming a method just id() is obfuscated and confusing.
Comment #143
fagoI think it's fine to go without the prepended "get" too, in particular for often-used stuff like $entity->id(). This surely is a matter of taste, so I'm happy to follow the majority here too.
Comment #144
tstoecklerWell, at the very least let's not derail this issue on that.
Using getFoo(), while a standard elsewhere, is not a Drupal standard. If we want to establish it as such, that deserves its own issue.
Comment #145
fagoAgreed - we should move on with this issue asap given quite some stuff depends on it. Any open points? Else, rtbc anyone? :)
Comment #146
bojanz CreditAttribution: bojanz commented+1 for RTBC. We've dragged this on long enough.
Comment #147
RobLoach#138: drupal_8_entity_crud.patch queued for re-testing.
Comment #148
xjmI also think this is RTBC.
Comment #149
aspilicious CreditAttribution: aspilicious commentedCome on...
Everybody is scared to rtbc this...
Let us do this, it's not that we destroy production sites and this got like a gazillion reviews!
Comment #150
tstoecklerI just read through the entire patch for I think the 3rd time and I couldn't find anything wrong with it. Not even nitpicks. Let's do this!
Comment #151
catchI'm happy with this as well, will likely commit this some time over the weekend after another look through.
While we're waiting ;) it'd be great to open the follow-up issues for this and document them in the issue summary, I can think of the following but might have missed some too:
- converting all the different core entities apart from Comment.
- adding revision support to the base controller (which will need co-ordination with #1082292: Clean up/refactor revisions)
- I think the next architectural stuff is handled by #1346204: [meta] Drupal 8 Entity API improvements so maybe that's plenty?
Comment #152
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedCan be written on one line.
exists -> exist
Can be written on one line.
unserializiation -> unserialization
Missing 'of'.
New line before EOF.
New line before EOF.
Other than that, this looks great to me. Excited to see this committed :)
Comment #153
xjmRe: #151 -- I think the followup issues are all getting tracked in the summary of #1346204: [meta] Drupal 8 Entity API improvements. I'll add a link to that to the summary here for posterity.
The changes in #152 looks good.
Comment #153.0
xjmUpdated issue summary.
Comment #153.1
xjmUpdated issue summary.
Comment #154
geerlingguy CreditAttribution: geerlingguy commented@152 - are you sure about the exists -> exist? I wouldn't want to have to do a one-line follow up in a year if it needs to be reverted :)D'oh. I speak English. Apparently, I'm not very good proofreading it.
Comment #155
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedMany comments (plural) exist, one comment (singular) exists.
I'm not a native English speaker so I could be wrong though :)
Comment #156
redndahead CreditAttribution: redndahead commented#155 is correct.
Comment #157
theunraveler CreditAttribution: theunraveler commentedI beg to differ re: comment #83:
In ActiveRecord, the convention is to use
#build
or#new
to construct a new record without saving.Object#create
, to me, means that the record should be both instantiated and saved to the DB.Comment #158
Dries CreditAttribution: Dries commentedWhile reviewing the patch, I felt like several methods could be updated to start with "get". So I agree with @pounard in #139 that it would be nicer to have more explicit getXXX() functions instead of XXX() functions. It is common practice and would improve readability. I don't think that should hold up this patch though.
I reviewed this patch in depth and really like it. It makes things easier to follow and it is nice, clean and straight-forward OOP. Definitely improves the developer experience for me. Nice work.
I'll leave it up the @catch to commit this patch as he indicated he was planning to.
Comment #159
Cyberwolf CreditAttribution: Cyberwolf commentedMaybe not the right place to ask this, but is there more background info publicly available somewhere why exactly has been chosen to implement something looking like the ActiveRecord pattern? Drupal 7 and its entity API seem to be more "Data Mapper"-oriented to me.
Comment #160
Crell CreditAttribution: Crell commentedCyberwolf: http://www.garfieldtech.com/blog/orm-vs-query-builders and the articles linked therein.
Comment #161
fagoI've added follow-up issues and linked them in the meta issue #1346204: [meta] Drupal 8 Entity API improvements
Comment #162
catchAlright then. Committed and pushed to 8.x. This will need a change notice (and probably a CHANGELOG.txt entry too - let's do the CHANGELOG.txt here but leave anything else to new issues).
Comment #163
Dries CreditAttribution: Dries commentedCyberwolf: do you have more specific thoughts (after reading the things referenced by Larry)?
Comment #164
Gábor HojtsyCross-post I guess.
Comment #165
Cyberwolf CreditAttribution: Cyberwolf commented@Crell: thanks for the link.
@Dries: it's probably something very subjective and personal, but I strongly dislike the Active Record pattern as it doesn't clearly separate the concerns of your domain model (nodes, users, comments, ...) and how/where the data in your model is persisted (stored, updated and retrieved). Letting objects store() themselves just doesn't feel right to me from a OOP point of view and will never do.
Comment #166
pounardI agree with Cyberwolf about this description, but I guess is as long as the controller still exists, we still have separation of concern. I'm not fond of this active record pattern variant, but as long as we have a separate object responsible for doing the real CRUD the data I'm happy with it, because it means it stays injectable.
There are still stuff bothering me, like the fact that every entity instance carries its own entity info: it's a lot of array copies. I know that until we don't modify the arrays PHP will uses references thanks to the copy on write mecanism, but that's still too much arbitrary descriptive metadata hanging around. Because we still have the controllers behind. Controllers should carry this data, not the entity themselves.
Plus the fact that CRUD functions are only a proxy to the controller behind, there's something odd is that the controller is not being injected to the instances, but fetched using procedural accessors by the object itself: the full API is OO but you depend on procedural accessors (keeping only those as procedural pieces of this full API definitely is weird).
Instead of using something like
return $this->{$this->entityInfo['id key']}
(which is being done, but using two steps instead of one) I'd go for a more hardcoded schema: every entity of core could adopt a raw "id" or "entityId" property (while keeping the accessor distinct of the property) so that we wouldn't need to use the entityInfo array at all, making it useless to carry inside the objects thereof. Any specific implementation could use the key they want, just by overriding the default implementation andreturn $this->whatever;
instead. EDIT: and you could do it, because core entities already have their own class, we're not counting overrides I guessIt would avoid to let huge arrays hanging everywhere in memory (even if copy on write make them being only references). Objects would be easier to dump and debug, code would be lighter and more comprehensible, and we'd definitely separate concerns (we don't as long as we keep database references, such as the id key and bundle key names inside the object itself).
I'm all for separation of concern, and right now this API has some flaws:
thus avoiding potential notices if the id key is not set in the object, for exampleEDIT: that won't happen because you already declared the properties in specializations)All of this is easily fixable without modifying the public API.
Comment #167
pounardBTW: You could replace
function_exists<()/code> using <code>is_callable()
instead: that would naturally allow developers to use anonymous functions instead (without changing anything else in the code).EDIT: It sounds weird to define anonymous functions in entity info (not really doable if entity info is being cached) but I guess it's still doable doing some alterations at some point.
Comment #168
Crell CreditAttribution: Crell commentedI think pounard's comments are valid, but I at least understood this issue as "first cut to get over the hump", not a final implementation. Further cleanup along the lines in #166 (especially injecting the controller object rather than re-fetching it) should be straightforward enough to do in new issues. Let's get this one closed as soon as the update notification is made.
Also, anon functions are not viable anywhere that we serialize data to the DB or disk, which right now is nearly all info hooks, for better or worse. So I don't think that is relevant for the time being.
Comment #169
pounardYes, it sounds fair to get this issue finalized before starting messing with details.
Comment #170
fagoAs pounard pointed out, we have separated it to our storage controllers.
Also, I'd not say it's an Active Record pattern as we are not tied to relational databases at all. I'd see the entity API as overall data API covering data integration and persistence, whereby persistence is handled by the storage controller and the entity objects are Drupal's representation of the data. So modules can rely upon this representation without having to know where the data comes from. That way, we can easily do entity controllers that expose any accessible data, e.g. data from RESTful web services or mongo db.
Ad #166:
I think you make some very valid points. Could you create an issue for discussing and fixing that?
Comment #171
sunComment #172
xjmWriting this change notification, but maybe not tonight. :)
Comment #173
xjmFirst draft of the change notice at : http://drupal.org/node/1400186
Please pretty please review it for accuracy, clarity, missing content, etc. We may want to add a specific example of how to convert a simple custom entity to the new system, as well.
Comment #174
xjmNot sure what happened to the tag.
Comment #175
xjmberdir helped review the change notification in IRC. Suggested changes:
Comment #176
Gábor HojtsyTagging for the content handling leg of D8MI.
Comment #177
klausiChange notification looks already pretty good.
* Added a recommendation to use entity_create() and not PHP new.
* Added the example implementation code as suggested above. Also added some entity handling snippet.
I did not add the protected methods for the classes, IMO that would only clutter the change notification (are protected methods API relevant? Are they that important here?).
Comment #178
xjmThanks @klausi! Let's call this fixed. :)
Comment #179
hinikato CreditAttribution: hinikato commentedI suggest to use just the entity() function instead of using entity_create() or entity_construct().
Comment #180
klausi@hinikato: please open a new issue for that.
Comment #182
fagoAdded a follow-up to add type-hinting to comments, as suggested in #32:
#1480866: Add type-hinting and parameter type docmentation for comment objects
Comment #183
David_Rothstein CreditAttribution: David_Rothstein commentedDoesn't look like the CHANGELOG.txt entry ever happened. Perhaps could be moved to a separate issue at this point, if necessary.
Comment #184
BerdirWhat about this for a start?
Looking at the changelog, it seems rather sparse in general, with the notable exception of the D8MI initiative (Gabor++). I thought about adding a line for the get()/set() methods as well, but core is not using them anywhere yet, they're also not that interesting without being backed by translatable properties. And are going to change for the generic Property API, so I left it out. Also not relevant for this issue.
Comment #185
Gábor HojtsyLooks good for me.
Comment #186
webchickCommitted and pushed to 8.x. Thanks!
Comment #188
plachComment #188.0
plachUpdated issue summary.