recursed entity loading doesn't work [#731426]

I just ran into a weird bug with entity_load() returning the wrong result with the controller of the entity API. In my case the entity object creation triggered a rules cache rebuilt, which also needs to fetch some entities. Thus the controllers properties related to load() call were overwritten and the initial load() returned wrong results.

While I agree with catch that entity load calls triggering other load calls is something which one better avoids, it will still happen sometimes in practice as my case shows. Thus the suggested fix is to make the controller more robust, so recursed loads don't cause bugs. For that we may not set any load request related properties in the controller object, instead we just pass around the variables which was most times already the case anyway.

Attached is a patch that fixes the controller to do so. I've looked over all core controllers and replaced $this->ids with $ids and so on. While going through the code I also fixed a wrong comment I noted and I removed some dead code I found in the taxonomy controllers.

Comment	File	Size	Author
#6	fix_comment.patch	655 bytes	fago
#6
	drupal_controller_fix.patch	15.13 KB	fago

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Frando CreditAttribution: Frando commented 3 March 2010 at 15:58

Yes, I thought about the same recently. We shouldn't set load-specific properties directly in the controller. Making the pass around explicit also makes it much clearer what e.g. buildQuery needs.

These two hunks in the taxonomy controllers - are you sure they're dead? I haven't actually checked the code and have no development environment at hand right now, so just wanted to make sure. Otherwise, RTBC from me after tests pass.

Comment #2

fago

German

Vienna

CreditAttribution: fago commented 3 March 2010 at 16:43

One hunk added a 'type' property to the taxonomy controller, which was nowhere used (there is no type at all). The other hunk from the vocabulary controller is related to a join on the node table, which isn't there any more.

Comment #3

catch

he/him

English

CreditAttribution: catch commented 5 March 2010 at 12:12

Status:

Needs review

» Reviewed & tested by the community

Patch looks good to me, I found some dead code in #729968: Dead code in TaxonomyVocabularyController but looks like fago found more. This conflicts with #566940: Move node specific code out of entity.inc I think but that's an easier re-roll, let's get this in.

Comment #4

Dries CreditAttribution: Dries commented 5 March 2010 at 13:41

Status:

Reviewed & tested by the community

» Fixed

Committed to CVS HEAD. Thanks.

Comment #5

neclimdul

He/Him

CreditAttribution: neclimdul commented 9 March 2010 at 16:50

Status:

Fixed

» Needs work

for what its worth, this breaks the function documentation.

   * This has full revision support. For entities requiring special queries,
   * the class can be extended, and the default query can be constructed by
   * calling parent::buildQuery(). This is usually necessary when the object

The function can't be called without arguments. The function signature requires the first argument, $ids, be passed to the function
The buildQuery now returns the query object instead of setting the local query object. If not explicitly mentioned in the comments, we should probably at least have a @return

Comment #6

fago

German

Vienna

CreditAttribution: fago commented 10 March 2010 at 09:12

Status:

Needs work

» Needs review

File	Size
fix_comment.patch	655 bytes

I'd not say that parent::buildQuery() means you have to call the function without arguments - parent::buildQuery() just tells you what to call. That you have to pass the arguments when you call the parent method is normal, so I don't think we have to document that somehow.

But yes I'd be good to have a @return, thus here is a patch that adds one.