search

mybb / mybb

MyBB is a free and open source forum software.
https://mybb.com
GNU Lesser General Public License v3.0
1.09k stars 411 forks source link

A function to (re)generate the header (/footer) #3939

Open lairdshaw opened 4 years ago

lairdshaw commented 4 years ago

I am starting this conversation based on discussions in #3926. The basic problem is this:

In 1.8.*, the $header variable is generated right near the very start of the processing of a given request, in global.php. Sometimes, though, after that, the further processing of a page (or plugin) invalidates the content of that $header variable in some way, such that it needs to be regenerated. The argument runs, though, that simply performing that regeneration for a core page some time after global.php was run is plugin-unfriendly, because it might overwrite changes made to the header by a plugin in a hook in global.php. Moreover, even if a plugin - rather than core code - regenerates the header, the same applies: it could be overwriting unrecognised changes made either by core code or by another plugin.

I have now encountered this issue twice: once in the course of preparing PR #3926, and once in the course of writing a plugin which hooks in to the global_intermediate hook: sometimes, my plugin's later processing invalidates its earlier processing in that hook, and it needs to later update the header already generated in that hook from global.php (FWIW: right now, my plugin simply goes ahead and regenerates the header, regardless of any consequences).

My ultimate suggestion for solving this problem is: generate the header at the very end of processing, rather than at the very beginning. In that way, we can be sure that all changes that need to be reflected in the header will be.

This would be a significant challenge/change for the 1.8. codebase, however, @Sama34 has indicated that any such change would not be implemented until at least the 1.9. codebase anyway - a codebase which is much, much more amenable to it, largely because with the new Twig interface, the equivalent of the $header variable - the included base/partials/header.twig Twig template - is only generated on demand and right at the end of processing anyway, as included by the final page template, typically via the base/layouts/master.twig template. This seems to make it more feasible to convert the code which generates the variables upon which that header template depends - currently in global.php - into a function which can be called only as and when that "header" Twig template needs it, however, I am not sure whether such a function - including its side-effects - could be called from within that Twig template, simply because I am not particularly familiar with Twig.

Likewise with the footer template.

So, the conversation I propose for this issue is:

  1. Discuss (if necessary) and resolve the (my) question: is it possible for the code in global.php which generates the variables used for the base/partials/header.twig Twig template be moved into a function which is only called (automagically) just as (or before) that template is rendered (by MyBB/Twig)?

  2. If so, and given the issues raised above, should this function be implemented? Why / why not?

  3. If not, what alternative (to solve the issues raised above, and any others) would you suggest?

  4. Should something similar be done for the footer, with the same ancillary questions?

Sama34 commented 4 years ago

Discuss (if necessary) and resolve the (my) question: is it possible for the code in global.php which generates the variables used for the base/partials/header.twig Twig template be moved into a function which is only called (automagically) just as (or before) that template is rendered (by MyBB/Twig)?

I think this can be done in 1.9, it worries me the possible plugin incompatibilities but ultimately belief this shouldn't be something hard for contributors to implement.

lairdshaw commented 4 years ago

it worries me the possible plugin incompatibilities

I agree that there are possible plugin incompatibilities, but I also suggest that, generally, each plugin is, for the most part, only going to be adding its own little independent flairs to the header, such that, generally, the different additions made by the different plugins will be compatible.

euantorano commented 4 years ago

Looking at the 1.9 code, the whole $header variable is gone, and we instead use partials:

These variables and conditions would only be evaluated when the overall page is being rendered, which is usually right at the end of processing.

I plan to also add hooks to hook in whenever a template is included in Twig so that plugins can modify the context before and after rendering.

The only variables that don't seem to be easy to modify globally would perhaps be headerMessages, unless I'm missing something. And I believed that is registered as a Twig global which could be altered.

If there is any logic in global.php that affects the header partial that isn't modifiable, that logic could be moved to a Twig extension which would allow it to be altered. I plan to write a blog post about how the new IoC container allows modifying registered classes like this.

lairdshaw commented 4 years ago

