Feature #1844
Merge the run_hooks_by_ref() and run_hooks() functions of the plugin system.
| Status: | Closed | Start date: | 12/11/2011 | |
|---|---|---|---|---|
| Priority: | Normal | Due date: | 12/14/2011 | |
| Assignee: | Dylan Myers | % Done: | 100% |
|
| Category: | Plugin System | |||
| Target version: | 1.6.7 | Estimated time: | 0.50 hour |
Description
This wont require any changes to plugins beyond 1.6.5's update. However, it will streamline the code quite a bit.
Essentially what this means is that I'm going to change it so run_hooks() can either accept values received by ref in the plugin, or accept return values. This ensures compatibility with existing plugins, and looks towards the future. Really I should have done the 1.6.5 update this way, however I didn't realize it at the time.
Screenshot.png (26.5 kB)
test.php - Testing wrapper function
(407 Bytes)
Associated revisions
Fixes Merge the run_hooks_by_ref() and run_hooks() functions of the plugin system. (fixes #1844)
Fixes Merge the run_hooks_by_ref() and run_hooks() functions of the plugin system. (fixes #1844)
Feature Merge the run_hooks_by_ref() and run_hooks() functions of the plugin system. (completed #1844)
History
#1 Updated by Dylan Myers 6 months ago
- Status changed from Assigned to Resolved
- % Done changed from 0 to 100
Applied in changeset r5730.
#2 Updated by Stefan T. 5 months ago
- Status changed from Resolved to Feedback
Fatal error: Cannot pass parameter 2 by reference in /inc/functions.php on line 2947
#3 Updated by Dylan Myers 5 months ago
- Status changed from Feedback to Resolved
Applied in changeset r5731.
#4 Updated by Nathan Malcolm 5 months ago
- Status changed from Resolved to Closed
#5 Updated by Dylan Myers 3 months ago
- Status changed from Closed to Feedback
Should have retained the run_hooks_by_ref() function as a wrapper to run_hooks()!
#6 Updated by Dylan Myers 3 months ago
- Status changed from Feedback to Resolved
Applied in changeset r5747.
#7 Updated by Nathan Malcolm 3 months ago
- Status changed from Resolved to Feedback
The new wrapper isn't functioning correctly. I'm not sure why though.
#9 Updated by Nathan Malcolm 2 months ago
- File Screenshot.png added
I've just noticed something. Plugins such as NewPoints use run_hooks() without arguments (E.g. $plugins->run_hooks_by_ref("newpoints_admin_upgrades_begin")), so it returns a PHP warning.
Warning [2] Missing argument 2 for pluginSystem::run_hooks_by_ref()
The function should be as follows:
function run_hooks_by_ref($hook, &$arguments="")
{
$this->run_hooks($hook, $arguments);
}
But the wrapper still doesn't work as it should.
See the attached screenshot.
#10 Updated by Chris Köcher 2 months ago
- File test.php added
For me, the wrapper function works as aspected with PHP 5.3.6-13ubuntu3.6.
In the attached file I've tested run_hooks_by_ref and it works as it should (it outputs "foobar" and not "foofoo").
#11 Updated by Dylan Myers 2 months ago
Nathan Malcolm wrote:
I've just noticed something. Plugins such as NewPoints use run_hooks() without arguments (E.g. $plugins->run_hooks_by_ref("newpoints_admin_upgrades_begin")), so it returns a PHP warning.
[...]
The function should be as follows:
[...]
But the wrapper still doesn't work as it should.
See the attached screenshot.
By definition, that is incorrect since the "by_ref" portion refers to the variable being passed (or now received) by reference. He should not be using run_hooks_by_ref() unless he is passing a variable. Thats just bad plugin code. For that he should be using just run_hooks().
When used correctly, the code works as expected. :)
However... I suppose we should put the other in there to save people from themselves. Or should we not? Actually I'm for leaving this as it is, and having people fix their plugins instead.
#12 Updated by Nathan Malcolm 2 months ago
- Status changed from Feedback to Closed
The wrapper wasn't working due to an issue on my end. Testing it locally instead of on my buggy, half assed excuse for a PHP build, it's working correctly.