euantorano
commented
1 year ago My preference would be for a custom MyAlerts specific hook that gets ram wherever needed, and then documenting this hook. Only downside would be the possible breakage of existing integrations. On 16 Jul 2023, at 13:46, Laird @.***> wrote: Unfortunately, the fix in (the now merged) PR #290 causes its own problem, which is perhaps not unsurprising, because it was a bit of a hack. That hack was to run the global_start hook in myalerts_xmlhttp(), which itself runs during the xmlhttp hook called from xmlhttp.php. This is because that (global_start) is the hook that most client plugins use to register their MyAlerts formatters, which typically (for client plugins) aren't otherwise registered during xmlhttp.php, leading to unformatted alerts being returned during dynamic updates. The problem this in turn causes is due to at least one plugin - PHP and Template Conditionals - calling the same code - the phptpl_run() function in this case - from both the global_start and xmlhttp hooks, both of which now run for the xmlhttp.php script when MyAlerts is active. The problem this results in is that phptpl_run() declares a class, and in PHP the same class cannot be declared more than once, which is exactly what calling this function twice results in, leading to an error message being appended to the output from xmlhttp.php. I can see three possible alternative solutions which avoid this problem:
Make no changes to MyAlerts but require (via documentation and publicity) all client plugins to ensure that their formatters are correctly registered for xmlhttp.php too, whether via adding an additional hook-in to the xmlhttp hook or, e.g., hooking in solely to pre_session_load, which runs for all scripts, xmlhttp.php and otherwise. Provide a normative MyAlerts-specific hook into which client plugins should hook to register their formatters, and ensure that it runs no matter which script is called, perhaps by itself being run from a hook-in to the pre_session_load hook. Shift all dynamic MyAlerts calls from xmlhttp.php to alerts.php, appending a query parameter ajax=1 as is common in the MyBB codebase to indicate that the call is dynamic. Maintain the hook-in to xmlhttp so as to redirect, via HTTP, requests at the old locations to requests at the new locations, for backwards compatibility.
My preferred option is the third and last, because it is backwards compatible and doesn't require plugin authors to make any changes. Its only drawback is that it sacrifices the slight performance boost of xmlhttp.php, which avoids the more expensive require of global.php. Unless anybody objects, or has improvements to suggest, then I'll implement this solution (and revert the existing hacky solution) after PR #282 is merged, because it will build on that PR. So as for all of this to occur fairly swiftly, I'll in turn merge that PR myself fairly shortly in the absence of any objections or suggested improvements if it's not otherwise merged fairly shortly.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you modified the open/close state.Message ID: @.***>
lairdshaw
Unfortunately, the fix in (the now merged) PR #290 causes its own problem, which is perhaps not unsurprising, because it was a bit of a hack.
That hack was to run the
global_starthook inmyalerts_xmlhttp(), which itself runs during thexmlhttphook called fromxmlhttp.php. This is because that (global_start) is the hook that most client plugins use to register their MyAlerts formatters, which typically (for client plugins) aren't otherwise registered duringxmlhttp.php, leading to unformatted alerts being returned during dynamic updates.The problem this in turn causes is due to at least one plugin - PHP and Template Conditionals - calling the same code - the
phptpl_run()function in this case - from both theglobal_startandxmlhttphooks, both of which now run for thexmlhttp.phpscript when MyAlerts is active.The problem this results in is that
phptpl_run()declares a class, and in PHP the same class cannot be declared more than once, which is exactly what calling this function twice results in, leading to an error message being appended to the output fromxmlhttp.php.I can see three possible alternative solutions which avoid this problem:
xmlhttp.phptoo, whether via adding an additional hook-in to thexmlhttphook or, e.g., hooking in solely topre_session_load, which runs for all scripts,xmlhttp.phpand otherwise.pre_session_loadhook.xmlhttp.phptoalerts.php, appending a query parameterajax=1as is common in the MyBB codebase to indicate that the call is dynamic. Maintain the hook-in toxmlhttpso as to redirect, via HTTP, requests at the old locations to requests at the new locations, for backwards compatibility (i.e., when board admins fail to update their MyAlerts templates).My preferred option is the third and last, because it is backwards compatible and doesn't require plugin authors to make any changes. Its only drawback is that it sacrifices the slight performance boost of
xmlhttp.php, which avoids the more expensiverequireofglobal.php.Unless anybody objects, or has improvements to suggest, then I'll implement this solution (and revert the existing hacky solution) after PR #282 is merged, because it will build on that PR. So as for all of this to occur fairly swiftly, I'll in turn merge that PR myself fairly shortly in the absence of any objections or suggested improvements if it's not otherwise merged fairly shortly.