We have the various _load_multiple functions, which is a great thing. What is not so great, though, is that there is a lot of code in them that is the same among all implemenations, but is not shared in any way.

This patch is very much WIP.

First of all, naming. In this patch I went with "entity" for a "loadable thingy, that can optionally be fieldable". I.e. nodes, users, taxonomy terms, taxonomy vocabularies, files.

I am not sure if this is the best name, but it was the best I could come up with. Please don't yet start bikeshedding on the name, I am willing to change it but first I'd like some serious reviews.

Here's an overview on what the patch does:

  • hook_fieldable_info() is renamed into hook_entity_info(). The information provided there (name, description, id key, revision key) is in not specific to field API. I needed this information anyways to provide a working default implementation of the entity loader. hook_entity_info() has additional keys now: 'base table', the name of the entity's base table, 'loader class', the name of the loader class (see below), and a boolean 'fieldable'. Field API simply only cares for entity types that have 'fieldable' => TRUE.
  • Added a function drupal_load_multiple(). node_load_multiple, user_load_multiple etc. are just convenience wrappers around drupal_load_multiple now. If we want, we can remove them in a follow up patch.
  • drupal_load_multiple() is a factory function, and now we're approaching the heart of the patch:

In a very first working revision of the patch, I just took the various _load_multiple functions and generalized them into a drupal_load_multiple function. But the problem is that there are some differences between the various implementations, and so one needed to provide hooks at many places, forcing node.module, user.module and taxonomy.module to implement many special-named functions to provide proper loading of their own entity types, which was kinda ugly IMO from a DX POV - it was not clearly visible what was happening upon loading.

So I went with a different approach. I created a very very basic interface DrupalEntityLoader, that has just two methods - loadMultiple($ids, $conditions) and <code>resetCache().
In hook_entity_info, for each entity type, a loader class key is returned. This class has to implement DrupalEntityLoader and is instanciated (only once, as a Singleton) by drupal_load_multiple().

This makes it possible to completely and cleanly swap the entity loading mechanisms (via hook_entity_info_alter)!
(Remember Crell's law, right?)

Now, I wrote a general DrupalDefaultEntityLoader class that replaces, without any special casing, file_load_multiple and taxonomy_term_load_multiple! It can also attach fields (if 'fieldable' is TRUE for the entity type) and calls hook_ENTITY_TYPE_load(). For node, user and taxonomy_vocabulary, a little special casing was needed, so these entity types provide their own loader class that extends DrupalDefaultEntityLoader, providing only very little own code.

So - we used to have 5 _load_multiple functions in core (node, user, file, taxonomy_term, taxonomy_vocabulary) that duplicated a lot of code. All this is gone, we have a completely swappable entity loading mechanism (think couchdb, anyone? I know this is far off, but is one of the needed steps when going into this direction), we have a default implementations that can handle many things including fields and calling the load hooks, and the default implementations is easily expandable for users and nodes, which need some custom code.

There are many follow up patches possible, e.g. moving more of the custom code for nodes and users to the default implementations by letting it handle basic joins, or trying to remove the user and taxonomy special casing by moving stuff (e.g. user pictures) to fields, or basing node types upon the object loaders somehow, or moving the node building into the loader class, or fetching the entities into actual objects instead of stdClasses that provide more functionality, etc. The great thing is that many of these improvements would then not only go to node, but also to all other entity types.

Also, I know that the documentation needs work, this is a first WIP patch, after all.

Oh, and this patch could be split up to e.g. convert only node and user in the first go, but I coded up the other (simpler) implementations in the first go as well to test the general API. Dunno if it's worth to split it up.

Setting CNR to get a testbot run, even though this definitely still needs work (and might have some fails as well).

CommentFileSizeAuthor
#180 460320-entity_24.patch77.36 KBTheRec
Unable to apply patch 460320-entity_24.patch View
#178 entity.patch78.19 KBcatch
Failed: Failed to apply patch. View
#174 entity.patch77.67 KBcatch
Failed: Failed to apply patch. View
#171 entity.patch77.66 KBcatch
Failed: Failed to apply patch. View
#166 460320.patch75.66 KBRobLoach
Failed: 12339 passes, 4 fails, 3 exceptions View
#154 entity.patch77.38 KBcatch
Failed: Failed to apply patch. View
#151 entity.patch75.94 KBcatch
Failed: Failed to apply patch. View
#144 entity.patch75.29 KBcatch
Failed: Failed to apply patch. View
#142 entity.patch75.48 KBcatch
Failed: 12002 passes, 29 fails, 963 exceptions View
#140 entity.patch75.47 KBcatch
Failed: 10303 passes, 834 fails, 1773 exceptions View
#138 entity.patch75.48 KBcatch
Failed: Failed to apply patch. View
#136 load2.patch79.52 KBfago
Failed: Failed to apply patch. View
#134 load2.patch79.06 KBfago
Failed: Failed to apply patch. View
#131 entity.patch80.43 KBcatch
Failed: Failed to apply patch. View
#128 entity.patch74.72 KBcatch
Failed: Failed to apply patch. View
#127 entity.patch74.85 KBcatch
Failed: Failed to apply patch. View
#126 entity.patch74.89 KBcatch
Failed: Failed to apply patch. View
#123 entity.patch75.15 KBcatch
Failed: 12071 passes, 13 fails, 22 exceptions View
#122 entity.patch75.52 KBcatch
Failed: 12092 passes, 21 fails, 22 exceptions View
#117 entity.patch75.05 KBcatch
Failed: Failed to apply patch. View
#115 entity.patch86.78 KBcatch
Failed: 11618 passes, 0 fails, 58 exceptions View
#112 entity.patch73.28 KBcatch
Failed: 10882 passes, 99 fails, 570 exceptions View
#110 entity.patch73.25 KBcatch
Failed: 10882 passes, 99 fails, 2135 exceptions View
#105 entity.patch72.88 KBcatch
Failed: Failed to apply patch. View
#103 entity.patch72.51 KBcatch
Failed: 10775 passes, 0 fails, 66 exceptions View
#101 entity.patch62.15 KBcatch
Failed: Failed to install HEAD. View
#97 entity.patch72.18 KBcatch
Failed: Failed to apply patch. View
#96 entity_loading.patch69.46 KBcatch
Failed: 10722 passes, 0 fails, 64 exceptions View
#92 entity_loading.patch69.46 KBcatch
Failed: Failed to apply patch. View
#89 entity_loading.patch69.54 KBcatch
Failed: Failed to apply patch. View
#87 entity_loading.patch68.26 KBcatch
Failed: 10583 passes, 6 fails, 0 exceptions View
#83 entity_loading.patch61.45 KBcatch
Failed: 10583 passes, 7 fails, 4122 exceptions View
#81 entity_loading_4.patch60.2 KBcatch
Failed: 10135 passes, 492 fails, 293 exceptions View
#77 entity_loading.patch61.24 KBcatch
Failed: Failed to apply patch. View
#75 entity_loading.patch51.2 KBcatch
Failed: Failed to install HEAD. View
#73 entity_loading.patch63.66 KBcatch
Failed: Failed to apply patch. View
#70 entity_loading.patch51.68 KBcatch
Failed: Failed to install HEAD. View
#69 entity_loading.patch63.16 KBcatch
Failed: Failed to apply patch. View
#67 entity_loading.patch63.03 KBcatch
Failed: Failed to apply patch. View
#67 entity_loading-loader.patch63.17 KBcatch
Failed: Failed to apply patch. View
#64 entityloading_frh.patch60.87 KBFrando
Failed: Failed to apply patch. View
#58 entityloading_frh.patch60.91 KBFrando
Failed: 10433 passes, 21 fails, 16 exceptions View
#53 entityloading_frh.patch64.74 KBFrando
Failed: Failed to apply patch. View
#50 entityloading_frh.patch61.71 KBFrando
Failed: 11125 passes, 0 fails, 14 exceptions View
#48 entityloading_frh.patch58.63 KBFrando
Failed: Failed to apply patch. View
#46 entityloading_frh.patch58.73 KBFrando
Failed: Failed to apply patch. View
#42 entity.patch63.42 KBcatch
Failed: Failed to apply patch. View
#32 entityloading_frh_TESTS_DISABLED_3.patch59.22 KBFrando
Failed: Failed to apply patch. View
#31 entityloading_frh_TESTS_DISABLED_2.patch61.76 KBFrando
Failed: Failed to apply patch. View
#30 entityloading_frh_TESTS_DISABLED_1.patch61.94 KBFrando
Failed: Failed to apply patch. View
#25 entityloading_frh.patch54.99 KBFrando
Failed: 11252 passes, 3 fails, 0 exceptions View
#22 entityloading_frh.patch54.99 KBFrando
Failed: 11252 passes, 3 fails, 0 exceptions View
#9 entityloading_frh.patch52.6 KBFrando
Failed: 11246 passes, 3 fails, 0 exceptions View
#3 entityloading_frh.patch47.63 KBFrando
Failed: 11031 passes, 227 fails, 179 exceptions View
entityloading_frh.patch47.71 KBFrando
Failed: Failed to apply patch. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Frando’s picture

Title: Standarize entity loading » Standarize all entity loading (nodes, users, taxonomy, files)
Assigned: Unassigned » Frando

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

Status: Needs work » Needs review
FileSize
47.63 KB
Failed: 11031 passes, 227 fails, 179 exceptions View

Hrm, forget to set --no-prefix while diffing. This one actually applies.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

So I'd been wanting to go back and consolidate this code for quite a while, but like frando says there's a lot of special casing based in the history of these modules which makes that hard. Doing it this way is an interesting way of getting around that.

Even ignoring couchdb, this'd make it possible to do #111127: Cache node_load entirely from contrib - and the same for any other 'entity', so putting all objects into memcache becomes a very easy thing to do without hacking core (cache clearing is easy to do from hooks anyway). So it'd have both short and longer term benefits there.

I'm wondering about attachLoad() a bit. Body as field should remove the join on node revisions, the user information could either be a userreference field (or more short term just use user_node_load()). Having the capability to do this is great, but maybe we need to spin a couple of issues off while this is in progress so it's less necessary - would make the patch smaller that way.

Side note, #343788: Taxonomy module doesn't use it's own APIs adds a special case to taxonomy_term_load_multiple() so we'd need a buildQuery() for terms if/when that's in.

sun’s picture

Nice job, frando. Even comes with very nice comments that even me can understand.

Tracking.

moshe weitzman’s picture

Oh my, this is nice. Sure it makes Drupal more abstract but really the benefits outweigh the risks. Learning this abstraction brings much rewards to the drupal dev. consistency++.

I thought I would be smart and ask that Frando add a tag for the default load queries but he did that already :)

