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

[Oleg]

No, it is because the point of modules is to be independent from changes to the core.

the general assumption is that all MODs are at their newest version.

I don't know how else I can convey this to you, but what should be and what is are two different things. Core will change and break modifications. People will run old versions for a nontrivial amount of time. People do update their modified boards in pieces. This is the reality that you are ignoring. I can only hope that someone else is more persuasive.

No, because it is pointless trying to come up with a list of all incompatibilities and side effects.

This is why conflicts are made optional, just like version number ranges and dependencies. If a mod author does not want to use them, they don't have to. This functionality exists for mod authors who want to support easy upgrading and who are concerned with compatibility.

Interesting, where do you see the mismatch? Care to expand on the subject? Feedback is the sole reason for this topic being here and if you see a pressing problem, please share.

You simply need to scroll up to my first post in this thread and try to understand where I'm coming from. Everything I posted was a result of experience using and modifying phpbb in particular as well as other software. I didn't make that stuff up just now.

Maybe you think you are obligated to implement everything anyone proposes. This is not true. There is a difference between saying "I don't need feature X, and I won't code it, but if you want it and you implement it, I will accept it" and "feature X is useless and will not be included regardless of how you feel about it". The former is perfectly acceptable, the latter is, well, read the paragraph about upstream in this post.

This is more about the design of particular hooks than the system; please post RFCs for the hooks once we are in that phase.

