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.

Files: 
CommentFileSizeAuthor
#184 entity-changelog-1184944-183.patch553 bytesBerdir
PASSED: [[SimpleTest]]: [MySQL] 36,978 pass(es). View
#152 drupal-8-entity-crud-5315182-152.patch53.31 KBTor Arne Thune
PASSED: [[SimpleTest]]: [MySQL] 34,248 pass(es). View
#152 interdiff-138-152.txt2.9 KBTor Arne Thune
#138 drupal_8_entity_crud.patch53.33 KBfago
PASSED: [[SimpleTest]]: [MySQL] 34,128 pass(es). View
#135 drupal_8_entity_crud-1184944-135.patch53.33 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,013 pass(es). View
#134 drupal_8_entity_crud-1184944-134.patch53.33 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,009 pass(es). View
#134 interdiff-131-134.txt2.25 KBxjm
#131 drupal_8_entity_crud.patch53.32 KBfago
PASSED: [[SimpleTest]]: [MySQL] 34,021 pass(es). View
#129 1184944-entity-crud-129.patch52.04 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 34,023 pass(es), 1 fail(s), and 0 exception(es). View
#129 entity-crud-diff.txt1.52 KBaspilicious
#125 drupal_8_entity_crud.patch53.28 KBfago
PASSED: [[SimpleTest]]: [MySQL] 34,023 pass(es). View
#124 drupal_8_entity_crud.patch53.22 KBfago
PASSED: [[SimpleTest]]: [MySQL] 34,009 pass(es). View
#124 drupal_8_entity_crud_interdiff.txt4.29 KBfago
#123 drupal_8_entity_crud-123.patch53.29 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,002 pass(es). View
#123 interdiff-118-123.txt3.76 KBxjm
#118 drupal_8_entity_crud-118.patch53.14 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,000 pass(es). View
#118 interdiff-116-118.txt6.6 KBxjm
#116 drupal_8_entity_crud.patch53.22 KBfago
PASSED: [[SimpleTest]]: [MySQL] 33,989 pass(es). View
#113 drupal_8_entity_crud.patch54.8 KBfago
PASSED: [[SimpleTest]]: [MySQL] 33,995 pass(es). View
#113 drupal_entity_interdiff.txt1.33 KBfago
#98 entity-crud-1184944-98.patch55.11 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,983 pass(es). View
#98 interdiff-92-98.txt17.61 KBxjm
#92 drupal_8_entity_crud-1184944-92.patch52.13 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,812 pass(es). View
#92 interdiff-90-92.txt3.19 KBxjm
#90 drupal_8_entity_crud.patch52.13 KBfago
PASSED: [[SimpleTest]]: [MySQL] 33,770 pass(es). View
#85 drupal_8_entity_crud.patch52.04 KBfago
FAILED: [[SimpleTest]]: [MySQL] 30,180 pass(es), 908 fail(s), and 480 exception(es). View
#84 drupal_8_entity_crud.patch51.86 KBfago
PASSED: [[SimpleTest]]: [MySQL] 33,660 pass(es). View
#82 1184944-entity-crud-82.patch51.86 KBbojanz
PASSED: [[SimpleTest]]: [MySQL] 33,660 pass(es). View
#82 1184944-entity-crud-82-interdiff.txt8.61 KBbojanz
#78 drupal_8_entity_crud_issue.patch52.16 KBfago
PASSED: [[SimpleTest]]: [MySQL] 33,410 pass(es). View
#76 1184944-entity-classes.patch61.1 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 33,400 pass(es). View
#76 1184944-interdiff.txt8.97 KBklausi
#75 1184944-entity-classes.patch59.89 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 33,400 pass(es). View
#73 1184944-entity-classes.patch60.04 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 33,292 pass(es), 106 fail(s), and 52 exception(es). View
#64 drupal_8_entity_crud_issue.patch52.95 KBfago
PASSED: [[SimpleTest]]: [MySQL] 33,357 pass(es). View
#57 drupal_8_entity_crud.patch52.84 KBfago
PASSED: [[SimpleTest]]: [MySQL] 33,087 pass(es). View
#35 drupal_8_entity_crud_issue-D8.patch42.71 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue-D8.patch. Unable to apply patch. See the log in the details link for more information. View
#35 drupal_8_entity_crud_full.patch373.01 KBfago
PASSED: [[SimpleTest]]: [MySQL] 32,972 pass(es). View
#33 drupal_8_entity_crud_full.patch373.01 KBfago
FAILED: [[SimpleTest]]: [MySQL] 32,961 pass(es), 1 fail(s), and 0 exception(es). View
#33 drupal_8_entity_crud_issue-D8.patch42.71 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue_4.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#29 drupal_8_entity_crud_full.patch372.58 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_full_4.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#29 drupal_8_entity_crud_issue.patch43.19 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue_3.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#13 drupal_8_entity_crud_full.patch366.85 KBfago
PASSED: [[SimpleTest]]: [MySQL] 33,574 pass(es). View
#13 drupal_8_entity_crud_issue8.patch56.54 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue8.patch. View
#10 drupal_8_entity_crud_full.patch366.83 KBfago
FAILED: [[SimpleTest]]: [MySQL] 33,498 pass(es), 1 fail(s), and 0 exception(es). View
#10 drupal_8_entity_crud_issue.patch55.77 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue_2.patch. View
#8 drupal_8_entity_crud_full.patch366.21 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_full_1.patch. View
#8 drupal_8_entity_crud_issue.patch55.15 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue_1.patch. View
#7 drupal_8_entity_crud_issue.patch55.08 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue_0.patch. View
#7 drupal_8_entity_crud_full.patch366.13 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_full_0.patch. View
#2 doc errors.txt6.86 KBaspilicious
drupal_8_entity_crud_issue.patch54.52 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue.patch. View
drupal_8_entity_crud_full.patch366.42 KBfago
FAILED: [[SimpleTest]]: [MySQL] 30,076 pass(es), 1 fail(s), and 5 exception(es). View

Comments

Status: Needs review » Needs work

The last submitted patch, drupal_8_entity_crud_issue.patch, failed testing.

aspilicious’s picture

FileSize
6.86 KB

Ok, 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.

catch’s picture

Subscribing.

mikeryan’s picture

Subscribe.

casey’s picture

I 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.

xjm’s picture

Tracking.

Subscribers, see also: http://groups.drupal.org/node/133579

fago’s picture

Status: Needs work » Needs review
FileSize
366.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_full_0.patch. View
55.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue_0.patch. View