Lets get those field system tests passing and push this one in.

Would be nice to get early feedback from the committers. I will email them.

joshk’s picture

Subscribing. I have yet to start serious testing w/d7 but I like the look of this.

It's the sort of stuff that caused learning Drupal to turn me into a much better programmer. ;)

Frando’s picture

Status: Needs work » Needs review
FileSize
52.6 KB
Failed: 11246 passes, 3 fails, 0 exceptions View

Great to see some positive voices here.

Getting some early feedback by webchick/Dries on the direction of the patch would be amazing!

The attached patch should fix most, if not all, test fails. Also includes some minor documentation improvements.

@catch #5 reg. attachLoad(): Surely it would be great to get rid of the special cases for nodes and users. I dunno, though, if this really is feasable for this patch. Simply removing them would be a regression. Reworking them to be less special-casey is an important task, but I'd like to move that to a folllow-up issue to not hold this up, as there will surely arise quite some discussion.

chx’s picture

Pray tell me, why this is being considered if handlers is not?

Frando’s picture

Well, arent't there already quite a few places in core where we are doing handler-style abstractions?

Should the general handler patch go in, this could and should surely be converted to be based on that. But I don't think that the handler patch should hold up improvements in other areas, only because they tend to use similar abstraction patterns.

This patch consolidates code that otherwise has to be maintained seperately and opens the door for many other improvements. Now for example if we add entity caching in core (or contrib, which is only possible with this patch), it does not only help nodes, but also users, taxonomy terms and vocabularies.

And btw, 100% passes! Yay!

catch’s picture

The reason handlers isn't in yet is because it's a layer of abstraction over and above doing things like this right? Seems like a different thing to me.

edit: @frando, definitely not removing the special casing in this patch, it just makes it much more obvious, but I opened #461298: Move node_load_multiple() special casing into user_node_load() to make a start on it.

chx’s picture

Frando, read the handlers patch, you almost reimplemented the whole thing...

Frando’s picture

chx, I read the handlers patch, more than once. I like it a lot, for what it's worth. But in this patch, the part that does handlers style stuff is what, 15 LOCs plus comments. So, I don't think at all that this patch should be held up because it does a similar kind of abstraction as the handlers patch does. Should the latter get in, this patch could surely use its API. But please, for now, judge this patch on its own merits and not on the ones of another patch that is still quite a bit away from being committed.

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

Status: Needs work » Needs review

Hm, all the 3 fails are in "Taxonomy vocuabularies", but locally I get 100% percent passes there, so I cannot reprocue the failures. Requesting retest.

Edit: I reran the tests locally on a clean HEAD checkout plus this patch, and I get 47 passes and 0 failures for "Taxonomy vocabularies", while the Testbot reports three failures. Can anyone reproduce the testbot's behaviour?

The last submitted patch failed testing.

catch’s picture

Status: Needs review » Needs work

Can't reproduce the failures locally either.

Despite this being a 50k patch, most of it is just moving code around, and only 19 lines added:
13 files changed, 552 insertions(+), 533 deletions(-)

Found some minor nitpicky things, otherwise hard to find much to complain about at this point:

+        // If loading entities only by conditions, fetch all available entities from
+        // the cache. Nodes which don't match are removed later.

s/Nodes/Entities

s/entites/entities