Thanks for your response, @euantorano. I like the hook idea that you have, and I'll be interested to read your blog post. I'm curious though to know your reaction to my main suggestion: that the code in global.php which sets/evaluates at the very beginning of a request all of the variables that are later referenced in the header partial right at the very end of a request be moved into a function which is called on demand just before the header partial is rendered: that is, to move their evaluation from the beginning to the end of each request. This would minimise the extent to which those variables even need to be modified globally.

euantorano commented 4 years ago

IMO if the variables are only needed in header.twig, I can see a clear use case for a Twig extension that registers functions/filters to get these values. Perhaps if you could give me an example of the variables causing problems I'd be able to understand better and come up with a solution.

lairdshaw commented 4 years ago

Oh, a very current example of the variables causing problems is in #3926. To summarise the problem being discussed in that PR:

In global.php, at the beginning of the request, the $modnotice variable is set based on the moderation queue of unapproved attachments, posts, and threads, and this variable is then used in the eval() which sets the $header variable.

Later, though, in editpost.php, it is possible that an unapproved attachment is deleted, or that an attachment's approval status is reversed, which invalidates the $modnotice variable - thus invalidating $header too.

This would be very simply solved if $modnotice (and, subsequently, $header) were set after editpost.php had done its processing, rather than before.

euantorano commented 4 years ago

It looks like modnotice and such are all now handled by the headerMessages variable.

I can't seem to find where this is actually passed in to any templates though, so it looks like that functionality may be totally not working in 1.9 right now...

As such, now would be a good time to work out how to do it.

Currently, headerMessages is defined right before the global_intermediate is ran so plugins could add new elements then via that hook, but not adjust elements that are added later.

If headerMessages was then registered as a Twig global, it would be accessible everywhere, including when header.twig eventually gets rendered. So editpost.php could just edit the Twig global, so long as it's before MyBB\template() is called - and this is usually done only when rendering the full page.

Or we could create a new singleton class for holding header messages and use a Twig extension to pull messages from that class. Then anywhere in the code base could access and modify the header messages so long as it was done before the call to MyBB\template().

yuliu commented 4 years ago

Sorry I'm not sure whether I get a clear mind about how Twig is working with MyBB, though I've been reading it for months. 🤣

And why eval('$header = "'.$templates->get('header').'";'); is still there..

And "I plan to also add hooks to hook in whenever a template is included in Twig so that plugins can modify the context before and after rendering." is what I've been waiting for, even for 1.8 branch..

Currently, headerMessages is defined right before the global_intermediate is ran so plugins could add new elements then via that hook, but not adjust elements that are added later. If headerMessages was then registered as a Twig global, it would be accessible everywhere, including when header.twig eventually gets rendered. So editpost.php could just edit the Twig global, so long as it's before MyBB\template() is called - and this is usually done only when rendering the full page.

It's here {% for template, options in headerMessages %} and it could be altered as long as the header is evaluated at the end of processing just before the output in MyBB\template().

euantorano commented 4 years ago

That eval code is completely useless, and I have no idea why it's still there either - see #3684 as there are still quite a few cases like this that I plan to go through with a fine toothed comb once I finish rebasing. This weekend is rebase completion weekend I hope, so long as there are no other distractions.

The way that Twig templates work, you have to explicitly pass in each variable to MyBB\template() unless it's registered as a Twig global, and I can't see that being done at all for headerMessages based off a GitHub search, hence my confusion.

Templates and variables in Twig are only evaluated when templates are rendered, which should solve a whole load of problems.

lairdshaw commented 4 years ago

If headerMessages was then registered as a Twig global, it would be accessible everywhere, including when header.twig eventually gets rendered. So editpost.php could just edit the Twig global, so long as it's before MyBB\template() is called

I don't see how it could simply edit that global: how, without some sort of potentially fallible regex pattern-matching, would it know which element of the array it needs to edit? Wouldn't it need, in fact, to regenerate the entire variable (array)?

yuliu commented 4 years ago

I think I get the idea how Twig is working in MyBB, partly. Thanks for the explanation. A variable that will passed to Twig will be either explicitly passed to \MyBB\template() or registered as a Twig Global Variable.

It's OK for headerMessages, it's registered here in https://github.com/mybb/mybb/blob/develop/1.9/inc/src/Twig/Extensions/ThemeExtension.php#L63 .