ad #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 beImplements Interface::method(). though, so it can be turned into a link by the API module?

fago’s picture

FileSize
55.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue_1.patch. View
366.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_full_1.patch. View

ok, 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?

Status: Needs review » Needs work

The last submitted patch, drupal_8_entity_crud_issue.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
55.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue_2.patch. View
366.83 KB
FAILED: [[SimpleTest]]: [MySQL] 33,498 pass(es), 1 fail(s), and 0 exception(es). View

updated Git repo to the latest 8.x branch + created new patches

Status: Needs review » Needs work

The last submitted patch, drupal_8_entity_crud_issue.patch, failed testing.

xjm’s picture

Looks 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. :)

fago’s picture

Status: Needs work » Needs review
FileSize
56.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue8.patch. View
366.85 KB
PASSED: [[SimpleTest]]: [MySQL] 33,574 pass(es). View

yep, there was another stdclass comment creation.. fixed that.

amitaibu’s picture

subscribe

Status: Needs review » Needs work

The last submitted patch, drupal_8_entity_crud_issue8.patch, failed testing.

tstoeckler’s picture

I 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

catch’s picture

Those 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.

dixon_’s picture

sub.

catch’s picture

To be a bit clearer on controllers, I've been thinking something like this, although not sure where the separation should lie exactly:


class EntityController implements EntityControllerInterface {
  function __construct() {
      // Set up entity type and info etc.
   }
   // probably some shared properties if they make sense for all the controllers.
}

class EntityStorage extends EntityController implements EntityStorageInterface {
  // implements create/load/save/delete
}

class EntityRender extends EntityController implements EntityRenderInterface {
 // implements view() and related.
}

class EntityForm extends EntityController implements EntityFormInterface {
  // entity forms + submit.
}

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.

fago’s picture

- it's a shame comment_save() is so far off the parent controller we have to completely override it.

Yes, there is quite some custom stuff in there. It's not completely overridden though - it calls the parent implementation.

- 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.

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:

* 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.

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?

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.

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 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.

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!

catch’s picture

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.

Right 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.

Thus consequently, we should add a "static public function invokeHookLoad() {}" method too. Thoughts?

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?

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'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 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?

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.

What about schema-upgrades or adding/deleting indexes?

Yeah we will need something similar to hook_field_schema() if we do this, and that is not without its problems.

fago’s picture

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

Agreed.

>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.

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 :)

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.

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.

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.

fago’s picture

So I'd move buildContent just into the entity class, and keep view() implemented out-side as it can be implemented generically.

ok, 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.

catch’s picture

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.

From 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?

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.

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).

rlmumford’s picture

sub

yched’s picture

Staying out of the Entity class / Controller class architectural debate for now ;-), a couple down-to-earth remarks after a visual review of #13 :