<code>
+  public function resetCache() {
+    $this->entity_cache = array();
+  }
+
+  public function loadMultiple($ids = array(), $conditions = array()) {

No phpdoc.

+class NodeLoader

No phpdoc.

edit: Neutral benchmarks between HEAD and the patch.

chx’s picture

Well at least this patch gives a good reason to why handlers are so badly needed -- that issue was shot down as "oh we will just use a pattern" but it turns out that everyone will use a slightly different pattern and we end up with a mess. Sure, we can go ahead with this one without handlers, but it's a mess -- one that I contributed to with the queue patch which is also quite handlers like but does things differently.

Somewhere this madness must stop. We are not simply making Drupal more OOP-looking but using dynamically constructed class names which makes code flow very very hard to follow. I have again, contributed to this mess by DBTNG and the queue patch too. I have tried to blow a whistle already at #363802: Document class methods with class, factory and defining interfaces and was shut down. I tried to help API module but that's a slow process. (Note that DBTNG and queue especially are secreted away in their own corners. I can open up taxonomy module and copy how a query is made without understanding really the code flow behind db_select.)

Of course, this patch can go forward regardless hoping that handlers happen one day (to avoid learning multiple handlers-like mechanisms) and hoping that api.drupal.org will deal with OOP one day (to help following the code flow). It's still rather unclear to me how the heck will any static code analyzer (like the api module or any IDE) follow these dynamic class constructs, but sure, just go ahead.

chx’s picture

This where we need committer guidance: do we pour in as many features as we can during code thaw and work on documentation during freeze or do we stop now work on docs and api module hoping that the thaw is long enough to get this in too?

Or simply I am misguided, this patch is easy to follow and there is no concern at all.

sun’s picture

As much as I like this patch, I'm sharing chx's concerns. We badly need some core committer guidance here.

My take is: It feels wrong to introduce yet another pattern. But OTOH, adding another one could as well help to properly flesh out handlers, and, this patch could probably committed a bit faster. Ideally, we do both, but make sure that this patch already accounts for conversion to handlers later on.

Frando’s picture

Status: Needs work » Needs review
FileSize
54.99 KB
Failed: 11252 passes, 3 fails, 0 exceptions View

Improved the documentation a bit, and fixed the nitpicks catch mentioned. Following DBTNG, I didn't add docblocks to the resetCache() and loadMultiple() methods as they are documented in the interface.

Now. Of course this makes the code flow a bit more complex. But I really think the benefits outweigh the disadvantages. The former being a consistent load API for all core entity types and the possiblity to completely replace them in contrib. And it would be a perfect fit for the handlers patch, sure. I know that I won't have the time to really work on the handlers patch, but I will gladly convert this patch to use its API, should handlers get in.

And I think (read: fear) it is already a requirement for D7 to be released to make api.module able to parse class and interface doxygen. DBTNG already forces us to do so (try to look up the different fetch methods of a select query on api.drupal.org - good luck). So, this has to be addressed, anyways. Maybe we need some "api.module sunday" or something.

Also note that all public facing API functions are completely unchanged. The changes here are purely internal. node_load_multiple() et. al. and the related _load hooks are not changed at all, but now use the same API, instead of a different implementation each.

Edit: I still cannot reproduce the three fails in taxonomy vocabularies test. I get 100% passes here. Weird.
Edit 2: I do share chx's and sun's concerns. But I don't think that this code is especially hard, and I think that they can be solved by improving documentation, and that they shouldn't hold innovation and this patch up. That's my opinion, at least. And I agree with sun that if this gets in, it could make it easier to work on handlers by having another perfect use case in core.

The last submitted patch failed testing.

Frando’s picture

Status: Needs review » Needs work

I still don't get where the three fails in taxonomy vocabularies are coming from. I'm having 100% passes on a fresh and clean environment with the patch from #22 locally. Weird. Anyone with good testing knowledge maybe has an idea?

Frando’s picture

Status: Needs work » Needs review
FileSize
54.99 KB
Failed: 11252 passes, 3 fails, 0 exceptions View

Here we go again, once more.

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Status: Needs work » Needs review

Patch does work for me too when running it with run-tests.sh. Not sure why it doesn't work. Let's try again...

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Not sure how we're going to debug this - possible brute force approach is posting three patches - each with one of the test methods commented out, then narrow down from there - if we can see which assertions are failing it should get more obvious.

Frando’s picture

Status: Needs work » Needs review
FileSize
61.94 KB
Failed: Failed to apply patch. View

Hm - so here we go, bruteforcing the testbot to find out where it fails.

The reported and unreproducible fails are all in "taxonomy vocabularies".
This test contains three methods - testTaxonomyVocabularyLoadReturnFalse, testTaxonomyVocabularyLoadStaticReset and testTaxonomyVocabularyLoadMultiple.

Now, in the patch attached here, all methods in TaxonomyVocabularyUnitTest but testTaxonomyVocabulary are commented out.

Frando’s picture

FileSize
61.76 KB
Failed: Failed to apply patch. View

All but testTaxonomyVocabularyLoadStaticReset commented out.

Frando’s picture

FileSize
59.22 KB
Failed: Failed to apply patch. View

All but testTaxonomyVocabularyLoadMultiple commented out.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Did a quick review a few days ago, but didn't find the time to post it so far.

1) Generally speaking: IMO this is the only way we'll ever get closer to the mythical 'data API' : take baby steps and start by abstracting out one feature - and load seems to make most sense as the 1st op being shared. Then, hopefully, more features (save, delete ?) can be ported. +1 on the general idea.

2) Given the above, it seems important to do this first step keeping a future broader scope in mind, so that we don't paint ourselves in a corner. In the current approach, would a later 'delete data API op' mean 'deleter class' => 'DrupalDefaultEntityDeleter' ?

3) $this->entity_cache / DrupalEntityLoader::resetCache(): It seems awkward to introduce a new way of clearing a static cache when we just recently settled on a new static cache API ?

4) +1 on sticking more metadata in hook_fieldable_info() / hook_entity_info()

5) minor : if you introduce a 'static cache' boolean property, then the existing 'cacheable' should be renamed 'persistent cache' (and its logic reverted) ?

6) 'base table' : I find this highly interesting in the light of #443422: 'per field' storage engine (proposal 2), where we need to state that some entity types primarily live in the local db, while some others (non-core...) primarily live remotely. It's probably just a matter of having each entity type state which field storage engine it primarily relates to: node, user, etc. relate to the default 'field_sql_storage'.

8) PHPdoc for drupal_load_multiple:

+ * @param $conditions
+ *   An array of conditions on the entity base table in the form 'field' => $value.

Not accurate, since taxonomy allows conditions on node 'type', which is not a column in the 'taxonomy_term_data' base table.

7) $this->revision_table = $entity_type . '_revision';: should we hardcode that much ?

8) I'm not sure I like the attachLoad() method - both the name and the actual function:
- 'attach' in API function names is currently a Field API thing, not sure we want to take this to a broader level
- Currently, being 'fieldable' means : "call the various field_attach_*() functions at the places that make sense in your workflow".
Having DrupalDefaultEntityLoader::attachLoad() take care of calling field_attach_load() means we'd need to complete that with "... except for field_attach_load(), which is taken care of if you use DrupalDefaultEntityLoader".

9) minor: in _field_info_collate_types(), $fieldable_types is a misnomer, since it includes types that are not fieldable.

yched’s picture

"...then the existing 'cacheable' should be renamed 'persistent cache' (and its logic reverted) ?"
More generally, it seems the current take of #111127: Cache node_load is 'postponed, DrupalEntityLoader would allow it in contrib' ? If so, should Field API keep its 'cacheable' / 'use field cache only for entity types that don't have their own cache' stuff ?

catch’s picture

We don't want to duplicate the field cache - I can't think of use cases for having both used at the same time for the same entity type, so IMO yes it should. Or am I missing something?

yched’s picture

It's just that if node cache is implemented in contrib, then nodes are not inherently field-cacheable or not field-cacheable. No big deal with drupal_alter('entity_info'), I guess.

catch’s picture

That makes sense, in which case an alter hook would be good - but we still need the concept I think.

webchick’s picture

subscribe. I really wanted to look at this last week but that didn't pan out. Going to try this weekend instead. :)

bjaspan’s picture

subscribe

catch’s picture

re-rolling the brute force patches over at #474502: Brute force the entity loading test bot failures

catch’s picture

Status: Needs work » Needs review
FileSize
63.42 KB
Failed: Failed to apply patch. View

This is CNW after yched's #34, but setting back to CNR because I've got all tests passing (I hope, test bot will give us final confirmation). Was missing an orderBy in the vocabulary loader class.

catch’s picture

Addressed a couple of yched's minor points and fixed indenting in the taxonomy test. So this is properly back to needs review now.

Remaining from yched - all of which need more discussion:

3) $this->entity_cache / DrupalEntityLoader::resetCache(): It seems awkward to introduce a new way of clearing a static cache when we just recently settled on a new static cache API ?

I agree on this. Modules using the node / taxonomy / file APIs shouldn't need to know about the entity loader (and we haven't quite got to a point where they'll never need to reset the static cache themselves). I also found that via taxonomy_term_path(), taxonomy_vocabulary_load_multiple() can be called hundreds of times during a request, and we can save around 2% in those cases by sharing the static cache with taxonomy_vocabulary_load() - see #470398: taxonomy_vocabulary_load() should share the _multiple() static cache. Given there'd be extra layers of redirection to do here, leaving that kind of optimization open could be useful. However I didn't try to go through and remove this in the last patch and would be interested to hear why Frando went in that direction.

5) minor : if you introduce a 'static cache' boolean property, then the existing 'cacheable' should be renamed 'persistent cache' (and its logic reverted) ?

Makes sense to me - but again didn't address this in the last patch.

6) 'base table' It's probably just a matter of having each entity type state which field storage engine it primarily relates to: node, user, etc. relate to the default 'field_sql_storage'.

Views just committed a patch for remote storage and 'base table' comes from there, haven't looked at how this issue was dealt with but would be interesting to look.

7) $this->revision_table = $entity_type . '_revision';: should we hardcode that much ?

Add another key to hook_entity_info() instead?

8) I'm not sure I like the attachLoad() method - both the name and the actual function:
- 'attach' in API function names is currently a Field API thing, not sure we want to take this to a broader level
- Currently, being 'fieldable' means : "call the various field_attach_*() functions at the places that make sense in your workflow".
Having DrupalDefaultEntityLoader::attachLoad() take care of calling field_attach_load() means we'd need to complete that with "... except for field_attach_load(), which is taken care of if you use DrupalDefaultEntityLoader".

