[RFC|Merged] General Hook Architecture of phpBB3.1

[Jhong]

yes, integrating with other packages. The first hook is not as important as the last hook, but both are nice to have.

For the early hook, it is nice to be able to set defines to override error reporting settings, etc. as they might not be compatible with other packages.

If the hook system is DB driven, fine -- but perhaps allow to auto-include any files in the hooks folder very early, before the hook system is initialised, so we can at least run some procedural code early.

[Nelsaidi]

Whats the latest on hooks in 3.1?

Given the OOPless format of phpBB hooks should be documented (typically you'd just pass $this to the callback) - And there should be a good bundle of hooks/event triggers in 3.1 to cover the absolute basics at the very least:

user sign up; user login; page render; warning issue; new topic; new post; post delete;

Are all things I'd like to see, whilst I understand area 51 is to discuss the needs of functionality forthe general public, I do feel these being the very basics could be used by many (ie, blog integration, membership integration).

[igorw]

Given the OOPless format of phpBB hooks should be documented (typically you'd just pass $this to the callback) - And there should be a good bundle of hooks/event triggers in 3.1 to cover the absolute basics at the very least:

Re-read the first post. The new hooks will use OOP.

[EXreaction]

In addition to the proposed design in the first post, but this goes somewhat against the current design implementation in the development branch for it.

Modify the existing 3.0 hooks system to fit our needs and wants.

Notes:

• Hook means a single point in the code where additional code can be run.
• Plugin means a package which may use one or more hooks to modify the phpBB system without editing files.

Change the hook class to use static methods

• No more declaring global $phpbb_hook in order to use it

Remove the requirement of sending the list (array) of valid hooks to the hook class

• It's simply not reasonable to have a list of valid hooks for 3.1, as there will be too many hooks.
• Adding hooks becomes a real PITA with requiring the declaration of valid hooks before allowing the new hook to be called
• Any invalid registers by plugins (registering a hook that does not exist) is not a problem (it'll just never be called)

Simplify the use model for hooks, three common needs are satisfied by hooks:

1. Taking over a function completely to replace it with your own (should only rarely occur)
2. Modifying data (input and output)
3. Outputing additional data to the template

Two main styles of hooks can accomplish that:

• Taking over a function; this looks similar, though simplified, to what we use already:
  

  Code: Select all

        // If boolean false is returned, that means something has taken over the function
      if (phpbb_hook::call_hook(__FUNCTION__, $url, $params, $is_amp, $session_id) === false)
      {
         return;
      }

  
  
• Modifying data could look like this:
  

  Code: Select all

        // Here we can modify some variables
      extract(phpbb_hook:call_hook(array('posting', 'submit'), compact('sql_data', 'poll_data', ...)));

      // Or a simpler version for sending a single variable for modification:
      extract(phpbb_hook:call_hook(array('posting', 'test'), $test_variable));

  
  
• Outputting additional data to the template (or for other reasons) in key areas without the need for data modification:
  

  Code: Select all

        // This is easy
      phpbb_hook::call_hook(array('foo', 'bar'));

Expanding hooks into templates:

• Option one: simply add extra template variables for plugins to use to the template
  This is a bit ugly and requires a bunch of unique template variable names.
  Probably requires that we create our own output function in the hooks class to prevent overwritting of others and enforce it's use
• Option two: Add a new kind of template variable for hooks
  It could look like:
  {HOOK posting, userdata}
  This is not as ugly, does not require long unique variable names, and is not prone to overwriting
  Requires we create our own output function in the hooks class to output what the plugin wants

Allowing templates to act like plugins too only for when that template is loaded:

• When a template is used, a check for a plugins.php file would be made by the hooks system.
  This greatly increases the flexibility for tweaking some data sent to the user, allowing styles to become even more customised.
• A warning during installation of a style if the plugins file exists and a global on/off for template plugins (like the PHP include)

Plugins (packages):

• The current method of just dropping a plugin file in a directory and it automatically starting up does not fit the user's needs
  Users should be able to install, enable, disable, and uninstall plugins
  
  • Install:
    The installation method is what is required first, it allows plugins to easily handle any DB alterations required before installation
    
  • Enable/Disable:
    Enables or disables the plugin without uninstalling (losing) what is stored in the database.
    
  • Uninstall:
    Removes any changes made to the database as well (the plugin handles it through an uninstall method)
• A new ACP page is created to list installed/uninstalled hooks and manage them.
  
• A database table holds the list of currently installed plugins which are to be loaded at runtime.
  
• At runtime the plugin files are included and register the locations that they need.
  
• There is a slight problem that could come up here however. In the instance where a change to an existing DB table is made and a plugin is disabled.
  
  • This may cause SQL errors in some cases.
  • In order to prevent this we can do two things:
  • Plugins can specify that they are not able to be disabled (requires uninstallation to disable)
  • This is the easiest method, but data will be lost for temporary disables
  • Plugins can specify some specific items to be called if they are installed but disabled
  • A bit more difficult, but data will be saved without SQL errors

If something here isn't clear just ask, I wrote this up on little sleep after a long day.

[igorw]

Change the hook class to use static methods

This is a bad idea. Static methods lead to monolithic design and make testing hard. Plus you don't really get much benefit, except not having to global your instance.

Are you suggesting to re-start the implementation of the hooks system based on the existing hooks in 3.0?

[igorw]

There's two things I'd like to suggest this system to support.

Namespacing

Some MODs may want to provide their own hook locations. To avoid naming clashes it would be great to be able to namespace these. Something simple like:

Code: Select all

$hook_handler->add_hook('blog.add_post', $callable);

The core hooks don't need a 'core' namespace, they can just use the global namespace instead.

[naderman]

I'd say that your "dynamic proxies" are really quite independent of what this topic is discussing. Using __call is quite a performance hit so you wouldn't normally want to do that for methods called tons of times on every request. It's also something a MOD could easily do by itself, we could still provide such a proxy but it's really little code. The concept also doesn't fit into the automatically registered hooks concept.

As for the namespaces I agree that that is a good idea but don't really see any sensible way to enforce it, so it'll be up to MOD authors to do that. Could be mentioned in documentation.

[A_Jelly_Doughnut]

As for the namespaces I agree that that is a good idea but don't really see any sensible way to enforce it, so it'll be up to MOD authors to do that. Could be mentioned in documentation.

No way to enforce in MODs, but we could come up with examples in the core code where namespacing might help too

[igorw]

Yes, this should definitely be highlighted in the hook docs.

[James78]

Incompatible: An array of other MODs known to be incompatible with this one

Also needs version specification.

Maybe version specification can use wildcards: 1.* means 1.0 <= x < 2.0. Then all boundaries can be made inclusive.

Perhaps it is obvious, but all hooks should go through "requesting hooks" process, in particular explaining the use case for the hook, incuding the core hooks listed.

I don't know if this changed or not yet, or if my input is even worth anything as I'm not a development team member, but I would like to add here, on the topic of version specification.. If you're going to have a feature to allow wildcards for the versions, you might as well just allow the authors to use a regex; that way specifying the version(s) are truly in the hands of the MOD author. I would much rather be able to (possibly) have many options regarding this. E.g., you can use a simple string '3.0.8', a range '3.0.6-3.0.8' *, or a regex, which is ultimately so much more power in the hands of the modder (if they ever need it).

* Not sure how easy or feasible that actually is, as version strings may not always end up matching what you want, e.g. What if my version string is '0.9-RC-PL1'? How then can a range work on that really?

Edit: Arggg.. My mistake. I'm pretty sure the general hook architecture has already been finalized..