+++ b/modules/comment/comment.module
@@ -1691,6 +1528,128 @@ class CommentController extends DrupalDefaultEntityController {
+  public function __construct(array $values = array(), $entityType = NULL) {
+    return parent::__construct($values, 'comment');

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 :

Comment::__construct($values = array());
Entity::__construct($values = array(), $entityType);
+++ b/modules/entity/entity.class.inc
@@ -0,0 +1,213 @@
+  protected function setUp() {
+    $this->entityInfo = entity_get_info($this->entityType);

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() ?

+++ b/modules/entity/entity.class.inc
@@ -0,0 +1,213 @@
+  public function view($view_mode = 'full', $langcode = NULL) {

'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)

+++ b/modules/entity/entity.class.inc
@@ -0,0 +1,213 @@
+  public function invokeHookPreSave() {
+    if (!empty($this->entityInfo['fieldable'])) {
+      field_attach_presave($this->entityType, $this);
+    }
+    module_invoke_all($this->entityType . '_presave', $this);
+    module_invoke_all('entity_presave', $this, $this->entityType);
+  }

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) ?

+++ b/modules/entity/entity.controller.inc
@@ -388,3 +388,339 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+    // If this is the bundle of another entity, set the bundle key.
+    if (isset($this->entityInfo['bundle of'])) {
+      $info = entity_get_info($this->entityInfo['bundle of']);
+      $this->bundleKey = $info['bundle keys']['bundle'];
+    }

'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 ?

+++ b/modules/entity/entity.controller.inc
@@ -388,3 +388,339 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+    // For Field API and entity_prepare_view, the entities have to be keyed by
+    // (numeric) id.
+    $entities = entity_key_array_by_property($entities, $this->idKey);

Shouldn't that be a prerequisite on the incoming $entities array ? That's extra CPU if the incoming array is correctly keyed.

+++ b/modules/entity/entity.module
@@ -232,6 +233,155 @@ function entity_load_unchanged($entity_type, $id) {
+  return (object) $values;

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 ?)

+++ b/modules/entity/entity.module
@@ -424,3 +574,120 @@ function entity_form_submit_build_entity($entity_type, $entity, $form, &$form_st
+function entity_var_json_export($var, $prefix = '') {
...
+function template_preprocess_entity(&$variables) {

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.

plach’s picture

heyrocker’s picture

subscribing

fago’s picture

FileSize
43.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue_3.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
372.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_full_4.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Update:
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.

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.

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.

'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 ?

That was a left-over, thanks removed. I also removed some other unrelated entity-api.module improvements, i.e. load().

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) ?

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?

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 ?)

Nope, everything has to be an instance of EntityInterface now. For now this applies only to the converted entity-types though. Fixed that.

>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

Agreed.

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()?

fago’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal_8_entity_crud_issue.patch, failed testing.

klausi’s picture

+++ b/modules/entity/entity.controller.inc
@@ -388,3 +390,122 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+   *
+   * @param $entity
+   *   The entity to save.
+   *
+   * @return
+   *   SAVED_NEW or SAVED_UPDATED is returned depending on the operation
+   *   performed.
+   */
+  public function save($entity);

shouldn't we type hint the $entity parameter here? we don't want that arrays or stdClass objects are passed in here, so maybe:

public function save(EntityInterface $entity);

is better?

fago’s picture

Status: Needs work » Needs review
FileSize
42.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue_4.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
373.01 KB
FAILED: [[SimpleTest]]: [MySQL] 32,961 pass(es), 1 fail(s), and 0 exception(es). View

#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.

Status: Needs review » Needs work

The last submitted patch, drupal_8_entity_crud_issue.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
373.01 KB
PASSED: [[SimpleTest]]: [MySQL] 32,972 pass(es). View
42.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_8_entity_crud_issue-D8.patch. Unable to apply patch. See the log in the details link for more information. View

I 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.

fago’s picture

As 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.

catch’s picture

OK did not review every line of the patch but here's a few thoughts reading through. Mainly nitpicks, not up to much else today.

+++ b/modules/comment/comment.moduleundefined
@@ -99,6 +99,7 @@ function comment_entity_info() {
+      'entity class' => 'Comment',
       'entity keys' => array(
         'id' => 'cid',

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.

+++ b/modules/comment/comment.moduleundefined
@@ -1662,10 +1499,10 @@ function comment_load($cid) {
+class CommentController extends EntityDefaultStorageController {

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.

+++ b/modules/comment/comment.moduleundefined
@@ -1691,6 +1528,117 @@ class CommentController extends DrupalDefaultEntityController {
+  public $cid, $pid;
+  public $uid = 0;
+  public $language = LANGUAGE_NONE;

These should all be on their own lines with their own docblock.

+++ b/modules/comment/comment.moduleundefined
@@ -1691,6 +1528,117 @@ class CommentController extends DrupalDefaultEntityController {
+  public function invokeHookPreSave() {

Maybe just preSave()? This does a lot more than invoke the hook. Similarly with the other methods.

+++ b/modules/comment/comment.moduleundefined
@@ -1691,6 +1528,117 @@ class CommentController extends DrupalDefaultEntityController {
+    // Make sure we have a proper bundle name.
+    if (!isset($this->node_type) || $this->node_type == 'comment') {
+      $node = node_load($this->nid);
+      $this->node_type = 'comment_node_' . $node->type;

How can we get here without a proper bundle name? Or why would it vary?

+++ b/modules/comment/comment.moduleundefined
@@ -1691,6 +1528,117 @@ class CommentController extends DrupalDefaultEntityController {
+        // This is a comment with no parent comment (depth 0): we start
+        // by retrieving the maximum thread level.

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.

+++ b/modules/comment/comment.moduleundefined
@@ -1691,6 +1528,117 @@ class CommentController extends DrupalDefaultEntityController {
+  public function invokeHookInsert() {
+    // Update the {node_comment_statistics} table prior to executing the hook.

Change _comment_update_node_statistics() to a method?

+++ b/modules/entity/entity.class.incundefined
@@ -0,0 +1,342 @@
+  /**
+   * Returns the UUID of the entity.
+   *
+   * @return
+   *   The UUID of the entity, or NULL if the entity type does not provide
+   *   UUIDs.
+   */

I think we should leave uuid for a followup - no core entities have uuid support yet.

+++ b/modules/entity/entity.class.incundefined
@@ -0,0 +1,342 @@
+  protected function setUp() {
+    $this->entityInfo = entity_get_info($this->entityType);
+    $this->idKey = $this->entityInfo['entity keys']['id'];
+    $this->bundleKey = isset($this->entityInfo['entity keys']['bundle']) ? $this->entityInfo['entity keys']['bundle'] : NULL;

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?

+++ b/modules/entity/entity.class.incundefined
@@ -0,0 +1,342 @@
+  public function isNew() {
+    return empty($this->{$this->idKey});

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.

+++ b/modules/entity/entity.class.incundefined
@@ -0,0 +1,342 @@
+  public function label() {
+    return entity_label($this->entityType, $this);

Why not move that logic into the class? Or not have the method on the class at all? Feels like duplication at the moment.

+++ b/modules/entity/entity.moduleundefined
@@ -256,6 +257,38 @@ function entity_load_unchanged($entity_type, $id) {
+function entity_delete_multiple($entity_type, $ids) {
+  entity_get_controller($entity_type)->delete($ids);

Could we switch to entity($entity_type)->delete($ids); for the public API? Would save all the wrappers.

+++ b/modules/entity/entity.moduleundefined
@@ -256,6 +257,38 @@ function entity_load_unchanged($entity_type, $id) {
+/**
+ * Creates a new entity object.
+ *
+ * @param $entity_type
+ *   The type of the entity.
+ * @param $values
+ *   An array of values to set, keyed by property name. If the entity type has
+ *   bundles the bundle key has to be specified.
+ *
+ * @return Entity
+ *   A new entity object.
+ */

The naming here confuses me a bit, maybe because we have field_create_field() which actually saves the field?

plach’s picture

Issue tags: +D8MI

tagging

bojanz’s picture

Subscribing.

plach’s picture

Status: Needs work » Needs review

While 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:

<?php
$body = $node->getBody();
// $body holds '<p>this is a test node!</p>'
?>

instead of

<?php
$body = $node->get("body");
// $body holds array(array('value' => '<p>this is a test node!</p>'));
?>

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:

  • performance: a _call invocation would add an overhead for each property access, which should be added to the one already introduced by the actual get()/set() pair. A __call method implementation might look like this:
    <?php
    function __call($name, $arguments) {
      $property_name = strtolower(substr($name, 3, 1)) . substr($name, 4);
      $method = $name{0} == 'g' ? 'get' : 'set';
      return call_user_function_array(array($this, $method), $arguments);
    }
    ?>
    
  • ancestry: we should find a way to make work a 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.

chx’s picture

Status: Needs review » Needs work
+++ b/modules/comment/comment.moduleundefined
@@ -735,7 +736,7 @@ function comment_node_page_additions($node) {
+    $build = drupal_get_form("comment_node_{$node->type}_form", entity_create('comment', array('nid' => $node->nid)));

I 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?

+++ b/modules/comment/comment.moduleundefined
@@ -1691,6 +1528,117 @@ class CommentController extends DrupalDefaultEntityController {
+  public function invokeHookPreSave() {

Why is a function called invoke* do all sorts of stuff? That's for implementing a hook and not invoking it.

+++ b/modules/entity/entity.class.incundefined
@@ -0,0 +1,342 @@
+  public function set($property_name, $value, $language = NULL);

Insert rant against settter/getters. I realize it's necessary due to multilanguage. Allow me to hate it nonetheless and then move on.

+++ b/modules/entity/entity.class.incundefined
@@ -0,0 +1,342 @@
+  public function save();

See above -- save() and CRUD kind of does not mesh. What about entity_construct instead of create, hm?

+++ b/modules/entity/entity.class.incundefined
@@ -0,0 +1,342 @@
+   * Set up the object instance on construction or unserializiation.
...
+  protected function setUp() {

*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.

+++ b/modules/entity/entity.class.incundefined
@@ -0,0 +1,342 @@
+  public function invokeHookPreSave() {

These need to be final IMO. Quite in league what I said above about invoke* implementing.

+++ b/modules/entity/entity.controller.incundefined
@@ -194,11 +194,13 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+        $query_result->setFetchMode(PDO::FETCH_CLASS, $this->entityInfo['entity class'], array(array(), $this->entityType));

You seriously write down array(array(), $this->entityType) without a nick of a comment :) ? What's that first empty array, huh?

+++ b/modules/entity/entity.controller.incundefined
@@ -388,3 +390,122 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+class EntityDefaultStorageController extends DrupalDefaultEntityController implements EntityAPIStorageControllerInterface {

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.

Crell’s picture

Status: Needs review » Needs work

I 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.

plach’s picture

@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).

catch’s picture

I 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.

plach’s picture

For language, could we do something like $entity->setPreferredLanguage($langcode);

Yes, 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.

catch’s picture

Title: add CRUD support to the entity API » Make entities classed objects, introduce CRUD support

While 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.

fago’s picture

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.

I 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()

Why not move that logic into the class? Or not have the method on the class at all? Feels like duplication at the moment.

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.

+++ b/modules/comment/comment.moduleundefined
@@ -1691,6 +1528,117 @@ class CommentController extends DrupalDefaultEntityController {
+ public $cid, $pid;
+ public $uid = 0;
+ public $language = LANGUAGE_NONE;

These should all be on their own lines with their own docblock.

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()

Maybe just preSave()? This does a lot more than invoke the hook. Similarly with the other methods.

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.

Why is a function called invoke* do all sorts of stuff? That's for implementing a hook and not invoking it.

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.

Could we switch to entity($entity_type)->delete($ids); for the public API? Would save all the wrappers.

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()

The naming here confuses me a bit, maybe because we have field_create_field() which actually saves the field?

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).

*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.

oh, it's called by __construct()?

EntityAPIwhateverinterface? I know most of this code comes from Entity API project but we got rid of API even in hook_nodeapi.

Right.

I 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

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).

Ok, but besides that, what is being planned here is trying to unfiy field and property handling.

Yep, but let's postpone that for now, so we can concentrate on the more foundational issues now.

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).

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... ?

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.

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.

bojanz’s picture

I 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.

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?

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.

chx’s picture

As 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?

Crell’s picture

Why 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.

catch’s picture

>I agree, though we have to keep it as long as APIs (old+new) co-exist. We should remove it from the Entity class though.

That works for me.

@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?

Sounds fine but either a @todo or @deprecated on entity_label() or something would be good to track this.

it would be more consistent to have no defined class properties at all.

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.

Then, for setting default values we would have to come up with something new like defining those in that hook_entity_property_info()...

Hmm what kind of default values? I'd think the schema definition is enough?

@invokeHookPreSave()

* 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.)

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.

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.

Sounds great!

Thus, I'd vote for entity_construct() (as chx came up with).

That's a good change yeah.

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).

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... ?

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.

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.

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.

beejeebus’s picture

subscribe.

moshe weitzman’s picture

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).

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... ?

FYI, 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.

mikeryan’s picture

+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.

chx’s picture

Why 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.

pillarsdotnet’s picture

fago’s picture

Status: Needs work » Needs review
FileSize
52.84 KB
PASSED: [[SimpleTest]]: [MySQL] 33,087 pass(es). View

ok, 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.

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.

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.

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.

Makes sense, changed that.

These should all be on their own lines with their own docblock.

Fixed that.

How can we get here without a proper bundle name? Or why would it vary?

It wouldn't vary and it looks like it doesn't happen. So I've removed that check.

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.

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.

Change _comment_update_node_statistics() to a method?

Makes sense to me, I've moved it to the storage controller.

I think we should leave uuid for a followup - no core entities have uuid support yet.

I've done so.

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?

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.

Why not move that logic into the class? Or not have the method on the class at all? Feels like duplication at the moment.

Copied the code over for now and added an @todo to remove entity_label() once everything is converted.

You seriously write down array(array(), $this->entityType) without a nick of a comment :) ? What's that first empty array, huh?

Added a comment.

EntityAPIwhateverinterface? I know most of this code comes from Entity API project but we got rid of API even in hook_nodeapi.

Right, done so.

The naming here confuses me a bit, maybe because we have field_create_field() which actually saves the field?

ok, renamed entity_create() to entity_construct().

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.

ok, I've added support for the is_new flag in $entity->isNew(). That should do it.

Why 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.

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().

bojanz’s picture

I 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:

+    // Setup standard comment properties.
+    foreach ($comments as $key => $comment) {
+      $comment->name = $comment->uid ? $comment->registered_name : $comment->name;
+      $comment->new = node_mark($comment->nid, $comment->changed);
+      $comment->node_type = 'comment_node_' . $comment->node_type;
+      $comments[$key] = $comment;
+    }

(Why not in the entity class constructor?)

or

+    if (!isset($comment->status)) {
+      $comment->status = user_access('skip comment approval') ? COMMENT_PUBLISHED : COMMENT_NOT_PUBLISHED;
+    }

and

+      if (empty($comment->created)) {
+        $comment->created = REQUEST_TIME;
+      }
+      if (empty($comment->changed)) {
+        $comment->changed = $comment->created;
+      }
+      if ($comment->uid === $user->uid && isset($user->name)) { // '===' Need to modify anonymous users as well.
+        $comment->name = $user->name;
+      }
+      // Add the values which aren't passed into the function.
+      $comment->thread = $thread;
+      $comment->hostname = ip_address();
+    }

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 :)

fago’s picture

I like what I see. So I'm gonna skip the nitpicking and go straight to the tricky stuff: class VS controller ( + storage concerns).

Good choice ;)

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 :)

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:

  • If - as you suggest - we have custom storage related methods in the controller like updateNodeStatistics() the storage controller is already customized, thus it is not pluggable in a general fashion any more.
  • How would we deal with updates to the schema? Or to indexes?

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.

+ // Setup standard comment properties.
+ foreach ($comments as $key => $comment) {
+ $comment->name = $comment->uid ? $comment->registered_name : $comment->name;
+ $comment->new = node_mark($comment->nid, $comment->changed);
+ $comment->node_type = 'comment_node_' . $comment->node_type;
+ $comments[$key] = $comment;
+ }

(Why not in the entity class constructor?)

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).

...Which could easily be in the entity class save() method (overriding the method, and calling the parent, like Entity API does in contrib currently).

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 why we don't require at least "created" on all entity types?

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.

klausi’s picture

Status: Needs review » Needs work
+++ b/modules/comment/comment.entity.inc
@@ -0,0 +1,257 @@
+  protected function postSave($comment) {

type hinting with EntityInterface $comment would be nice here

+++ b/modules/entity/entity.controller.inc
@@ -388,3 +393,184 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+  protected function preSave($entity) { }

type hinting again, and several other places where $entity is used.

+++ b/modules/entity/entity.module
@@ -256,6 +257,38 @@ function entity_load_unchanged($entity_type, $id) {
+  if ($class = $info['entity class']) {
+    return new $class($values, $entity_type);
+  }
+  throw new Exception("Entity class is missing");

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.

bojanz’s picture


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.

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.

mikeryan’s picture

any reason why we don't require at least "created" on all entity types?

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.

Actually, 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).

fago’s picture

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).

I 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.

Actually, I'd like to require "changed" on all entity types, which would make it simple to incrementally pull content as it changes: #1052692: Use migrate module for major version updates. 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).

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.

fago’s picture

Status: Needs work » Needs review
FileSize
52.95 KB
PASSED: [[SimpleTest]]: [MySQL] 33,357 pass(es). View

type hinting again, and several other places where $entity is used.

fixed that.

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.

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.

tstoeckler’s picture

About requiring "created", "changed", etc. please see #1295148: Maintain common properties/statistics about entities.

Crell’s picture

Status: Needs review » Needs work

"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().

Crell’s picture

Status: Needs work » Needs review

Oops, crossposted.

bojanz’s picture

Nice explanation, Crell.
And your point about cache clears makes sense, fago.
I have no more points about the class / controller split. Makes sense.

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

@bojanz :)

I've added an issue summary.

chx’s picture

As 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.

aspilicious’s picture

@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 :)

klausi’s picture

Status: Needs review » Needs work
+++ b/modules/comment/comment.module
@@ -1440,147 +1441,7 @@ function comment_access($op, $comment) {
 function comment_save($comment) {

another 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.

klausi’s picture

Status: Needs work » Needs review
FileSize
60.04 KB
FAILED: [[SimpleTest]]: [MySQL] 33,292 pass(es), 106 fail(s), and 52 exception(es). View

Same patch as in #64, only added more type hinting. Want to see what the testbot thinks.

Status: Needs review » Needs work

The last submitted patch, 1184944-entity-classes.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
59.89 KB
PASSED: [[SimpleTest]]: [MySQL] 33,400 pass(es). View

A-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().

klausi’s picture

FileSize
8.97 KB
61.1 KB
PASSED: [[SimpleTest]]: [MySQL] 33,400 pass(es). View

fago 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.

Crell’s picture

Status: Needs review » Needs work
+++ b/modules/entity/entity.module
@@ -256,6 +257,36 @@ function entity_load_unchanged($entity_type, $id) {
+function entity_construct($entity_type, array $values) {
+  $info = entity_get_info($entity_type) + array('entity class' => 'Entity');
+  $class = $info['entity class'];
+  return new $class($values, $entity_type);

Perhaps 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).

fago’s picture

FileSize
52.16 KB
PASSED: [[SimpleTest]]: [MySQL] 33,410 pass(es). View

Thanks klausi, doing that makes sense to me. Here a review:

+++ b/modules/comment/comment.module
@@ -1435,12 +1435,10 @@ function comment_access($op, $comment) {
- *
- * @see comment_int_to_alphadecimal()

This seems unrelated. It belongs to the implementation though, so I've moved it over into the controller.

+++ b/modules/comment/comment.pages.inc
@@ -47,11 +47,12 @@ function comment_reply($node, $pid = NULL) {
-        ))->fetchObject();
-        if ($comment) {
+        ))->fetchAssoc();
+        if (!empty($values)) {
+          $comment = entity_construct('comment', $values);

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.

As 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.

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.

fago’s picture

Status: Needs work » Needs review
fago’s picture

@entity_create() vs entity_construct()

Short discussion history:

@entity_create()

>> The naming here confuses me a bit, maybe because we have field_create_field() which actually saves the field?

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).

#7:

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).

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.

indytechcook’s picture

Status: Needs review » Needs work
+++ b/modules/entity/entity.class.incundefined
@@ -0,0 +1,326 @@
+    elseif (!empty($this->entityInfo['entity keys']['label']) && isset($entity->{$this->entityInfo['entity keys']['label']})) {
+      $label = $entity->{$this->entityInfo['entity keys']['label']};

From Entity::label(). This will never get called since $entity is undefined. Should be replaced with $this.

bojanz’s picture

Status: Needs work » Needs review
FileSize
8.61 KB
51.86 KB
PASSED: [[SimpleTest]]: [MySQL] 33,660 pass(es). View

#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:

@@ -269,7 +269,7 @@ function entity_delete_multiple($entity_type, $ids) {
 }
 
 /**
- * Construct a new entity object.
+ * Construct a new entity object, without saving it to the database.
  *
  * @param $entity_type
  *   The type of the entity.

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...)

indytechcook’s picture

On 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.

fago’s picture

FileSize
51.86 KB
PASSED: [[SimpleTest]]: [MySQL] 33,660 pass(es). View

ok, 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.

fago’s picture

FileSize
52.04 KB
FAILED: [[SimpleTest]]: [MySQL] 30,180 pass(es), 908 fail(s), and 480 exception(es). View

re-rolled the patch

Status: Needs review » Needs work
Issue tags: -D8MI

The last submitted patch, drupal_8_entity_crud.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#85: drupal_8_entity_crud.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8MI

The last submitted patch, drupal_8_entity_crud.patch, failed testing.

indytechcook’s picture

+++ b/core/modules/user/user.testundefined
@@ -732,7 +732,7 @@ class UserCancelTestCase extends DrupalWebTestCase {
diff --git a/modules/comment/comment.entity.inc b/modules/comment/comment.entity.inc

diff --git a/modules/comment/comment.entity.inc b/modules/comment/comment.entity.inc
new file mode 100644
index 0000000..64b450f

index 0000000..64b450f
--- /dev/null

Creating files in modules instead of core/modules

+++ b/modules/comment/comment.entity.incundefined
@@ -0,0 +1,260 @@
diff --git a/modules/entity/entity.class.inc b/modules/entity/entity.class.inc

diff --git a/modules/entity/entity.class.inc b/modules/entity/entity.class.inc
new file mode 100644
index 0000000..3b8f5aa

index 0000000..3b8f5aa
--- /dev/null

Creating files in modules instead of core/modules

+++ b/modules/entity/tests/entity_test.infoundefined
@@ -0,0 +1,7 @@
diff --git a/modules/entity/tests/entity_test.install b/modules/entity/tests/entity_test.install

diff --git a/modules/entity/tests/entity_test.install b/modules/entity/tests/entity_test.install
new file mode 100644
index 0000000..ec2e5bd

index 0000000..ec2e5bd
--- /dev/null

Creating files in modules instead of core/modules

fago’s picture

Status: Needs work » Needs review
FileSize
52.13 KB
PASSED: [[SimpleTest]]: [MySQL] 33,770 pass(es). View

indeed, thanks. Looks like Git is not able to magically resolve everything..

xjm’s picture

@fago -- It probably could; we just don't know the esoteric commands we need. ;)

xjm’s picture

FileSize
3.19 KB
52.13 KB
PASSED: [[SimpleTest]]: [MySQL] 33,812 pass(es). View

Attached 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.)

catch’s picture

Priority: Normal » Major

Bumping 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.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/entity/entity.class.incundefined
@@ -0,0 +1,325 @@
+   * @todo Add invokeHook*() methods.

Shouldn't we do this or is it for a followup

+++ b/core/modules/entity/entity.class.incundefined
@@ -0,0 +1,325 @@
+  public function __construct(array $values = array(), $entity_type) {

Functions without any docblock, many of them in this patch. I think if we are overriding a function we have to write *something*

+++ b/core/modules/entity/entity.class.incundefined
@@ -0,0 +1,325 @@
+   * Set up the object instance on construction or unserializiation.

3th person verbs needed

+++ b/core/modules/entity/entity.class.incundefined
@@ -0,0 +1,325 @@
+   * @todo

Better docs needed

+++ b/core/modules/entity/entity.controller.incundefined
@@ -388,3 +393,158 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+   * Act on entities when deleted before the delete hook is invoked.
...
+  protected function invokeHook($hook, EntityInterface $entity) {
+    if (!empty($this->entityInfo['fieldable']) && function_exists($function = 'field_attach_' . $hook)) {

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.

xjm’s picture

In 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.

xjm’s picture

Assigned: Unassigned » xjm
Status: Needs work » Needs review

I'll work on polishing the docs a bit.

xjm’s picture

Status: Needs review » Needs work
xjm’s picture

Status: Needs work » Needs review
FileSize
17.61 KB
55.11 KB
PASSED: [[SimpleTest]]: [MySQL] 33,983 pass(es). View

First 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.

Dave Reid’s picture

Not 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).

xjm’s picture

@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?)

Dave Reid’s picture

hook_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.

xjm’s picture

Assigned: xjm » Unassigned
xjm’s picture

If 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.

xjm’s picture

Alright, I looked over all the @todos in the patch:

+++ b/core/modules/entity/entity.class.incundefined
@@ -0,0 +1,372 @@
+   * @todo
+   *   Which default language should be used.
+   *
+   * @return
+   *   The property value, or NULL if it is not defined.
+   */
+  public function get($property_name, $language = NULL);
...
+   * @todo
+   *   Which default language should be used.
+   */
+  public function set($property_name, $value, $language = NULL);
...
+   * @todo
+   *   Implement default handling of language.
+   */
+  public function get($property_name, $language = NULL) {
...
+   * @todo
+   *   Implement default handling of language.
+   */
+  public function set($property_name, $value, $language = NULL) {

These relate to default language handling.

+++ b/core/modules/entity/entity.class.incundefined
@@ -0,0 +1,372 @@
+   * @todo Add invokeHook*() methods.

This seems kind of important?

+++ b/core/modules/entity/entity.moduleundefined
@@ -173,19 +173,20 @@ function entity_extract_ids($entity_type, $entity) {
+  // @todo: Once all entities are converted, just rely on entity_create().

@@ -321,6 +352,9 @@ function entity_prepare_view($entity_type, $entities) {
+ * @todo
+ *   Remove once all entity types are implementing the EntityInterface.

@@ -362,6 +396,9 @@ function entity_uri($entity_type, $entity) {
+ * @todo
+ *   Remove once all entity types are implementing the EntityInterface.

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.)

Gábor Hojtsy’s picture

@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 :)

Dave Reid’s picture

Yeah 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').

RobLoach’s picture

This 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.

Crell’s picture

Rob Loach: Eventually yes, but we haven't figured out yet how autoloading will work for modules. Please stay tuned. :-)

webchick’s picture

Tagging.

xjm’s picture

Regarding deletion...

+++ b/core/modules/entity/entity.controller.incundefined
@@ -388,3 +397,167 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+      $this->preDelete($entities);
+      $ids = array_keys($entities);
+
+      db_delete($this->entityInfo['base table'])
+        ->condition($this->idKey, $ids, 'IN')
+        ->execute();
+      // Reset the cache as soon as the changes have been applied.
+      $this->resetCache($ids);
+
+      $this->postDelete($entities);
+      foreach ($entities as $id => $entity) {
+        $this->invokeHook('delete', $entity);
+      }
+      // Ignore slave server temporarily.
+      db_ignore_slave();

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. ;)

+++ b/core/modules/entity/entity.controller.incundefined
@@ -388,3 +397,167 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+    // Invoke the hook.
+    module_invoke_all($this->entityType . '_' . $hook, $entity);
+    // Invoke the respective entity level hook.
+    module_invoke_all('entity_' . $hook, $entity, $this->entityType);
+  }
+}

Hmm, why does the type-specific hook come before the entity-level hook?

xjm’s picture

xpost :P

Gábor Hojtsy’s picture

@Dave Reid: agreed.

fago’s picture

FileSize
1.33 KB
54.8 KB
PASSED: [[SimpleTest]]: [MySQL] 33,995 pass(es). View

Great to see so much movement here! :)

#92 and #98 look good to me, I've added them to the Git repository.

hook_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.

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.

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. ;)

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.

Yeah 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').

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.

Hmm, why does the type-specific hook come before the entity-level hook?

That's our usual ordering.

I've updated the Git repo and re-rolled the patch. Please review the changes.

moshe weitzman’s picture

I'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

fago’s picture

ad #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?

fago’s picture

FileSize
53.22 KB
PASSED: [[SimpleTest]]: [MySQL] 33,989 pass(es). View

@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.

xjm’s picture

Assigned: Unassigned » xjm

I 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.

xjm’s picture

Assigned: xjm » Unassigned
FileSize
6.6 KB
53.14 KB
PASSED: [[SimpleTest]]: [MySQL] 34,000 pass(es). View

Attached:

  • Uses comment_increment_alphadecimal() where appropriate for better readability.
  • Clarifies a few comments.
  • Adds miscellaneous capitalization and punctuation fixes.
xjm’s picture

Also, I think #1346204: [meta] Drupal 8 Entity API improvements addresses all the outstanding questions above, so let's get some final reviews? :D

Status: Needs review » Needs work

The last submitted patch, drupal_8_entity_crud-118.patch, failed testing.

xjm’s picture

xjm’s picture

Assigned: Unassigned » xjm

aspilicious reviewed for me in IRC and found a few more things to clean up:

  • we should add @see comment_increment_alphadecimal()
  • $build['comment_form'] is formatted two different ways
  • @throws should come after @return, not before
  • "exception is thrown" with no @throws -- confusing.
xjm’s picture

Assigned: xjm » Unassigned
FileSize
3.76 KB
53.29 KB
PASSED: [[SimpleTest]]: [MySQL] 34,002 pass(es). View

Alright, the attached clean up the issues mentioned in #122. However, the exceptions are still a little weird:

  1. interface EntityStorageControllerInterface extends DrupalEntityControllerInterface is defined in entity.controller.inc.
  2. class EntityStorageException extends Exception is defined in entity.class.inc, not entity.controller.inc, and is not (yet?) used anywhere.
  3. The docblocks for EntityInterface::save() and EntityInterface::delete() claim they throw EntityStorageException, but this isn't the case anywhere (yet?).
  4. EntityStorageControllerInterface::save() and EntityStorageControllerInterface::delete() throw a plain Exception. Doesn't it seem like EntityStorageException would have been intended for this, not the base interface?

Can anyone clarify what's going on here?

fago’s picture

FileSize
4.29 KB
53.22 KB
PASSED: [[SimpleTest]]: [MySQL] 34,009 pass(es). View

Thanks, I've incorporated #118

$build['comment_form'] is formatted two different ways

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().

fago’s picture

FileSize
53.28 KB
PASSED: [[SimpleTest]]: [MySQL] 34,023 pass(es). View

@$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).

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.pages.incundefined
@@ -35,7 +35,7 @@ function comment_reply($node, $pid = NULL) {
+      $build['comment_form'] = drupal_get_form("comment_node_{$node->type}_form", entity_create('comment', array('pid' => $pid, 'nid' => $node->nid)));

On one line (pid before nid)

+++ b/core/modules/comment/comment.pages.incundefined
@@ -86,8 +86,8 @@ function comment_reply($node, $pid = NULL) {
+      $comment = entity_create('comment', array('nid' => $node->nid, 'pid' => $pid));
+      $build['comment_form'] = drupal_get_form("comment_node_{$node->type}_form", $comment);

On two lines (nide before pid)

+++ b/core/modules/comment/comment.moduleundefined
@@ -738,7 +739,7 @@ function comment_node_page_additions($node) {
+    $build = drupal_get_form("comment_node_{$node->type}_form", entity_create('comment', array('nid' => $node->nid)));
     $additions['comment_form'] = $build;

On two lines but different than the previous one

I would like us to be consistent:

For the first one

$comment = entity_create('comment', array('nid' => $node->nid, 'pid' => $pid));
$build['comment_form'] = drupal_get_form("comment_node_{$node->type}_form", $comment);

Second one can stay the same

$comment = entity_create('comment', array('nid' => $node->nid, 'pid' => $pid));
$build['comment_form'] = drupal_get_form("comment_node_{$node->type}_form", $comment);

Last one

$comment = entity_create('comment', array('nid' => $node->nid))
$additions['comment_form'] = drupal_get_form("comment_node_{$node->type}_form", $comment);

Nice work Xjm and fago!

9 days to next Drupal core point release.

fago’s picture

@aspilicious: Thanks for clarifying - so that should work now in #125? It has those changes included.

aspilicious’s picture

Status: Needs work » Needs review

Well...

+    $comment = entity_create('comment', array('nid' => $node->nid));
+    $build = drupal_get_form("comment_node_{$node->type}_form", $comment);
     $additions['comment_form'] = $build;

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.

aspilicious’s picture

FileSize
1.52 KB
52.04 KB
FAILED: [[SimpleTest]]: [MySQL] 34,023 pass(es), 1 fail(s), and 0 exception(es). View

I changed those lines and a comment. I hope this is rdy to go now :)

xjm’s picture

@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.

fago’s picture

Status: Needs review » Needs work
FileSize
53.32 KB
PASSED: [[SimpleTest]]: [MySQL] 34,021 pass(es). View
+++ b/core/modules/comment/comment.entity.inc
@@ -194,7 +194,7 @@
-    // Care about hook_comment_publish(),
+    // Invoke hook_comment_publish().

I'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.

fago’s picture

Status: Needs work » Needs review

grml

xjm’s picture

Assigned: Unassigned » xjm

I read the patch hopefully for the last time. :) Here's a few things I will fix in a quick reroll:

  1. +++ b/core/modules/comment/comment.entity.incundefined
    @@ -0,0 +1,280 @@
    +      if ($comment->uid === $user->uid && isset($user->name)) { // '===' Need to modify anonymous users as well.
    

    This comment should be on the preceding line.

  2. +++ b/core/modules/comment/comment.entity.incundefined
    @@ -0,0 +1,280 @@
    +    // Care about hook_comment_publish(),
    

    I agree; let's just kill this comment.

  3. +++ b/core/modules/entity/entity.class.incundefined
    @@ -0,0 +1,301 @@
    +    // We support creating entities with pre-defined ids to ease migrations.
    +    // For that the "is_new" property may be set to TRUE.
    

    Another ids vs. IDs.

  4. +++ b/core/modules/entity/entity.class.incundefined
    @@ -0,0 +1,301 @@
    +      // lookup this entity again.
    

    "Look up" should be two words.

xjm’s picture

Assigned: xjm » Unassigned
FileSize
2.25 KB
53.33 KB
PASSED: [[SimpleTest]]: [MySQL] 34,009 pass(es). View

Now I'm going to hassle some folks and ask them to review. :)

xjm’s picture

FileSize
53.33 KB
PASSED: [[SimpleTest]]: [MySQL] 34,013 pass(es). View
+++ b/core/modules/comment/comment.entity.incundefined
@@ -179,7 +179,9 @@ class CommentStorageController extends EntityDatabaseStorageController {
+      if ($comment->uid === $user->uid && isset($user->name)) { ¶

Ahh whitespace!

Fixed:

Status: Needs review » Needs work
Issue tags: -DX (Developer Experience), -D8MI

The last submitted patch, drupal_8_entity_crud-1184944-135.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience), +D8MI
fago’s picture

FileSize
53.33 KB
PASSED: [[SimpleTest]]: [MySQL] 34,128 pass(es). View

Changes 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!

pounard’s picture

I'm all in for this kind of change, but why don't you call you EntityInterface accessors as accessors? Seeing a function just named EntityInterface::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 a EntityInterface::getSomething() method instead (here EntityInterface::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.

tstoeckler’s picture

#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.

Crell’s picture

I 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.

pounard’s picture

It'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.

fago’s picture

I 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.

tstoeckler’s picture

Well, 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.

fago’s picture

Agreed - we should move on with this issue asap given quite some stuff depends on it. Any open points? Else, rtbc anyone? :)

bojanz’s picture

+1 for RTBC. We've dragged this on long enough.

RobLoach’s picture

#138: drupal_8_entity_crud.patch queued for re-testing.

xjm’s picture

I also think this is RTBC.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Come 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!

tstoeckler’s picture

I 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!

catch’s picture

I'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?

Tor Arne Thune’s picture

FileSize
2.9 KB
53.31 KB
PASSED: [[SimpleTest]]: [MySQL] 34,248 pass(es). View
+++ b/core/modules/comment/comment.entity.inc
@@ -0,0 +1,281 @@
+   * - last_comment_name: The name of the anonymous poster for the last
+   *   comment.

Can be written on one line.

+++ b/core/modules/comment/comment.entity.inc
@@ -0,0 +1,281 @@
+   *   this node, or the node author's user ID if no comments exists for the

exists -> exist

+++ b/core/modules/entity/entity.class.inc
@@ -0,0 +1,301 @@
+   *   TRUE if the entity is new, or FALSE if the entity has already been
+   *   saved.

Can be written on one line.

+++ b/core/modules/entity/entity.class.inc
@@ -0,0 +1,301 @@
+   * Sets up the object instance on construction or unserializiation.

unserializiation -> unserialization

+++ b/core/modules/entity/entity.controller.inc
@@ -388,3 +397,170 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+   * Invokes a hook on behalf the entity.

Missing 'of'.

+++ b/core/modules/entity/entity.controller.inc
@@ -388,3 +397,170 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+

New line before EOF.

+++ b/core/modules/entity/tests/entity.test
@@ -0,0 +1,69 @@
+

New line before EOF.

Other than that, this looks great to me. Excited to see this committed :)

xjm’s picture

Re: #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.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

geerlingguy’s picture

@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.

Tor Arne Thune’s picture

Many comments (plural) exist, one comment (singular) exists.
I'm not a native English speaker so I could be wrong though :)

redndahead’s picture

#155 is correct.

theunraveler’s picture

I beg to differ re: comment #83:

On 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.

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.

Dries’s picture

While 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.

Cyberwolf’s picture

Maybe 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.

Crell’s picture

Cyberwolf: http://www.garfieldtech.com/blog/orm-vs-query-builders and the articles linked therein.

catch’s picture

Title: Make entities classed objects, introduce CRUD support » Change notification for: Make entities classed objects, introduce CRUD support
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Alright 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).

Dries’s picture

Title: Change notification for: Make entities classed objects, introduce CRUD support » Make entities classed objects, introduce CRUD support
Priority: Critical » Major
Status: Active » Reviewed & tested by the community

Cyberwolf: do you have more specific thoughts (after reading the things referenced by Larry)?

Gábor Hojtsy’s picture

Title: Make entities classed objects, introduce CRUD support » Change notification for: Make entities classed objects, introduce CRUD support
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Cross-post I guess.

Cyberwolf’s picture

@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.

pounard’s picture

I 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 and return $this->whatever; instead. EDIT: and you could do it, because core entities already have their own class, we're not counting overrides I guess

It 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:

  1. Controller is not being injected at object instanciation but fetched by the object itself
  2. Object knows some database information/description, where the controller would be self-sufficient to set itself the properties over the objects
  3. It's keeping array copies everywhere at places it doesn't need to be
  4. Still does some heavy dynamic stuff (getting the id using the idKey property instead of returning its own idea directly) where it could return the value of an hardcoded and declared property name (thus avoiding potential notices if the id key is not set in the object, for example EDIT: that won't happen because you already declared the properties in specializations)
  5. Objects serialization/deserialization is obfuscated because all of this and objects depends of the meta description where it could easily be avoided

All of this is easily fixable without modifying the public API.

pounard’s picture

BTW: 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.

Crell’s picture

I 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.

pounard’s picture

Yes, it sounds fair to get this issue finalized before starting messing with details.

fago’s picture

@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.

As 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?

sun’s picture

Issue tags: +Entity system
xjm’s picture

Assigned: Unassigned » xjm

Writing this change notification, but maybe not tonight. :)

xjm’s picture

Status: Active » Needs review

First 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.

xjm’s picture

Issue tags: +Needs change record

Not sure what happened to the tag.

xjm’s picture

berdir helped review the change notification in IRC. Suggested changes:

  • Add an example implementation that:
    • extends the base class and adds a property
    • extends the controller class and overrides postSave()
  • List protected methods.
Gábor Hojtsy’s picture

Issue tags: +language-content

Tagging for the content handling leg of D8MI.

klausi’s picture

Change 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?).

xjm’s picture

Title: Change notification for: Make entities classed objects, introduce CRUD support » Make entities classed objects, introduce CRUD support
Assigned: xjm » Unassigned
Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -Needs change record

Thanks @klausi! Let's call this fixed. :)

hinikato’s picture

I suggest to use just the entity() function instead of using entity_create() or entity_construct().

klausi’s picture

@hinikato: please open a new issue for that.

Status: Fixed » Closed (fixed)

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

fago’s picture

Added a follow-up to add type-hinting to comments, as suggested in #32:
#1480866: Add type-hinting and parameter type docmentation for comment objects

David_Rothstein’s picture

Title: Make entities classed objects, introduce CRUD support » CHANGELOG.txt entry for: Make entities classed objects, introduce CRUD support
Priority: Major » Normal
Status: Closed (fixed) » Active

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).

Doesn't look like the CHANGELOG.txt entry ever happened. Perhaps could be moved to a separate issue at this point, if necessary.

Berdir’s picture

Status: Active » Needs review
FileSize
553 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,978 pass(es). View

What 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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good for me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

plach’s picture

Title: CHANGELOG.txt entry for: Make entities classed objects, introduce CRUD support » Make entities classed objects, introduce CRUD support
plach’s picture

Issue summary: View changes

Updated issue summary.