Well the entity loader takes care of a lot of other worklfow as well. The only way I can see to get around this would to be to entirely remove the static caching support from entity loading and have entities manage that themselves. That'd bring back a lot of the code duplication we have now and generally make things pretty messy. I'd rather see more field_attach_*() functions being taken care of if we manage to unify insert/update/delete in later patches (even if that's in Drupal 8).

Berdir’s picture

A few minor things... :)

+      $queried_entities = $this->query->execute()->fetchAllAssoc($this->id_key);

chained methods should be on a separate line, even if there are only two..

+    if ($this->conditions) {
+      foreach ($this->conditions as $field => $value) {
+        $this->query->condition('base.' . $field, $value);
+      }
+    }

As I already said, I'd love to have the ability to pass in a DatabaseCondition object as that would allow to use other operators than just '=' and also combinations. But we can do that later on..

RCS file: /cvs/drupal/drupal/modules/block/block.module,v
retrieving revision 1.337
diff -u -p -r1.337 block.module
--- modules/block/block.module	27 May 2009 18:33:54 -0000	1.337
+++ modules/block/block.module	28 May 2009 00:42:36 -0000
@@ -634,10 +634,16 @@ function _block_render_blocks($region_bl
       // Erase the block from the static array - we'll put it back if it has content.
       unset($region_blocks[$key]);
       if ($block->enabled && $block->page_match) {
-        // Try fetching the block from cache. Block caching is not compatible with
-        // node_access modules. We also preserve the submission of forms in blocks,
-        // by fetching from cache only if the request method is 'GET' (or 'HEAD').
-        if (!count(module_implements('node_grants')) && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD') && ($cid = _block_get_cache_id($block)) && ($cache = cache_get($cid, 'cache_block'))) {
+        // Try fetching the block from cache. By default, block caching is
+        // disabled when using any node_access module. This may be overridden by
+        // setting the block_cache_node_access_bypass variable to TRUE (usually
+        // done in settings.php). This allows you to set block cache settings
+        // manually to per-role or per-user using a custom or contributed
+        // module where both node access and caching for authenticated users
+        // is required. @TODO, remove this in favour of better per-user caching.
+        // We also preserve the submission of forms in blocks, by fetching from
+        // cache only if the request method is 'GET' (or 'HEAD').
+        if ((variable_get('block_cache_node_access_bypass', FALSE) || !count(module_implements('node_grants'))) && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD') && ($cid = _block_get_cache_id($block)) && ($cache = cache_get($cid, 'cache_block'))) {
           $array = $cache->data;

Bad block.module hijacked the patch...

common.inc
+interface DrupalEntityLoader {
+class DrupalDefaultEntityLoader implements DrupalEntityLoader {

node.module
+class NodeLoader extends DrupalDefaultEntityLoader {

taxonomy.module
+class TaxonomyVocabularyLoader extends DrupalDefaultEntityLoader {

Hm.. The registry allows us to lazy-load classes.. maybe we should move atleast some of them to their own files? common.inc is already huge.

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

FileSize
58.73 KB
Failed: Failed to apply patch. View

Thanks everyone.

Reroll to keep up with HEAD attached.
Addressed the points Berdir mentioned. I did not move code to seperate files, though. Sure we could move the interface and the class from common.inc to a new file. Should I do this? Dunno if it's worth a new, seperate include file. If so, I'd suggest includes/entity.inc. That's basically a committer's decision (whether its worth an own include file).

7) $this->revision_table = $entity_type . '_revision';: should we hardcode that much ?
Add another key to hook_entity_info() instead?

Did that in this patch.

Regarding static caching. I really don't mind how we do it. Dries recently stated that drupal_static_reset() should not be used outside of tests, that's why I went with the approach taken here. I welcome other suggestions, but I tend to leave it as-is until some committer feedback on what's the "right" way currently to do static variable clearing inside regular code.

Well the entity loader takes care of a lot of other worklfow as well. The only way I can see to get around this would to be to entirely remove the static caching support from entity loading and have entities manage that themselves. That'd bring back a lot of the code duplication we have now and generally make things pretty messy. I'd rather see more field_attach_*() functions being taken care of if we manage to unify insert/update/delete in later patches (even if that's in Drupal 8).

I agree 100%.

Frando’s picture

Status: Needs work » Needs review
Frando’s picture

FileSize
58.63 KB
Failed: Failed to apply patch. View

this one is better.

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

Status: Needs work » Needs review
FileSize
61.71 KB
Failed: 11125 passes, 0 fails, 14 exceptions View

Puh, reroll attached. Starts getting hard to track the changes to the load_multiple functions ... Git annotate helps, though.

I incorporated the changes to taxonomy term loading that went in recently. For this, I moved the static cache handling code into a method of its own, to make it easier to modify or extend just that part of the entity loading process. I needed to reshuffle the code a little. I'd love catch's feedback on that part.

I also renamed the interface DrupalEntityLoader to DrupalEntityLoaderInterface.

Little summary on open issues:
* static cache clearing. As I said, I really don't mind how we do it. Possibilites are drupal_static_reset(), calling the resetCache() method or passing a parameter to drupal_load_multiple().
* should the DrupalDefaultEntityLoader class stay in common.inc or move somewhere else? and where?

Both these issues are really just decisions that need to be made. Most likely by commiters ;)

catch’s picture

Don't have time to do a full review but the cacheGet method looks fine and cool that it can handled weirdness like taxonomy_get_term_by_name().

Maybe consider putting the default class into system.entityloader.inc or somewhere like that?

I want to run some basic performance tests on this, hopefully later tonight.

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

Status: Needs work » Needs review
FileSize
64.74 KB
Failed: Failed to apply patch. View

This should fix the exceptions. Also moved DrupalEntityLoaderInterface and DrupalDefaultEntityLoader into includes/entity.inc and added docblocks to the new methods.

catch’s picture

yched's example of a future entity delete standardisation being called EntityDeleter makes me think this should just be Load class, not Loader class. Any reason to use Loader rather than Load?

+ * Entitiy types
typo.

Crell’s picture

"Loader" is a common name for delegated factories, from what I've seen. It's a thingie that loads stuff, therefore it is a loader.

Subscribe!

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

Status: Needs work » Needs review
FileSize
60.91 KB
Failed: 10433 passes, 21 fails, 16 exceptions View

The fieldable taxonomy terms patch broke this. Merged back in the changes. This should now apply cleanly again and hopefully still passes all tests.

I thought some more about the class naming and I actually like Loader better than Load. As Crell noted, it seems to be a good term for a class that handles loading.

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

OK, I will try to submit a patch that passes all tests again tonight.

I'd like to rename the functions to entity_load_multiple. drupal_load_multiple is too similar to drupal_load which is something completely different. Using entity_* for our yet-to-come entity API is the right way to go forward IMO.

Now, some committer feedback would be great! So far everyone seems to like my code, which is cool :) And the sooner we get this in, the more we can still try to build upon this for D7 (read: standarize more parts of our entity API). I still think a common entity loading mechanism is a very important first step in finally trying to build a "data API". Next steps would be entity saving, building, deleting - but think of the kittens ...

yched’s picture

Crosspost to #412518-50: Convert taxonomy_node_* related code to use field API + upgrade path which identifies another use case for this.

Frando’s picture

Status: Needs work » Needs review
FileSize
60.87 KB
Failed: Failed to apply patch. View

So here's a quickly updated patch that should hopefully pass all tests again. Also renamed the new functions to entity_* to avoid confusion with drupal_load(). No other changes.

kika’s picture

How would you call future classes for deleting and saving? Deleter / Saver? The latter sounds especially weird.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
63.17 KB
Failed: Failed to apply patch. View
63.03 KB
Failed: Failed to apply patch. View

Re-rolled.

Attached two versions of the patch one with 'Load class', one with 'Loader class' for comparison.

catch’s picture

Priority: Normal » Critical
Issue tags: +Performance, +Views in Core, +Fields in Core
catch’s picture

FileSize
63.16 KB
Failed: Failed to apply patch. View

Fixed a couple of test fails from the last patch.

So... things to decide:

1. Loader class vs. Load class. I lean towards 'Load' but if 'Loader' is standard in other projects could be persuaded either way.

2. How to reset statics caches - helper function, drupal_static_reset() entity_get_loader()->resetCache();

catch’s picture

FileSize
51.68 KB
Failed: Failed to install HEAD. View

Missed one. Next up, benchmarks.

catch’s picture

10 nodes, front page, ab -c1 -n1000

HEAD:

Requests per second:    7.94 [#/sec] (mean)
Time per request:       125.990 [ms] (mean)
Time per request:       125.990 [ms] (mean, across all concurrent requests)
Transfer rate:          137.35 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   114  126  12.1    122     256
Waiting:       75  117  12.2    114     246
Total:        114  126  12.1    122     256

Patch:

Requests per second:    7.87 [#/sec] (mean)
Time per request:       127.013 [ms] (mean)
Time per request:       127.013 [ms] (mean, across all concurrent requests)
Transfer rate:          136.24 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.3      0      10
Processing:   114  127  13.1    122     257
Waiting:       71  118  12.8    114     220
Total:        114  127  13.1    122     257

No obvious impact.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
63.66 KB
Failed: Failed to apply patch. View

grr.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
51.2 KB
Failed: Failed to install HEAD. View

This is the sort of patch which makes me think about using something other than CVS for core.

Either way, re-rolled. Doesn't explode, but didn't roll all tests.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
61.24 KB
Failed: Failed to apply patch. View

With entity.inc this time.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
60.2 KB
Failed: 10135 passes, 492 fails, 293 exceptions View

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
61.45 KB
Failed: 10583 passes, 7 fails, 4122 exceptions View

Getting there, this one fixes a missing fieldable/entity - will let the test bot find remaining gaps.

yched’s picture

I guess the patch should also rename hook_fieldable_info_alter() (was committed after this thread started) to hook_entity_info_alter() ?

Status: Needs review » Needs work

The last submitted patch failed testing.

fago’s picture

Oh my, standardized entity loading is long missing!

* Why not pass the entity info to the constructor? You load it before constructing and in the constructor.
* Code Style: Shouldn't it use CamelCase for all the variables in classes?

@static caching:
Why not just initialize $this->entity_cache with a usual drupal static? That way drupal_static_reset() resets your cache automatically...

catch’s picture

Status: Needs work » Needs review
FileSize
68.26 KB
Failed: 10583 passes, 6 fails, 0 exceptions View

File, node and taxonomy tests all pass now, sorry for the multiple bumps but my laptop can't compete with the 4 minute test bots.

On hook_fieldable_info_alter() I'm not sure. When frando started this, we didn't have the _alter() hook and this introduced hook_entity_info_alter() exclusively. _field_info_collate_types(); uses a different data structure, and at least in the current patch invokes hook_entity_info() directly rather than re-using drupal_get_entity_info(). We'd need one or the other to be refactored for the same _alter() hook to work for both I think.

I think it's desirable for either _field_info_collate_types() to simply use drupal_get_entity_info() directly, or otherwise sync those two up, but we need to look at the implications of this - i.e. if we can use drupal_get_entity_info() directly, we probably don't need to cache it separately either, but we'll still need to drill down which entities are fieldable and which aren't somewhere. I'm concerned about undertaking that much of a refactoring in this patch though, since it's already getting broken every five minutes and has had zero committer feedback so far - it's also likely to introduce conflicts with translatable fields.

Current diffstat - 17 files changed, 717 insertions(+), 620 deletions(-)

We can add comment_load_multiple() to this now, which should get us neutral code weight, however that's going to directly conflict with the fieldable comments patch, so again, reluctant to invite more patch rot at the moment especially when I'm working on both.

On static resets, I think we should probably make a wrapper function, so

drupal_get_entity_loader('taxonomy_vocabulary')->resetCache();

Becomes

entity_reset_static('taxonomy_vocabulary');

Or maybe even:

taxonomy_vocabulary_static_reset(); // and the same for terms, nodes etc.

Which would be consistent with some of the other wrappers introduced in drupal_static() conversions - I'm not tied to any one of these, but don't want this patch to get bogged down in it either.

So CNR for the testbot, @TODO but not necessarily showstoppers:

1. A decision on how to externally reset static caches - stick with the mechanism in the patch or add wrappers.
2. Convert comment_load_multiple() (probably after fieldable comments goes in)
3. Better unification of _field_info_collate_types and drupal_get_entity_info()

One annoyance - I had to make taxonomy_vocabulary_get_names() a direct query on the vocabulary table, because making it a wrapper around taxonomy_get_vocabularies() causes infinite recursion in taxonomy_entity_info(), that function is only used to provide bundle names in taxonomy_entity_info() though so it's pretty small in the scheme of things.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
69.54 KB
Failed: Failed to apply patch. View

Fixed the blogapi fails.

beejeebus’s picture

awesome patch. not a thorough review, but i'll come back to it.

+function taxonomy_vocabulary_load($vid) {
+  return reset(taxonomy_vocabulary_load_multiple(array($vid), array()));
 }

i think the second param to taxonomy_vocabulary_load_multiple is redundant.

    $loaders[$entity_type] = new $class($entity_type);
    if (!$loaders[$entity_type] instanceof DrupalEntityLoaderInterface) {
      throw new IncorrectEntityLoaderTypeException();
    } 

do we want to enforce the interface on entity loaders and add something like the snippet above to entity_get_loader?

+function hook_entity_info_alter(&$entity_info) {
+  // Set the loader class for nodes to an alternate Implement the
+  // DrupalEntityLoader interface.

nitpick, 'an alternate implementation of the...'.

off topic - are we using camelCase for class variables? we seem to do one thing in the DBTNG code and another elsewhere....

leaving at CNR to get eyeballs.

EDIT: correct DrupalEntityLoaderInterface check.

Crell’s picture

ClassName::methodName() and ClassName::$propertyName is the standard we should be using.

I really wish I had time to review this in more detail, but at least for now you have my moral support! :-)

catch’s picture

FileSize
69.46 KB
Failed: Failed to apply patch. View

Fixed all of Justin's points including making the variable names ugly ;) apart from the exception - someone who knows more than zero about OO should comment on that, although I'd lean towards just allowing things to blow up for broken code like we do elsewhere.

beejeebus’s picture

i agree with Crell on the camelCase stuff.

on the interface check, i'd prefer we die screaming as early as possible and with as useful an error message as possible. i think we should add the check, here and in any place where we can't use type hinting in function/method definitions to catch this sort of error.

scor’s picture

subscribe. I like this. This would reduce the code in the RDF patches.

some minor typo comments:

+   * the class can be extendend, and the default query can be constructed by

fix extendend

+    // Call hook_entityType_load(). The first argument for hook_TYPE_load() are

either 'arguments are' or 'argument ... is'

+ *   A DB result object from a query to fetch node entites. If your query

fix entites

Multiple occurrences of 'informations', should be singular.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
69.46 KB
Failed: 10722 passes, 0 fails, 64 exceptions View

Re-rolled.

catch’s picture

FileSize
72.18 KB
Failed: Failed to apply patch. View

Added comment loading.

catch’s picture

Remaining points of discussion/TODOs from #87:

1. A decision on how to externally reset static caches - stick with the mechanism in the patch or add wrappers?
3. Better unification of _field_info_collate_types and drupal_get_entity_info() (possibly followup patch).

Frando’s picture

Number 1 needs a committer's decision IMO. And I'd leave the better unification to a follow up patch as well. Let's get this in.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
62.15 KB
Failed: Failed to install HEAD. View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
72.51 KB
Failed: 10775 passes, 0 fails, 66 exceptions View

Forgot -N

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
72.88 KB
Failed: Failed to apply patch. View

Fixed notices.

This could really, really do with a look from a committer.

recidive’s picture

Nice patch.

I'm just a bit concerned on the term 'entity' as in entity_load_multiple(). I see entity being more like the 'class' of things, not just a single instance of a given 'class'. The use of function name drupal_load() has been proposed before (see #113435: Data API: first steps) for this same thing and I think this makes sense since this reflects where the money is in Drupal, i.e. in it's data.

Status: Needs review » Needs work

The last submitted patch failed testing.

Frando’s picture

Issue tags: +Needs committer feedback

Adding tag. No time right now for another reroll, but really, what this needs more than a reroll each two days is some serious feedback. I'm really low on drupal time currently, but what I thought about still doing to this patch is

1) expose the static cache directly to entity_load_multiple, so when loading already loaded entites by their ids they can be returnded directly by entity_load_multiple
2) add entity_load function that shares the static cache with entity_load_multiple
3) then, likely, instanciate a new loader object for every "real" load operation (real meaning dealing with unloaded IDs or dealing with conditions). In the current patch, doing a node_load e.g. from within a hook_query_alter for the node query could lead to weird bugs (as the same loader object would be shared between two load operations running simultaneously). The requirement for this is to deal with 1) and 2) to minimze the PHP overhead.

Stil - the overall concept stays the same, and really needs committer feedback I think. The OP still applies in lining out the advantages of the patch.

Objections were raised that if we make object loading OO-based, we should transform our entities into actual objects as well (so class Node etc). I agree, but that's a big effort, and likely won't make it into Drupal 8. IMO we won't get our magic standarized data API "all or nothing", but step by step and by standardizing our various APIs, and this is one important step into this directions.

webchick’s picture

I began studying this patch tonight, but I'm going to need some more time with it to post a thorough review.

General initial impressions: Um. This patch is awesome!! It takes gobs and gobs of essentially copy/paste + a couple simple changes code and centralizes it, leading to much improved consistency and lack of code blaot. It also uses objects for their intended purpose without pushing them in the face of every single Drupal developer.

The documentation is going to need some work, though. There are parts here I'm finding confusing which is impacting the length of time it's taking me to kind of wrap my head around things. I should have more stamina tomorrow night to finish up a massive write up. If not, definitely harrass me about it on IRC because I definitely want to get the "needs committer feedback" tag removed this week. :)

catch’s picture

Status: Needs work » Needs review
FileSize
73.25 KB
Failed: 10882 passes, 99 fails, 2135 exceptions View

Rawhide.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
73.28 KB
Failed: 10882 passes, 99 fails, 570 exceptions View

Comments are fieldable now.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

comment_entity_info() seems to be lacking object_key for 'bundle', which probably causes quite a bunch of the fails / exceptions.
Also, isn't the patch supposed to *replace* comment_fieldable_info() with comment_entity_info() ?

catch’s picture

Status: Needs work » Needs review
FileSize
86.78 KB
Failed: 11618 passes, 0 fails, 58 exceptions View

Should clear up the rest of the test fails, at least in comment.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
75.05 KB
Failed: Failed to apply patch. View

Added $Id$ tag to entity.inc, fixed the last of the last test breakage, added some additional comments and cleanup to entity.inc

beejeebus’s picture

this is looking really good! nitpicks follow...

- modules/system/system.install has a whitespace-only change.

<?php
+ // Specify additional fields from the user table.
+ $this->query->innerJoin('node', 'n', 'base.nid = n.nid');
+ $this->query->addField('n', 'type', 'node_type');
+ $this->query->innerJoin('users', 'u', 'base.uid = u.uid');
+ $this->query->addField('u', 'name', 'registered_name');

- should be "Specify additional fields from the user and node tables."

leaving as CNR as these are tiny changes.

merlinofchaos’s picture

Ok, I have read through this patch about half a dozen times, making sure I understand it. Congratulations, catch, this is probably the most thorough code review I've done in a year.

Initial impression:

This is precisely the direction we want to go. We standardize and abstract our entities which allows us to move behavior up a level so that we can have similar behavior on a wider variety of objects. This means that we can stop promoting some kinds of objects to a higher importance than they really should be.

Review notes:

drupal_get_entity_info() seems like it should be named entity_get_info(). I see no reason why that's in the drupal_* namespace when we've already established an entity_* namespace.

+  return empty($entity_type) ? $entity_info : $entity_info[$entity_type];

This will notice if the requested entity type does not exist.

All entity_* functions should be in entity.inc -- since entity.inc is loaded pretty early in the bootstrap I see no reason to have these functions in common.inc when there is an entity.inc.

DrupalDefaultEntityLoader::loadMultiple() should be just DrupalDefaultEntityLoader::load() because there isn't a single/multiple distinction to make.

The same is true for entity_load_multiple()

+      call_user_func_array($module . '_' . $this->entityType . '_load', $args);

This should be specified in the entity info, not generated like this. It can be generated to fit this format if it does not exist in the entity info, but this is hardcoding magic that can't be changed or specified. Also, it then forces this to be documented in hook_entity_info. This is a good thing.

+ *   cacheable: A boolean indicating whether Field API should cache

missing -

Because entity info is cached, we should consider putting hook_entity_info in another file so that the code does not need to be loaded.

Concerns:

This only covers the R in CRUD. And in taking baby steps, that's ok, but we should turn around and immediately and the CUD to it. For this reason, I don't like the name Loader in the object. I'd rather name it something a little more generic. Maybe Controller? Obviously the object isn't the entity itself, and that's fine. This is a Factory object. But if it's not named Loader, that will make it easier to add ::create(), ::save(), ::delete() and other methods. Doing that will also allow us to unify certain nodeapi_* style operations, which is a good thing too.

merlinofchaos’s picture

Above: "This will notice" should be "This will cause a notice"

yched’s picture

Just a side note about merlinofchaos #119, "we should turn around and immediately and the CUD to it":
'taxonomy as field + taxo fields on non-node entities' would need a way to build a page displaying an arbitrary list of entities of a given entity type. Taxonomy listing pages are currently hardcoded to show only nodes. So that would be a really cool followup. Just sayin'.

catch’s picture

FileSize
75.52 KB
Failed: 12092 passes, 21 fails, 22 exceptions View

This will notice if the requested entity type does not exist.

That seems fine to me - much worse things will happen if you're trying to do things with entities that don't exist. Or can you think of a situation where this is E_ALL rather than just completely broken calling code or a more serious issue elsewhere?

Changed loader class to 'controller class' - hoping we can avoid a bikeshed on the name, but seems nicer to have one class which we add stuff to than keep adding classes every time, also some things like 'revision key' are going to be needed to be shared between the classes anyway.

Changed ->loadMultiple to ->load and entity_load_multiple() to entity_load().

hook_entity_info() now has a 'load hook' key.

Dealt with some of the other nitpicks including Justin's. Left the code in includes/common.inc for now since my HEAD blew up when I tried moving it (probably due to user_load()?) - also didn't move code out of .module since sun has a 900k patch to do that more generally, and this doesn't belong in pages.inc or admin.inc

catch’s picture

FileSize
75.15 KB
Failed: 12071 passes, 13 fails, 22 exceptions View

Minus the registry_rebuild() in index.php and added 'load hook' key to system.api.php docs.

merlinofchaos’s picture

Hm. During processing in the function get module_invokes to get entity_info, I think I"d rather see this:

if (empty($info['load hook'])) {
  $info['load hook'] = // The auto generated version.
}

Then you don't actually *need* to include it, but you can. And then you don't need to add $module to the name when it's actually invoked, which is probably more confusing. Also technically at this point it's not a hook, it's a callback (but it's always been a callback).

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Title: Standarize all entity loading (nodes, users, taxonomy, files) » Standarized, pluggable entity loading (nodes, users, taxonomy, files, comments)
Status: Needs work » Needs review
FileSize
74.89 KB
Failed: Failed to apply patch. View

Makes sense. I was thinking of a situation where you might not want a hook_TYPE_load() for your entity at all, but who would possibly do such a thing.

Don't see how that gets us out of doing $module . '_' . $hook though.

catch’s picture

FileSize
74.85 KB
Failed: Failed to apply patch. View

Realised I misunderstood merlin's comment, bit more cleanup, no functional change at all.

catch’s picture

FileSize
74.72 KB
Failed: Failed to apply patch. View

dreditor pointed out that the find/replace for Loader/Controller had pushed several comments over 80 chars. Re-rolled for that, no code changes.

pwolanin’s picture

In general this looks ok, but "entity" is painfully generic.

this is trivial but should
entity_get_controller()

be
drupal_entity_get_controller()? Of should we be calling these object vs entities?

I'm wondering about the performance implications, but how man objects/entities do most pages load?

fago’s picture

As discussed shortly before in #113614: Add centralized token/placeholder substitution to core I started working on a entity property meta data system, which builds upon hook_entity_info() from this patch as suggested. Have a look at #551406: Centralize entity properties meta data.

So this patch currently defaults to have a controller class, which I think shouldn't be required for entities supported by the property meta-data system as entities might get into the system by various ways where a load() isn't possible, e.g. passed by a feed or a webservice. So if we go the route and do a generic hook_entity_info() I think the CRUD stuff has to be optional. Thus for building a generic "hook_entity_info" this shouldn't be restricted to objects either. Of course it's fine if the field API is, though.

I've ran this patch on my test site during development and experienced no issues. And have I mentioned that I really *love* this patch? Its' the first step into a unified API for CRUD which is long missing!

catch’s picture

FileSize
80.43 KB
Failed: Failed to apply patch. View

@pwolanin

entity vs. object, I'd thought the same thing before, but I think using 'object' could lead to confusion with objects in general, which could lead to having to clarify exactly what sort of object we're talking about all the time, which wouldn't be fun. While I didn't like entity when it first got brought up during the data API sprints, it's got a similar feel to node, and really what we're doing here is moving the node system up a level in the same way all the various hook_foo_load() patches and field API have done for adding stuff to things (hook_file_load(), hook_taxonomy_foo_load() and hook_comment_load() all added during D7).

entity_get_info() vs. drupal_entity_get_info(), merlinofchaos pointed out in #119 that we're already using the entity_* namespace, so need for the drupal prefix, so that's a recent change from how it was before, but I think that argument makes sense.

Performance: I think the only measurable performance implication would be if loading objects from the static cache over and over again - this is the same issue as #470398: taxonomy_vocabulary_load() should share the _multiple() static cache, and usually only arises if one entity's code needs to load another (taxonomy_term_path() calling taxonomy_vocabulary_load(), comment_links() calling node_load() etc.).

So I did benchmarks on node/n which calls node_load() eight times in HEAD at the moment to load just one node. There's a tiny hit there (more or less within margin of error though), which ought to be possible to deal with using that same static sharing approach I took in the other issue. However even if we're looking at a 2% hit here in certain circumstances, I measured a 10% performance improvement for node caching over on #111127: Cache node_load, so if we can easily install node_cache, user_cache and term_cache.module in contrib with no core hacks due to this patch, then IMO a very small penalty in core would be acceptable trade-off for much bigger contrib gains. I think it'd be quite possible to remove that overhead using the shared static approach, but 1. that issue in itself has had almost zero interest from anyone so I'm hesitant to try to refactor this patch to use it, I also got complaints about 'micro-optimization' on that issue and other similar ones 2. it'd be easy to do in a followup patch when the main API change has gone in - and this issue unfortunately still has the 'needs committer feedback' tag stamped firmly on it while needing to be re-rolled about twice a week (just got broken again by the file_load() sanity check commit, which would've been fixed by this issue automatically).

Benchmarks:

ab -c1 -n1000 http://d7.7/node/35
HEAD:

Concurrency Level:      1
Time taken for tests:   92.882 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      8959000 bytes
HTML transferred:       8497000 bytes
Requests per second:    10.77 [#/sec] (mean)
Time per request:       92.882 [ms] (mean)
Time per request:       92.882 [ms] (mean, across all concurrent requests)
Transfer rate:          94.20 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    82   93   9.0     90     145
Waiting:       77   87   8.6     84     137
Total:         83   93   9.0     90     145

Patch:

Concurrency Level:      1
Time taken for tests:   94.862 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      8959000 bytes
HTML transferred:       8497000 bytes
Requests per second:    10.54 [#/sec] (mean)
Time per request:       94.862 [ms] (mean)
Time per request:       94.862 [ms] (mean, across all concurrent requests)
Transfer rate:          92.23 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    84   95  10.4     91     180
Waiting:       79   89   9.9     86     174
Total:         84   95  10.4     92     180

@fago: it defaults to providing a load class, but I think if you described an entity which didn't need that you'd not run into serious problems right? We fill in defaults for a lot of info hooks where those defaults might not necessarily be needed in all cases.

Things which have been brought up which I haven't specifically dealt with in the last re-rolls (but don't think are necessary prior to getting a look from Dries and possibly not before commit):

1. entity and Controller namespace - (recently replaced Loader class with Controller class since it's more flexible for adding the other three letters in CRUD later on). pwolanin suggested object, I think in PHP terms this is even more generic than entity
2. PHP optimizations when loading lots of individual objects (or the same one lots of times) on a page - IMO this is a followup patch since there's no serious issue here compared to the dozens of 'micro' issues I've found elsewhere, and wouldn't require API changes.
3. Whether defaulting hook_entity_info() to getting a controller class when it's not specified makes it harder for non-database entities to use it if we want to add more metadata here later on (like RDF).

I found two more comments not quite wrapping at 80 chars, I've added a lot more documentation since webchick's review in #109 although she wasn't able to provide specific feedback on what was missing yet.

Just a note, I have 7 days before I have to move 8 timezones away, so while I'll do my best to keep it current and respond to followups until then, it's going to be significantly harder for me to do so just before code freeze.

Status: Needs review » Needs work

The last submitted patch failed testing.

shp’s picture

1. Why not to simply turn today's entities (nodes, users, taxonomy, files, comments) into ONE ENTITY (D7-bundle)? Then this patch is not required.

2. Then to make possibile to place any D7-field-bundle into TREE - and we get powerful and flexible Drupal core.

fago’s picture

Status: Needs work » Needs review
FileSize
79.06 KB
Failed: Failed to apply patch. View

1. Why not to simply turn today's entities (nodes, users, taxonomy, files, comments) into ONE ENTITY (D7-bundle)? Then this patch is not required.

Hm I think conceptually there is a difference between a field and an enitity - fields can never stand alone. Also I doubt this would be so simple ;)

@fago: it defaults to providing a load class, but I think if you described an entity which didn't need that you'd not run into serious problems right? We fill in defaults for a lot of info hooks where those defaults might not necessarily be needed in all cases.

Yep, it think the default is fine as it's no big deal to set 'controller class' to FALSE. I just wanted to point out that this shouldn't be required for entities implementing the hook.

I rerolled the patch and noticed that the entity-info for files states the wrong base table files. Fixed that.

Status: Needs review » Needs work

The last submitted patch failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
79.52 KB
Failed: Failed to apply patch. View

Hm, I'm not used to working with stg yet. This patch really contains the 'file' fix and is hopefully in the right format.

pwolanin’s picture

why is there a new tracker.install file in the patch?

catch’s picture

FileSize
75.48 KB
Failed: Failed to apply patch. View

Removed.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
75.47 KB
Failed: 10303 passes, 834 fails, 1773 exceptions View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
75.48 KB
Failed: 12002 passes, 29 fails, 963 exceptions View

um.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
75.29 KB
Failed: Failed to apply patch. View

Trying again.

RobLoach’s picture

Subscribing, this is very very very awesome. This could potentially save us lots of standard database loading code, so I'm applying #smallcore. Are there any other places where we could apply the Entity API?

moshe weitzman’s picture

#smallcore is about having less features in core, not about less lines of code. lets keep the meaning of the tag as clear as possible.

RobLoach’s picture

Issue tags: +smallcode

Fair enough. How about making #smallcode for the same amount of features, but with less code? ;-)

RobLoach’s picture

Status: Needs review » Needs work

If you're on a fresh install, I'd consider this patch RTBC. Reasons why?

  • It's a great well documented API
  • Fits in with our current API (hook_node_load, hook_user_load, etc) so that everything doesn't have to be completely refactored
  • Will save us a lot of database loading
  • Supports loading multiple entities
  • Let's face it, that Taxonomy/Vocabulary code was getting some bit rot adn this patch really cleans it up
  • Did I mention the documentation?

If you're upgrading through update.php, however, it results in a WSOD, so I'm sticking this back to needs work. Any clue on how we could do the update if user_load kills it?

Great work! This patch is awesome.

catch’s picture

Status: Needs work » Needs review

HEAD-HEAD upgrades will fail due to the registry not knowing about the class before it needs to load it. So this needs testing on a D6-HEAD upgrade instead, which I haven't done.

mfer’s picture

subscribe.

catch’s picture

FileSize
75.94 KB
Failed: Failed to apply patch. View

RobLoach tested upgrade path on irc and it was broken. Part of this seems to be down to IP blocking in HEAD, part of it seems to be down to needing to include common.inc and entity.inc in update.php. To test this you'll need to uncomment $conf['blocked_ips'] in settings.php.

Frando’s picture

First of all: Thanks A LOT catch for your persistance in rerolling this patch! I've got really little Drupal time currently, so I wouldn't have been able to keep pushing this as pretty much every core change touching entity loading broke this.. I owe you more than one beer!

Then: It is awesome to see that this patch is getting attention. And it is also great to see that the substantial code of this patch is basically the same as in the initial patch I posted. That's great to see, as that means that the design is mostly agreed upon.

So, I just reviewed the latest patch in here, and the changes, apart from general cleanup, documentation improvements and keeping up with HEAD are mostly the naming changes, to which I fully agree.

I still think that, for performance, we should return directly from entity_load for already loaded ids, but that can also easily happen in a follow up patch. Same goes for integrating more meta data in hook_entity_info. So, of course I'm not able to RTBC this patch as I'm the main author, but I think we're really close and - let's get this in before the freeze.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

catch clarified tha the upgrad from a D6 site to HEAD is broken without this patch as well. So - unrelated.

I read through this patch and it looks good to my non-expert eyes. Since this also has the Frando/merlinofchaos seals of approval, I think we can proceed to RTBC.

catch’s picture

FileSize
77.38 KB
Failed: Failed to apply patch. View

Re-rolled again.

shp’s picture

Hm I think conceptually there is a difference between a field and an enitity - fields can never stand alone. Also I doubt this would be so simple ;)

No, I meant that maybee it's possible to completely remove entities (and bundles). Why we cannot leave only fields which can be bind into objects? Extremely simple and flexible structure...

Modules, if they need, can reduce flexibility at the UI-level. For example, module "node" can allow to attach only certain fields to objects, handled by him ("nodes"). But the core will be able to attach any field to any object.

What do you think about it?

yched’s picture

shp: sorry, but this is pipe dream at the moment, and won't happen in D7. So, not relevant for this thread.

Dries’s picture

Quick update: I'm currently at FroSCon making it hard to review this patch. Lots of people to talk to, lots of people that want to talk to me, a presentation, etc. This patch is at the top of my TODO list once I'm back home and can sit down and isolate myself from the world for a couple of hours.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review

Hmm, this was passing before, setting back again just in case.

catch’s picture

Status: Needs review » Reviewed & tested by the community
joshk’s picture

+1

No, make that +100.

This is so good on so many levels. Way to go Catch/Frando!

joshmiller’s picture

Issue tags: -smallcode

fixing tag ;)