I don't see how it could simply edit that global: how, without some sort of potentially fallible regex pattern-matching, would it know which element of the array it needs to edit? Wouldn't it need, in fact, to regenerate the entire variable (array)?

headerMessages in 1.9 is an iterable array of ("message"/"banneduser"/"pmnotice" => string/array), but "message" is shared by many. BTW, 1.9 code base doesn't have moderation queue right now. We could add a $headerMessages['mod_queue'] to match the needs. Then in editpost you could just re-generate the content for $headerMessages['mod_queue'].

euantorano commented 4 years ago

@lairdshaw

I don't see how it could simply edit that global: how, without some sort of potentially fallible regex pattern-matching, would it know which element of the array it needs to edit? Wouldn't it need, in fact, to regenerate the entire variable (array)?

With the way, that variable is currently used it would not be easy, but IMO it should become an associative array like this:

headerMessages = [
    'boardClosed' => new TextMessage('bbclosed_warning'),
    'pending_joinrequests' => new PendingJoinRequestsMessage($total_joinrequests),
    'unread_reports' => new UnreadReportsMessage($unread),
   'banneduser' => new BannedUserMessage($banDetails),
...
]

Each of these Message classes would extend a base class which handles rendering the message as a template. This way, you can easily look up a specific message type by key, and modify the class properties. Plugin authors can easily add their own message types too. The message classes could also be used to set the ordering of the message and such too, which would add some more flexibility.

The actual usage might end up being slightly different, but that's the way I'd like to see things work.

euantorano commented 4 years ago

@yuliu

It's OK for headerMessages, it's registered here in https://github.com/mybb/mybb/blob/develop/1.9/inc/src/Twig/Extensions/ThemeExtension.php#L63

Aha! Good find. GitHub search seems totally unable to find that 🤷

euantorano commented 4 years ago

On a total side note, given the confusion around Twig, would it be a good idea for me to write a blog post explaining how it works and the benefits it offers and such?

JordanMussi commented 4 years ago

GitHub search seems totally unable to find that 🤷

From https://help.github.com/en/github/searching-for-information-on-github/searching-code:

Only the default branch is indexed for code search. In most cases, this will be the master branch.

yuliu commented 4 years ago

On a total side note, given the confusion around Twig, would it be a good idea for me to write a blog post explaining how it works and the benefits it offers and such?

Using a framework, for example Twig, is a non-negligible barrier for entry-level PHP developers, such as me. I think the most important part for me to learn is how Twig is involved in MyBB, i.e., 1) how Twig is offering template engine (.twig template files, extensions, etc.) to MyBB and 2) how to use Twig to output template in MyBB like the eval() thing and variable passing in 1.8. For anything else, I could just look up in Twig's documentation and for quick theme/template editing, save the time of reading source files in inc/src/.

As for benefit, it's kinda something that can't be seen. Security? Not knowing, I'm blinded. Easy to parse permission/control for content showing? Yes, but only after you've developed some theme/template.

It could be a forum post rather than a blog post for me to write.

euantorano commented 4 years ago

@yuliu I'll start drafting some stuff explaining how it all ties together and what exactly Twig does then. I'll create a PR on mybb/blog.mybb.com-drafts over the weekend, and then you and others can chime in with any alterations.

Sama34 commented 4 years ago

I plan to also add hooks to hook in whenever a template is included in Twig so that plugins can modify the context before and after rendering.

That simple? Won't adding hooks be resource intensive?

euantorano commented 4 years ago

Hooks themselves shouldn’t be particularly intensive really, but obviously having lots of plugins with lots of hooks will use more memory. In the case of nothing being registered for a hook, adding the hook just adds a single if isset() condition.

On Fri, 17 Apr 2020, at 18:49, Omar Gonzalez wrote:

I plan to also add hooks to hook in whenever a template is included in Twig so that plugins can modify the context before and after rendering.

That simple? Won't adding hooks be resource intensive?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mybb/mybb/issues/3939#issuecomment-615380241, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFW24ODRY7I6FSWAWNYR3LRNCJDFANCNFSM4MKLR6CA.

Sama34 commented 4 years ago