No software should be built in isolation from its users. Users of hook system are hooks. To build a hook system and then consider how to implement hooks for that system (which is what I think you mean by "once we are in that phase", please correct me if I'm wrong) is absurd.

There is no conflict with scalability at all, quite the contrary. The "it just works" factor is extremely important for hook systems, if they are to be adopted - I won't introduce any more descriptors than absolutely required.

"It just works" and scalability are not contrary, in fact, they have nothing to do with each other. Putting unnecessary code into critical path is not good because you increase the working set size, you make the code harder to audit, you make the code harder to optimize because it does irrelevant operations, etc.

The creation of a hook is nothing more than the insertion of one row into a table in case of an key not being defined - I don't see any scalability issues at all.

There is a more important issue here, which is that on-demand creation of hooks is not going to work at all.

If there are two mods, A and B, and A creates a hook which B uses, and A and B are installed together (that is, user installs A and immediately after installs B, perhaps because they really want B and A is B's prerequisite), then B's installation will attempt to resolve hook from A which does not exist because A's code was never run. Allowing linking to nonexistent hooks will make troubleshooting legitimate misspellings of references and missing dependency specifications unnecessarily hard, and running A's code so it can register its hooks is an epic kludge compared to simply asking A for a list of hooks it defines.

[Kellanved]

I don't know how else I can convey this to you, but what should be and what is are two different things. Core will change and break modifications. People will run old versions for a nontrivial amount of time. People do update their modified boards in pieces. This is the reality that you are ignoring. I can only hope that someone else is more persuasive.

The attempt to fix this by specification is guaranteed to fail. Incompatibilities only get discovered after deployment; it's easier to just resolve the issue than to push out a new release amended to include some version information. Moreover, installed versions change, a check run on install might and will cause trouble later on (speaking with quite a lot of experience in this regard here).

You simply need to scroll up to my first post in this thread and try to understand where I'm coming from. Everything I posted was a result of experience using and modifying phpbb in particular as well as other software. I didn't make that stuff up just now.

Maybe you think you are obligated to implement everything anyone proposes. This is not true. There is a difference between saying "I don't need feature X, and I won't code it, but if you want it and you implement it, I will accept it" and "feature X is useless and will not be included regardless of how you feel about it". The former is perfectly acceptable, the latter is, well, read the paragraph about upstream in this post.

Frankly, you haven't provided anything backing that up. You are mixing different areas of the software and different issues. The resolution of conflicts, dependencies and similar things will be the subject of the MOD installer, which as of now does not have its own RFC. Please feel free to contribute one, detailing the algorithms for conflict discovery etc (possibly even a patch ) .
As to the topic at hand, the system that manages and invokes hooks, I fail to see any issue.

No software should be built in isolation from its users. Users of hook system are hooks. To build a hook system and then consider how to implement hooks for that system (which is what I think you mean by "once we are in that phase", please correct me if I'm wrong) is absurd.

This topic is asking for feedback for the system. I asked for any issues you see before and can just repeat that: where do you see an issue?
The matter is: the system as proposed will manage and invoke almost any conceivable hook just fine, the important question is where to place the hook invocations. We are explicitly asking for proposals on where to place which invocation calls and expect an active discussion about this topic.

"It just works" and scalability are not contrary, in fact, they have nothing to do with each other. Putting unnecessary code into critical path is not good because you increase the working set size, you make the code harder to audit, you make the code harder to optimize because it does irrelevant operations, etc.

This is starting to be absurd. There are bottlenecks and scalability issues in a lot of places of the code, the same is true for hard to read, never mind understand sections. The hook manager as proposed (and by now mostly implemented) is not likely to become one of them. Hook invocations are to be rather common and will draw from a populated cache of which objects have methods to be called for a given hook. In case of a hookname that is not defined, the call of an additional function to add that single new entry is entirely trivial - in terms of critical path, performance, scalability, complexity - you name it. In case that it were not, it would be moved into a shutdown function or cron job. I really fail to see any issue here.

There is a more important issue here, which is that on-demand creation of hooks is not going to work at all.

If there are two mods, A and B, and A creates a hook which B uses, and A and B are installed together (that is, user installs A and immediately after installs B, perhaps because they really want B and A is B's prerequisite), then B's installation will attempt to resolve hook from A which does not exist because A's code was never run. Allowing linking to nonexistent hooks will make troubleshooting legitimate misspellings of references and missing dependency specifications unnecessarily hard, and running A's code so it can register its hooks is an epic kludge compared to simply asking A for a list of hooks it defines.

This is a wonderful example why auto-discovery is a viable way to go. Mutual dependencies would be next to impossible to resolve with a strict check on install. The system we are proposing will deal with this nicely:

• Hooks are not resolved, they are called. A hook invocation is a undirected method call, no resolution happens. The call thus would yield the correct empty result.
• However, the invocation of the - thus far undefined - hook will create all the internal bookkeeping for the hook
• The hook is now registered and the other MOD will work as intended (might take a cache purge, but otherwise it'll work)

The major misunderstanding here seems to be the nature of hooks: hooks are anonymous calls; the invoking module does not know who it is calling and what results it will receive (beyond the hook specification). Moreover, a given hook might be invoked by more than just one MOD and in more than just one place. In either case, there is nothing that would stop a MOD from setting up its hooks in the install method. There is a public method for registering hooks explicitly, auto-discovery is entirely optional - and will be removed, should it cause any problems.

On Edit:
To expand on priorities versus before/after

You have to consider the information known and the information unknown, as well as the typical use of priorities.
Typically, the execution order is irrelevant, so most modules won't set a priority at all. If the execution order is important, there are typically two scenarios:
a) one MOD needs data set by another. In that case, the other MOD's priority is known and can be set accordingly. It might be an idea to have logic for that in the installer, but that's another topic.
b) a MOD just has to run early/late in the game to set data or remove it from an array, for instance for finer permissions etc
Just setting a very high/very low priority is usually sufficient

Of course, priorities are not a perfect solution, but they are straightforward and have the benefit of working, even if the other MODs affecting the same hook are not known.

[Kellanved]

There's now an early version on github:
http://github.com/kellanved/phpbb3/tree/feature/9550

[DavidIQ]

I think changing the hooks in phpBB is an excellent idea. Coming from the MOD Team's perspective though, and as a MOD writer myself, creating hooks is not really straight forward for 3.0.x and a lot of MOD authors don't take them into consideration because they've just not educated themselves enough and, at first glance, they're pretty daunting. Take for instance this hook. The bottom part of that hook shows another hook created by ToonArmy which I forked into my own. You can see the check there for "$template->_rootref['MESSAGE_TEXT']" and "$template->_rootref['META']" which are not necessarily an easy thing to come up with. I am glad that part of the first post here shows some template injection proposal so this might be easier to come by/implement for 3.1. You'll need to have something in place to take into consideration different styles though as they may or may not be different from the base one.

On a related note: the biggest reason for hooks not being used as widely as they should be in MODs is because there are lots of MODs that don't just add functionallity. They change core functionallity. In a lot of those cases hooks wouldn't be usable especially if they're overriding large chunks of code. Plus the fact that most functionallity is generally added to a specific part of the page routine also sort of eliminates the usability of hooks. This would certainly deal with that:

MOD_page_<x>: where <x> is the current phpBB page (index, viewtopic, viewforum, …). Called several times during each page, with a parameter stating the current position in the page.

Seeing exactly how this is implemented will be very interesting.

Having good documentation for this new hook architecture being proposed for 3.1, some examples of implementation in real MODs, and some advertising of it in the MOD forums would allow for greater adoption by the MOD writers community. I feel like this was not adequate enough...

[Kellanved]

The actual hooks will be defined in their own RFC phase, the list is more an example than an actual compilation. The template-part can be found here: viewtopic.php?f=84&t=32796
I agree that there should be some style-dependent override; a strict solution would be to just return the name of a template file to be included in the hook.

[igorw]

The proposal looks good overall. A few minor things. This is without doubt the killer feature for MOD authors in phpBB 3.1.

Additonally, a info file with the following information is required:
s an associative array with information about the MOD. Required keys:

• Description : a String describing the MOD
• Target core version : version of phpBB this MOD is intended for
• Version: Version of the MOD
• Author : a string listing the author name
• Installation: a flag describing whether this is an editless MOD or not.
• Requires: An array of other MODs required by this one.
• Plugins: an associative array listing all plugins (captcha/search/db/*cp modules/…) the MOD provides

I would prefer this stuff to be covered by MODX. Since they have to provide this information to the MODX file anyway (right?) it would be a pain for them to put it into the PHP file as well. See the modx2 splitting suggestions, they make the modx meta-data a lot less obtrusive. If the ACP needs to get this data the MODX info file could be uploaded to a specific folder, preferably the same as AutoMOD will use.

Debug: Unless DEBUG is defined, phpBB will disable any MODs that are triggering errors. MODs are not to use trigger error, but have to call the phpBB_message method instead. This behavior can be overridden.

How about exceptions?

[Kellanved]

Debug: Unless DEBUG is defined, phpBB will disable any MODs that are triggering errors. MODs are not to use trigger error, but have to call the phpBB_message method instead. This behavior can be overridden.

How about exceptions?[/quote]

The problem is that trigger error - just like an exception - would interrupt the control flow right there. MODs later in the chain wouldn't get called, which might lead to unexpected behavior. It's cleaner to let all MODs do their thing and then handle the errors at the invocation place.

MODX and install are another option - we'll have to put an RFC for the MOD installer together soon.

[igorw]

Ok, good point. But you could also catch them right away. The custom error function could even throw an exception. Implementation detail, but imo cleaner than using some global scope.

[Jhong]

The first problem I have with the current hook system is that whether the core function is subsequently executed or not seems to be controlled by the return value of the hooked-in user function. i.e. returning an empty string vs returning null seem to do different things (it's been a while since I worked on it so forgive me if I am wrong).

This is rather opaque and doesn't make for nice code (and also causes my uncertainty in the above statement... it is not something I will ever find natural).

Secondly, I understand this will come in a later RFC, but it is imperative that the very outer shell of phpBB can be hooked. That is, the very last stage of trigger_error, and the final ob_gzhandler. I'd also like to see a hook very early in common.php -- or, even better -- in the include statement in every call to common.php (so error settings, etc. can be overridden). Even though you'll be eliciting these requests later, this will undoubtedly influence the overall architecture -- the hook system will need to be loaded before anything else, such as initialising the database. This is all important to embed phpBB successfully without core code changes.

Finally, when hooking into class methods, I need to be able to know what object is being hooked. In 3.0, I have trouble, for example, with the ('template', 'display') hook. The object is not always $template -- it can be other things that make use of the Template class. As a result I can't be sure that I'm intercepting the right object without getting hacky. On this point, there are otehr basic architectural oversights as well. For example, $template->assign_display calls $template->display, and the only way I can tell which was which is by examining the number of levels in the output buffer -- very hacky indeed.

In general, this is very painful -- and I think the way to improve the situation for v3.1 is for you to lay down strict coding guidelines for the Ascraeus dev team. That is, you should review all function calls to ensure that you are not re-using hookable functions for multiple purposes.

Since -- in the absence of a plugin system -- it is generally good coding practice to re-use existing functions wherever possible, this seems to have taken precedence over decent hooks for MOD authors in v3.0. You really need to drive home to the team that each hook in phpBB core has a specific purpose. If a function is used for multiple purposes, then it needs to have multiple, differently-named hooks. I think enforcing this paradigm (you are hooking process flows, rather than functions), will help a lot.

Hope this helps,

John

[A_Jelly_Doughnut]

Secondly, I understand this will come in a later RFC, but it is imperative that the very outer shell of phpBB can be hooked. That is, the very last stage of trigger_error, and the final ob_gzhandler. I'd also like to see a hook very early in common.php -- or, even better -- in the include statement in every call to common.php (so error settings, etc. can be overridden). Even though you'll be eliciting these requests later, this will undoubtedly influence the overall architecture -- the hook system will need to be loaded before anything else, such as initialising the database. This is all important to embed phpBB successfully without core code changes.

Well, the proposed hook system is DB driven, which would mean it could not do anything until the DB is initialized.

Is there some example where this would be needed?