catch’s picture

Issue tags: +smallcode

No this is really smallcode.

moshe weitzman’s picture

I propose that smallcode movement find a tag which isn't so easily confused with an existing movement. Or, stop using a tag at all ... Everyone wants as few lines of code as possible to achieve a given piece of functionality. Thats kinda basic software engineering.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
75.66 KB
Failed: 12339 passes, 4 fails, 3 exceptions View

Re-re-rolled.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review

Can't reproduce those fails locally, sending for retest.

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

I'm getting:

Message: FieldException: Attempt to create a field with invalid characters. Only lowercase alphanumeric characters and underscores are allowed, and only lowercase letters and underscore are allowed as the first character in field_create_field() (line 215 of /var/www/drupal/head/modules/field/field.crud.inc).

catch’s picture

Status: Needs work » Needs review
FileSize
77.66 KB
Failed: Failed to apply patch. View

Found it, field_test.module defines two entities, but only one of them was defined as fieldable.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

RobLoach’s picture

Status: Reviewed & tested by the community » Needs review

Great find, catch! Well done.

+++ modules/field/field.info.inc	24 Aug 2009 19:50:27 -0000
@@ -54,10 +54,11 @@ function _field_info_cache_clear() {
+ *     hook_entity_info()
  *   * module: module that exposes the entity type
+ * @TODO use entity_get_info().
  */
 function _field_info_collate_types($reset = FALSE) {
+++ includes/entity.inc	24 Aug 2009 19:50:23 -0000
@@ -0,0 +1,291 @@
+      // The id field is provided by entity, so remove it.
+      unset($entity_revision_fields[$this->idKey]);
+
+      // Change timestamp to revision_timestamp before adding it to the query.
+      // TODO: This is node specific and has to be moved into NodeController.
+      unset($entity_revision_fields['timestamp']);
+      $this->query->addField('revision', 'timestamp', 'revision_timestamp');

These will be provided by a follow-up patch, correct?

+++ modules/field/field.info.inc	24 Aug 2009 19:50:27 -0000
@@ -124,27 +125,29 @@ function _field_info_collate_types($rese
+            // If no bundle key provided, then we assume a single bundle, named
+            // after the type of the object. Make sure the bundle created
+            // has the human-readable name we need for bundle messages.
+            if (empty($entity_info['object keys']['bundle']) && empty($entity_info['bundles'])) {
+            $entity_info['bundles'] = array($name => array('label' => $entity_info['label']));
+            }
+            $info['fieldable types'][$name] = $entity_info;

Are we missing a double space here? ;-)

7 days to code freeze. Better review yourself.

catch’s picture

FileSize
77.67 KB
Failed: Failed to apply patch. View

Yep those two should be followup patches, but I fixed the spacing issue.

Status: Needs review » Needs work

The last submitted patch failed testing.

TheRec’s picture

Status: Needs work » Needs review

catch's concern about #492186: Authoring information is never updated. (see comment #49 there) should be addressed, the other patch has been committed.

TheRec’s picture

Status: Needs review » Needs work
catch’s picture

Status: Needs work » Needs review
FileSize
78.19 KB
Failed: Failed to apply patch. View

Re-rolled, for now fixing this in entity.inc since I'm about to go offline for about 48 hours, would ideally be done in NodeController::buildQuery() if someone has time for that but shouldn't require an API change to clean up later. I'll owe you beer if you re-roll this next time it breaks (once a day at the moment).

Status: Needs review » Needs work

The last submitted patch failed testing.

TheRec’s picture

Status: Needs work » Needs review
FileSize
77.36 KB
Unable to apply patch 460320-entity_24.patch View

Re-roll, now that blogapi is gone from core.
(I am only helping catch to maintain the patch for the moment... with code freeze coming up and him being away two days, this might help :) I did not test/revire anything)

Dries’s picture

Status: Needs review » Fixed

Sorry it took a bit longer but I wanted to be able to sit down and carefully go through this patch. I've just done that, and committed it to CVS HEAD. So yes, committed! Committed. Committed. :)

TheRec’s picture

Status: Fixed » Needs review

Great ! So my monkey job helped then ;) catch will be happy ! Thank you Dries.

