plugin system is flawed

Bug #1062748 reported by Uranium235
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Pluck CMS
Fix Committed
Medium
Unassigned

Bug Description

The plugin system, specifically the run_hook() function, is flawed.

First, it should be clarified in the functions description, that the parameter "par" is an array of parameters, not just an array of strings and that their meaning and index depend on the specific hook. It should also be made clear, that the parameters are passed by reference and that the return value is irrelevant, at least in effect.
And that's where the actual problem is, imho. If a function called via the hook returns a value, the processing of other possible hooks stops.

It is a design decision, either the return value of a hook determines if the "event" was handled finally, or it has some meaning and processing should not stop.

From the way the run_hook() is implemented , I suspect the return values of the individual hooks should have been collected in an array without stopping the process and returned after all hooks have been called.

But the return values of run_hook() are never used anywhere so far. Especially with the "theme_content" hook, there is unnecessary memory usage involved with the current method. The seo module for example returns the $content as well as altering it via the parameter, thus having two copies of the content in memory, at least until it is clear that the return value is not needed.

Also, I think plugin hooks should indicate the by reference passing in their declaration:
e.g. seo_theme_content(&$content) instead of seo_theme_content($content)
The current way is sort of a "work around" for the deprecated runtime calling by reference. Maybe it's just a matter of time until this too issues a warning.

So I propose to have the hooks' return value ignored and adjust all current hook function's declarations to indicate the passing by reference.
If the return value was to determine the stopping of processing, modules should have a way to prioritize themselves. But I think that is not needed here.

Related branches

Revision history for this message
Uranium235 (uranium235) wrote :
Changed in pluck-cms:
status: New → Fix Committed
Changed in pluck-cms:
importance: Undecided → Medium
Revision history for this message
Uranium235 (uranium235) wrote :

Oh boy, that fix of mine turned out to be a little over the top.
Problem is, i.e., that when the seo module is enabled, the url prefix should get rewritten. As I removed the require_once() statement from run_hook(), the seo module isn't loaded at the time the hook is run.
Will fix that...

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.