| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal_controller_fix.patch | 15.13 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 18,219 pass(es). | View details |
Comments
#1
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.
#2
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.
#3
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.
#4
Committed to CVS HEAD. Thanks.
#5
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
#6
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.
#7
#8
Committed to CVS HEAD. Thanks.
#9
Automatically closed -- issue fixed for 2 weeks with no activity.