yched’s picture

Great ! Although: we didn't let the tests run on #180 before committing ?

TheRec’s picture

yched: Well, phew... they passed http://testing.drupal.org/pifr/file/1/460320-entity_24.patch :) I guess this commit will need some API changes documentation at some point ?

webchick’s picture

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

Oh! Awesome!! :D Thanks a lot, Dries!

Marking "needs work" for documentation.

Frando’s picture

Woww! Thanks Dries! Very cool.

yched’s picture

Shameless plug: Field API needs a way to build the path to an arbitrary 'entity'. #525622: 'as link' formatters need a generic way to build the url of an 'entity' would love some feedback from the fine folks in this thread ;-)

fago’s picture

A big step towards a generic data API, yeah, awesome!! Thanks all!

Now let's add more metadata to hook_entity_info()! See #551406: Centralize entity properties meta data for centralizing entity property metadata, feedback wanted!

catch’s picture

Oooh lovely.

yched’s picture

Should we report issues on the 'base system' component, or is a new project component needed ?

plach’s picture

catch’s picture

Priority: Critical » Normal
Issue tags: -Performance, -Views in Core, -Fields in Core, -Needs committer feedback, -smallcode
jhodgdon’s picture

changing tagging scheme for update guides

jhodgdon’s picture

Status: Needs work » Postponed (maintainer needs more info)

This issue has 193 comments, and the last patch is quite large...

Can someone please provide a summary of what needs to be documented in the 6/7 module update guide (i.e. what would affect the developer of a Drupal 6 module updating to Drupal 7)? Or a themer?

And is there anything else that needs to be documented elsewhere perhaps, such as in the Working with Drupal APIs section of the d.o docs (http://drupal.org/developing/api)?

pwolanin’s picture

Status: Postponed (maintainer needs more info) » Closed (fixed)

This looks to be committed/fixed.