PHP implicitly references objects when they are passed to functions so there is no need to explicitly mark them as references. Attached patch fixes this.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3055775-6--remove_reference_from_query_alter_hooks.patch | 2.63 KB | drunken monkey |
Comments
Comment #2
legolasboComment #3
borisson_Patch doesn't apply.
Comment #4
legolasbo:facepalm: based the previous patch on a stale branch :). This should be better
Comment #5
borisson_yep :)
Comment #6
drunken monkeyThanks for creating this issue (and not discussing it further in the other one). However, I largely disagree:
&. Consider this hook body:$results = new ResultSet($query);. This is now possible, e.g., to override the results in case there are no hits – changing that would require passing an explicit copy of the variable when invoking the hook, and would be a BC break.alter()method always passes by reference, so hook implementations are always allowed to have the parameter as a reference, or not, as they choose. But what is possible should be reflected in the documentation.On the other hand, this already doesn’t work for the query alter hooks, for instance – as those are called with
$this, which of course can’t be passed as a complete reference. (I tested it: assigning the reference to a new object is just silently ignored in this case.)So, maybe removing it from those hook documentations does make sense. Patch attached, please tell me what you think.
Comment #7
legolasboI agree that being able to replace a passed in object as mentioned in 1. can be very useful in case there are no results, but it is also potentially destructive depending on when the hook is invoked, because it can potentially replace the object that has already been altered by another implementation. I'm aware that this is also the case with pretty much every other hook, so I guess that's sort-of-ok as is, but still is a design flaw in my opinion.
I guess any improvement is better than no improvement, so let's just get this in and see what the future brings for hooks. Hopefully we can get rid of this outdated pattern in future versions of Drupal.
Comment #9
drunken monkeyI think we discussed this in the events issue already as well: we can’t, and shouldn’t, keep people from doing something stupid. I think with every single alter hook in Drupal you can break things. Making them less powerful to make it harder to break things is not the way to go, at least in my opinion.
Anyways, committed this. Thanks again for the suggestion, and the feedback!