This patch fixes a PHP5 call time pass-by-reference error
Senpai - June 15, 2008 - 04:04
| Project: | Apache Solr Search Integration |
| Version: | 5.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
Line 71 of the apachesolr_search.module throws a PHP call time pass-by-reference error.
<?php
70 foreach ($response->documents as $doc) {
71 $extra = node_invoke_nodeapi(&$doc, 'search result');
?>The nodeapi documentation mentions an implied pass-by-reference in the api structure's parameter one, so I don't think this is needed in line 71, and removing it fixes the error.
Patch attached.
#1
Ok, maybe this time the patch will stick.
V (look below, and tell me if it's there?) V
#2
Take 2. There's another pass by reference error going on with the module_invoke_all() in apachesolr.module, and if I read things right, the intent is to allow other modules to alter the $document variable, not alter a copy of it.
This patch addresses both of these PHP5 warnings with each module file getting a one line change.
#3
Disregard the patch in #2 for now. I didn't think it through enough, and it causes cron to fatal error at apachesolr.module line 299 "Call to a member function setMultiValue() on a non-object".
Doh!
#4
Commit #2. Thanks.
#5
@Robert: Did you change/fix my #2 patch before committing it? I was trying to say that the patch I submitted in comment #2 causes a fatal error in certain cases. #2 is no good.
#6
No, I committed #1, which tested out ok. The issue in #2 will have to be looked at separately.
#7
The attached patch should repair the second error.
I changed it a little compared to the one I posted to the duplicate issue, this version of the patch should work in any case, I think. I just remembered that PHP allows calling a variable function by simply writing the variable and the arguments. That way, only the definition of the function is important and if its defined with the first parameter being passed by reference (as it should, if it intends to change the object), all's well.
I don't really know, whether the version with call_user_func_array() would have worked too.
Anyways, please review!
#8
#7 patch worked for me. Thanks.
#9
If febbraro and drunkenmonkey both sign off on #7, please commit to both branches.
#10
I don't see how #7 fixes the call-time pass-by-reference. It still uses &$ in the replacement foreach().
I'm curious... given the way PHP handles objects, do we need to make this a reference at all. Has it been tested w/ just passing $document? It's kind of funky when PHP passes references or not w/ objects, which is why I'm a little unsure... I just know I've been burned by it passing an object by reference when I thought it was by value. : ?
#11
Indeed, Robert is correct. It is the implementers who must receive the request by reference, that is what the ticket is about in the first place.
I've done 3 things (sorry, one may not be the task in this issue, but it made sense):
1. Moved the hook invocation to the bottom of the Document building code. Why? Because it is a lot more useful. Modules will sometimes want to change things provided by the node based additions in apachesolr.module, and this allows them to do so. Before, they could only add items, and what they added could easily have gotten overridden by the node, content and taxonomy based manipulations happening later on. In the future, I'd like to see this refactored, so there are different indexing modules which can be switched on and off which add stuff to the document. So apachesolr_cck and apachesolr_taxonomy which would implement this hook. That seems like a more flexible architechture. At any rate, I don't see any objection to moving the function
2. I fixed the patch above and simply removed the & before the $document. Since no modules are even using this hook now, I don't think this will cause any issues
3. I added documentation to the readme for module maintainers on how to use the hook.
#12
Fixed in D5 and D6. Thanks!
#13
Automatically closed -- issue fixed for two weeks with no activity.