Just asking as per previous discussions. But don't get to flexible, I might get crazy suggesting hooks myself 😝

euantorano commented 4 years ago

;)

With the new variadic hooks, I plan to have the hook be something like before_template_render and the args be $templateName, $templateContext (hopefully) as we can now easily pass multiple arguments to a hook without having to use arrays or anything.

lairdshaw commented 4 years ago

BTW, 1.9 code base doesn't have moderation queue right now. We could add a $headerMessages['mod_queue'] to match the needs.

Good catch. Yes, that seems to be missing.

lairdshaw commented 4 years ago

Then in editpost you could just re-generate the content for $headerMessages['mod_queue'].

However, if that content were generated at the end of the request rather than at the beginning, then there would be no need to REgenerate it: it could simply be generated once.

I'm sensing a general reluctance for this move though. I wonder whether there are disadvantages to it that I'm not taking into consideration? Right now, I see only advantages.

lairdshaw commented 4 years ago

With the way, that variable is currently used it would not be easy, but IMO it should become an associative array like this [explanation snipped --Laird]

Nice! That seems like a pretty thorough and stringent way to go about things.

lairdshaw commented 4 years ago

I'll start drafting some stuff explaining how it all ties together and what exactly Twig does then.

I would find this helpful too, being new to Twig and the 1.9.* codebase. Thumbs up.

Sama34 commented 4 years ago

I'm sensing a general reluctance for this move though.

If I understand Euantor reply the header will already be generated at the end, only global variables will be generated before and there shouldn't be an issue with later code modifying these variables before the header gets parsed.

Also, (if I understand correctly), developers will be able to hook to any twig template let it be before or after it gets parsed.

If that is the case, moving all the code to a sole function might be redundant, yes.

lairdshaw commented 4 years ago

there shouldn't be an issue with later code modifying these variables before the header gets parsed.

I think it depends on what those variables are though. Consider again the issues in #3926 which led to the discussion we're having here: the moderation queue needing to be regenerated. If one of the global variables to which you refer is the fully rendered moderation queue, then to modify (regenerate) that moderation queue in editpost.php would require either a fair bit of code duplication (as we have seen in #3926) or a function to abstract the common code. I think we would be right to be skeptical of that approach.

If, though, the global variables in question were simply "count of unapproved attachments", "count of unapproved posts", and "count of unapproved threads", and editpost.php only had to adjust +1 or -1 the "count of unapproved attachments" variable, which at the end got rendered by the Twig header template, then I agree, there's no significant problem with that approach.

But I still don't see any problems with moving the generation of all of the relevant globals on which the Twig header template depends until the very end of processing, whereas there is a big benefit to it: no need to regenerate anything...

Sama34 commented 4 years ago

But if the global is a integer then just doing -= $global['queue'] or ++$global['queue'] (or similar) would work.

Then once the header template is parsed it would not render the notification bar if the global has 0 as queue value.

We should know better once the system is ready for us to test. I'm waiting for the rebase to start testing the script to understand how it works.

lairdshaw commented 4 years ago

But if the global is a integer then just doing -= $global['queue'] or ++$global['queue'] (or similar) would work.

Yep, that's what I meant in my second paragraph.

yuliu commented 4 years ago

If, though, the global variables in question were simply "count of unapproved attachments", "count of unapproved posts", and "count of unapproved threads", and editpost.php only had to adjust +1 or -1 the "count of unapproved attachments" variable, which at the end got rendered by the Twig header template, then I agree, there's no significant problem with that approach.

It sounds a good alternative to me. Only to evaluate values of variables and pass them to Twig for render/output at the and of page processing, so that anyone can alter all the values without messing with intermediate variables that has language entries substituted or partial templates rendered.

But I still don't see any problems with moving the generation of all of the relevant globals on which the Twig header template depends until the very end of processing, whereas there is a big benefit to it: no need to regenerate anything...

Without digging in old code base, I'm guessing that developers in 2010s would like to say that such variables like mod_queue and pm_notice as well as bbclosed_warning and remote_avatar_warning didn't tend to change across modules.

The problem of the number of unapproved attachments specified in #3926 also applies to the number of unread pms and pm_notice:

So the conclusion I could get in this discussion till now lies in:

euantorano commented 4 years ago

I agree these values should be calculated right at the end, rather than right at the start.

With my suggestion regarding the new approach using the associative array, that can be done by having the ones that require database queries run the query themselves in the render method, meaning they only query when actually rendered. That way, for example, a plugin could remove one of them from the array (eg: to disable the PM notice completely in favour of alerts...) and it would never query.

On Sat, 18 Apr 2020, at 06:41, yuliu wrote:

If, though, the global variables in question were simply "count of unapproved attachments", "count of unapproved posts", and "count of unapproved threads", and editpost.php only had to adjust +1 or -1 the "count of unapproved attachments" variable, which at the end got rendered by the Twig header template, then I agree, there's no significant problem with that approach.

It sounds a good alternative to me. Only to evaluate values of variables and pass them to Twig for render/output at the and of page processing, so that anyone can alter all the values without messing with intermediate variables that has language entries substituted or partial templates rendered.

But I still don't see any problems with moving the generation of all of the relevant globals on which the Twig header template depends until the very end of processing, whereas there is a big benefit to it: no need to regenerate anything...

Without digging in old code base, I'm guessing that developers in 2010s would like to say that such variables like mod_queue and pm_notice as well as bbclosed_warning and remote_avatar_warning didn't tend to change across modules.

The problem of the number of unapproved attachments specified in #3926 https://github.com/mybb/mybb/pull/3926 also applies to the number of unread pms and pm_notice:

lairdshaw commented 4 years ago

Nice, it looks like there's a consensus on this.

With my suggestion regarding the new approach using the associative array, that can be done by having the ones that require database queries run the query themselves in the render method, meaning they only query when actually rendered.

Would there be opportunity for a plugin to hook in in-between the DB query and the render (e.g., to alter any values queried from the DB prior to their being rendered)? I can't quite provide a use case for this right now, but it might be worth supporting.

euantorano commented 4 years ago

That Would likely make sense, but would be decided down the line. If the classes were in the IoC and resolved at render time, then plugin developers could extend the classes themselves to do much the same thing.

On 18 Apr 2020, at 09:41, Laird notifications@github.com wrote:

Nice, it looks like there's a consensus on this.

With my suggestion regarding the new approach using the associative array, that can be done by having the ones that require database queries run the query themselves in the render method, meaning they only query when actually rendered.

Would there be opportunity for a plugin to hook in in-between the DB query and the render (e.g., to alter any values queried from the DB prior to their being rendered)? I can't quite provide a use case for this right now, but it might be worth supporting.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

lairdshaw commented 4 years ago

OK. Thanks.

plugin developers could extend the classes themselves

Could more than one plugin extend the same class? If so, I will be interested to see how we handle that scenario. I can think fairly abstractly of a way in which it could be done.

euantorano commented 4 years ago

Yeah, it can. Check out https://laravel.com/docs/7.x/container#extending-bindings for some information, though it’s lacking some detail. I plan to do a blog post about this too, as it should allow extending any core classes resolved via the IoC which will be pretty nice.

On 18 Apr 2020, at 10:07, Laird notifications@github.com wrote:

OK. Thanks.

plugin developers could extend the classes themselves

Could more than one plugin extend the same class? If so, I will be interested to see how we handle that scenario. I can think fairly abstractly of a way in which it could be done.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

lairdshaw commented 4 years ago

Nice. Thanks. I'm going to have to do some reading up on Laravel.

yuliu commented 4 years ago

From this thread: https://community.mybb.com/thread-227613.html , it looks like to me that another Twig Global besides the headerMessages should be added. And further will there be a convenient way for theme authors to add global variables when needed? (Still haven't dived into Twig 🤣 ..

euantorano commented 4 years ago

In that case @yuliu, we'd probably use twig blocks: https://twig.symfony.com/doc/2.x/tags/extends.html - we'd have a block in the layout template, then individual templates extending the layout can overwrite or add to the block.

Re: theme authors to add global variables, new globals can easily be registered from PHP code with MyBB\app('view')->addGlobal('foo', 'bar') (or something like that). A future plan is to allow themes to include bundled plugins.