Add NotifierManager::insertContent for easy content insertion.

[neekfenwick]

Fixes #6053 .

Needs Review

This PR is not finished, submitted for review as a concept. I include just a few example hooks in the Account History Info page. We would need to scatter $zco_notifier->insertContent('CONTENT_FOO_BAR'); calls throughout the template files, similar to how notify() calls were.

Improving NotifierManager Performance

I did some work on improving the array hunting done in NotifierManager, but it involves changing the core EventDto::$observers data structure which I think some addons rely on (I saw Watching Observers (https://www.zen-cart.com/showthread.php?226649-Watching-Observers-Support-Thread) for example). See https://github.com/neekfenwick/zencart/tree/content-notifiers-issue-6053

This PR is a completely separate branch, without the EventDto etc modifications.

Description

This PR takes the idea of NotifierManager and Observers, and extends it for arbitrary content inclusion at key points on shop front pages. We are used to e.g.:

$zco_notifier->notify('NOTIFY_ACCOUNT_HISTORY_INFO_EXTRA_COLUMN_HEADING', $order, $extra_headings);

Observers may then register for the NOTIFY_ACCOUNT_HISTORY_INFO_EXTRA_COLUMN_HEADING event and be called by NotifierManager when that event is fired. This PR introduces CONTENT_FOO_BAR events with different behaviour.

One line define page inclusion

The main aim is to allow templates to position define page content easily in a single line of code. For example modify tpl_account_history_info_default.php to have this line just under the <h1>:

$zco_notifier->insertContent('CONTENT_ACCOUNT_HISTORY_INFO_INTRO', $order);

Whether or not any Observers are registered, this will attempt to replace the 'CONTENT_' with 'DEFINE_', lowercase the result, and require a define page, e.g. define_account_history_info_intro.php, wrapped in a suitable div, e.g.:

<div class="define-page"> ### content here ### </div>

If a define page of that name is not found, no automatic output is made and no error is raised.

Event name to Define Page mapping

If an Observer wants to register for an event like CONTENT_ACCOUNT_HISTORY_INFO_INTRO but wants to use a differently named define page, e.g. one shipped with their addon, the Observer can declare a map instead of implementing a handler function, e.g.:

public array $contentDefinePageMap = [
    'CONTENT_ACCOUNT_HISTORY_INFO_INTRO' => 'define_my_addon_shipped_file'
];

You can equally handle the event in a custom handler function, and call the define page output function helper, but this is neater.

Custom handler functions

Similar to the existing Notify system, if a method is found matching the snakeCased event name, it is invoked. It may then use the define page output helper function, or directly output content, using data passed in.

Difference from Existing Notifiers

All this looks a lot like what we already had before, but is conceptually and functionally different. We can output HTML from a normal Notifier but this approach brings a couple of benefits:

• Easily output Define Pages by name, associated with the event names. This isn't done in NOTIFY_ hooks, and doesn't really suit their purpose.
• The average user can see a hook like "CONTENT_" and easily connect it with a Define Page they see on the admin Define Pages editor.
• An addon author writing an Observer can register for both NOTIFY_ and CONTENT_ events in the same Observer, and implement the handlers differently for each task. NOTIFY_ handlers would do data processing, email sending etc., CONTENT_ hooks would display data (or just use the $contentDefinePageMap).

Example

The system allows for Observers to register, too, i.e.

auto.content_notify_test.php:

<?php
// Test file for Content Notifier.

class zcObserverContentNotifyTest extends base
{
    public function __construct()
    {
        $this->attach($this, array(
            'CONTENT_ACCOUNT_HISTORY_INFO_INTRO'
        ));
    }

    // If this is uncommented, then the event will cause the define page to be output and not call the handler function.
    // public array $contentDefinePageMap = [
    //     'CONTENT_ACCOUNT_HISTORY_INFO_INTRO' => 'define_some_page_name_here'
    // ];

    /**
    * Handler function called when our observed event is fired.
    *
    * @param object $callingClass
    * @param string $notifier
    * @param array $params
    * @return void
    */
    public function content_account_history_info_intro(&$callingClass, $notifier, &...$params) {
        $order = &$params[0];
        $this->outputDefinePage('define_account_history_info_intro');
        // Show a different define page content, depending on the latest status history of the order.
        $latestStatus = $order->statuses[0]['orders_status_name'] ?? 'Pending';
?>
        <p>This order is currently <?php echo $latestStatus ?>, please be patient.</p>
<?php
    }

}

NotifierManager had some legacy cruft relating to what methods might be on the Observer class; the update() function and different snake_cased method names. This PR simplifies all that and assumes the function will be content_<snake_cased_eventID>(). There is also no '*' event registration possible.

Note that I am using Parameter Lists &...$params in passing data to the observer, instead of &$param1, &$param2, etc. The downside is that the handler methods no longer have named parameters, such as $order, $some_data etc. I could go either way on that.

[drbyte]

This else clause might be skippable, since it's guarding against older/legacy observer syntax support.
So if what's being built here won't ever need that legacy support, maybe the clause isn't needed.
Not that it's harmful to keep either.

[neekfenwick]

I thought this was still useful. If an observer registers interest in CONTENT_FOO and then does not declare a handler function, that's a bug that needs raising. Note that if the contentDefinePageMap contains a mapping, the loop is continued and the error would not be raised, as intended.

Now I think about it, my 'sniff for define page' logic is poor implementation of the design edit removed some rubbish I wrote here :)

FYI I have done a second take of development on a new branch, https://github.com/neekfenwick/zencart/tree/content-notifiers-take-two-issue-6053 as discussed in #6053 (comment) using suggestions from @lat9 to keep the new content notifier separate from NotifierManager, by instantiating a second global $zcc_notifier, similar to $zco_notifier. This work was a little while ago and if it's actually more sensible to keep everything under one hood in NotifierManager I can back out that branch and stick to the original.

[drbyte]

I like the simplicity of the one-line call to import/inject other content into certain places in a template.
It's a feature that's been begging for inclusion for some time.
Your proposed approach, while geared mostly around using Define Pages, helps address that need.

Nomenclature

There are lots of ways to "name" this. I've often referred to this sort of approach as using "render hooks", depending on what needs to be "rendered".

Define Pages as content source

Relying on Define Pages as the content source has its pros and cons.

pros

The main "pro" is that everyone's always begging for Admin-controlled editing of dynamic page content. And the merits you mentioned tie in to that and support why you've gone that route. I support it, although I've always disliked Define Pages (although it's served a purpose).

cons

The "cons" include increasing the demand to optimize the Define Pages list in the Admin, mainly to make it easier to navigate through dozens of pages that will make that list grow if people really embrace this. Not hard to do, just hasn't been a big demand yet.
Filesystem permissions for editing those files are less of an issue nowadays. They were nightmarish in time past.

Using content observer for realtime context-specific data enhancement

Your approach offers some degree of real-time dynamic content modification. One of your examples proposes altering the displayed order-status-name based on some criteria. That's gotta be done in the observer class, which is actually better than the past/current approach of having someone alter their template file to manipulate the data "there" before it's displayed: locating that logic in the observer class to be handled when that insertContent hook is called, is less "obvious" to the coder who just wants to tamper with the data procedurally, but keeps the templates cleaner, easier to upgrade without losing the changes to custom logic, etc etc etc.

Define Pages vs PHP code

While Define Pages "can" execute PHP code that's in proper <?php ...?> tags, that code must be inserted separate from the Admin UI.
Some call this a downside to Define Pages: that they can't handle adding PHP code to them via the Admin. This is actually by design, so that hackers with unauthorized admin access can't do malicious things by throwing PHP into those files.

Your proposal isn't geared so much around these Define Pages "processing" any PHP, but more around using them to "include content" at various "hooks" in the flow of outputting content to the browser.

Design Choices

I'm okay with the choices you've proposed, ie:

• having an observer declare a $contentDefinePageMap array, for simplicity
• Using CONTENT_ prefix
• dynamic CSS classes
• leveraging existing notifier/observer / pub/sub infrastructure (and exposing some areas where it can be optimized)

suggested changes

Nitpicky suggestions:

• change $classList to $additionalClasses or $addtionalCSSClasses or $additionalCss just for clarity about what "class" is referring to (as opposed to a PHP class)

[drbyte]

Question:
Are there more ways it can be enhanced to make dependence on Define Pages less "required"?

Perhaps the protected outputDefinePage() could be the "default" output of a protected renderOutput() method, if some "other" observer-generated-content isn't built?
And then some of the extra params that the observer receives from the insertContent() call in the template, could be used to render "stuff" instead of needing a Define Page at all?

I dunno that it would be necessary to support passing a Closure (numerous complexities to that, but doable), but even passing a function call as a parameter to insertContent(), where its output is in a form that the observer would accept and be able to process ... could be a way to allow plugins to leverage this using their own features.

Possible use-cases that come to mind include drop-shipping-provider status details, tracking number details, account-feature details like rewards-balances, or bad-address-format alerts.
People might want to inject ZC details in places where it's not already shown, like free-shipping-threshold-almost-met, etc.
Many of these wouldn't need Define Pages, but do need a render "hook" that can be called in order to be displayed at certain logical places within the template.

[drbyte]

Coming up with all the ideal places to build-in these render-hook calls to insertContent() is an interesting project in itself.

Another benefit of defining (and documenting) a list of built-in hooks is that it can make adaptation to different display-sizes quite a bit easier ... provided those hooks are placed in spots that are tested to render well under those conditions.

As you mentioned also, there may be value in encapsulating some of these calls to insertContent inside new <div>/whatever containers so they're easier to style, both while empty and while populated.

[neekfenwick]

Many thanks for the review, hugely appreciated.

Are there more ways it can be enhanced to make dependence on Define Pages less "required"?

Yes, see my Example in the OP, I have a handler content_account_history_info_intro that outputs raw HTML using PHP ?>...<?php tags. I do include a define page in that example but that's not necessary, you could simply output content. My PHP chops aren't the best, is there a better way you'd like to be able to output content?

Perhaps the protected outputDefinePage() could be the "default" output of a protected renderOutput() method, if some "other" observer-generated-content isn't built?

This is the intent of the lines in class.ContentNotifier.php https://github.com/neekfenwick/zencart/blob/724cb6935623939ce567caba70efe576705d3077/includes/classes/class.ContentNotifier.php#L34 .. a define page is looked for by default, and outputDefinePage will exit early if one is not found. So, you can have a define page on disk and zero extra coding to have its content output at the matching content hook point.

The "cons" include increasing the demand to optimize the Define Pages list in the Admin, mainly to make it easier to navigate through dozens of pages that will make that list grow if people really embrace this.

Agreed. Perhaps my $define_page_name = strtolower(str_replace('CONTENT_', 'DEFINE_', $eventID)) should not replace 'CONTENT_' .. i.e. we could have a set of define_xxxx.php files, and a set of content_xxxx.php files, in html_includes, to help keep the sets of files separate. Thus, these 'content' hooks would not necessarily be geared around define pages as they are known now. Seems like a bit of a muddy decision, though, I'm not sold either way.

Coming up with all the ideal places to build-in these render-hook calls to insertContent() is an interesting project in itself.

Certainly daunting. I haven't been involved in the similar task of sprinkling current notifier hooks around, a similar problem. Would want to exhaust discussion of use cases to get the API right before diving in. Your mention of closures as arguments is interesting, I don't understand your point well. Can you elaborate an example?

As you mentioned also, there may be value in encapsulating some of these calls to insertContent inside new

/whatever containers so they're easier to style, both while empty and while populated.

I could not decide on this. Often, wrapping content in a <div> can be constraining regarding layout. I wanted a way to indicate whether the content being inserted should be wrapped in a container or not, but couldn't come up with an elegant way to do it. To make it inline instead of block one could wrap it in a <span> instead, and either way use CSS to make the wrapper display:block or display:inline as required, but this feels like an anti pattern to me, having to bandage markup with CSS to defeat the markup's default styling. Suggestions most welcome.

[dbltoe]

Just my 3.9 cents worth.
Using <span> is often a roadblock to easily handling accessibility issues not considered in the original coding.
With a div or class, the content can be colored/sized easier if needed.

[drbyte]

Many will be tempted to include their own DIV wrapper in whatever they generate to send to the renderhook, so that they can style their bespoke block themselves.

So, my point about wrapping content in <div> containers was that a consistent pattern needs to be used so that "IF" a content div is injected into a page by this feature, that div needs to not break the current layout of the page. AND if we decide to just force an empty content DIV onto all pages where we register these hooks so they can be injected "into", then their presence needs to not break current layout, and hopefully whatever gets injected can't break things badly.

In shorter words: do we use:

<?php renderHook('foo'); ?>

or

<div class="renderHook rhCreateAccount">
    <?php renderHook('foo'); ?>
</div>

Obviously, whatever code calls the renderHook needs to be mindful that it doesn't inject invalid HTML or content blocks that break the layout. But that's on them.

[scottcwilson]

I have a client whose define pages use substitute tags (%%SOMETHING%%) so his code can be syndicated - I'll add on whatever is required to make that all work once this is merged.

Where do we stand with this PR?

[neekfenwick]

I have a client whose define pages use substitute tags (%%SOMETHING%%) so his code can be syndicated - I'll add on whatever is required to make that all work once this is merged.

Not sure what you mean by 'syndicated', that means to publish or broadcast, right?

On this PR, my approach was to use function calls in the same vein as the $zco_notifier->notify(...) code we already have. How would %%SOMETHING%% tags work? Core ZC doesn't use a templating engine for define pages.

On my main client's site, I actually use the Smarty templating system to pre-process many of my define pages so I can use substitutions and also more advanced things like custom PHP functions in the templates, which I like but as was mentioned in the email system overhaul (also on hiatus) Smarty is not necessarily the best for everyone, I ended up using the Laravel Blade engine there. Maybe that would be an option here too? You don't specify how the %%foo%% tags are interpreted, is there a templating engine used in your client's implementation?

Where do we stand with this PR?

I got stuck on the discussion at #6053 when the different approaches to modify NotifierManager, people stopped responding to my attempts, see latest comment on Dec 4. Both implementations work, it's a question of what is likely to be merged. I didn't want to continue spending my limited time experimenting with so little feedback. I know we're probably all volunteers so not trying to sling mud.

[scottcwilson]

How would %%SOMETHING%% tags work? Core ZC doesn't use a templating engine for define pages.

One of the observers would add string substitution. The specifics aren't important, just that you have more flexibility than you do by simply pulling a file directly into the content, as is done currently.

Not sure what you mean by 'syndicated', that means to publish or broadcast, right?

He licenses it to other people who use different values for %%SOMETHING%%. Substitution is done dynamically based on database files so the stores can share the same fileset.

[neekfenwick]

How would %%SOMETHING%% tags work? Core ZC doesn't use a templating engine for define pages.

One of the observers would add string substitution. The specifics aren't important, just that you have more flexibility than you do by simply pulling a file directly into the content, as is done currently.

Yeah, I like the idea of substitution. Using a more advanced templating engine gives a lot more flexibility. For example, I use Smarty to allow me to use custom functions in my define pages that look up the SEO friendly URL from the CEON URI Mappings module, so I don't have to hardcode URLs to our product pages. If we change the URL mapping in the CEON database table, our define pages all update automatically to use the new URL.

Blade works by turning your text into PHP code and executing it, so you can use globals directly in the template, e.g. {{ STORE_NAME }}. You don't have to write your own PHP code to do the substitutions. But, of course you have to deal with invoking the Blade engine. On my email work I borrowed a Blade helper class that encapsulates that as much as possible. I could add this as a proof of concept on this branch if that sounds good to you.

One sticking point I reached with the email work is that the Blade template can contain directives which are very unfriendly to a WYSIWYG HTML editor, for example having loops and conditional statements embedded in and around the HTML of the email content didn't render well in the CK Editor. Complex emails like the order confirmation, for example, have loops over products in the order.

This would be a similar problem in define pages, but a much lesser problem in that their contents is usually very simple so you'd only have simple things like {{ STORE_NAME }}. It could work pretty well.

Not sure what you mean by 'syndicated', that means to publish or broadcast, right?

He licenses it to other people who use different values for %%SOMETHING%%. Substitution is done dynamically based on database files so the stores can share the same fileset.

Dealing with "licensing" it sounds a bit heavyweight :) I'd suggest we just build it into core ZC as code we write ourselves.

I imagine having something like a zen_get_define_body($define_page) function which figures out the PHP file on disk, fetches it into memory with something like file_get_contents(), then calls the notifier system to allow observers to modify the contents, with a core ZC observer processing it through the templating engine of choice, e.g. Blade, to execute it as a template and do all the substitutions and other processing.

[neekfenwick]

@scottcwilson what did you think of my points about using a more advanced templating solution, instead of simple string substitutions? Is it worth me building a working proof of concept? I'm concerned that this, like my conversations on the email template overhaul, has gone quiet. I'm reluctant to put work into something that will ultimately be ignored and not merged.

[scottcwilson]

Would prefer that you keep it simple. My only intention in adding my comments was to show how your change might be used, not to suggest a roadmap. Most people who use this at all will just use the new feature you added and stop there.

[neekfenwick]

Would prefer that you keep it simple. My only intention in adding my comments was to show how your change might be used, not to suggest a roadmap. Most people who use this at all will just use the new feature you added and stop there.

Ah, then I misunderstood your earlier comment about your buddy with code that could be syndicated. It sounded like you were eyeing his alternative implementation of the same feature with the intent to merge that.

I can keep the initial implementation simple, with a notifier hook to allow modification of the define page content for e.g. processing through a substitution / templating solution of choice.

I'm left with a couple of implementation options that have been raised before:

1/ Mentioned in #6053 (comment) it seems strange for NotifierManager to brute force hunt an array for matching observers, instead of having a data structure keyed by event ID that can be indexed into much more efficiently. I have re-written it on one of my branches, it doesn't change the API of NotifierManager, just means a more efficient internal operation. Does this sound terrible to anyone?

2/ Should the new 'CONTENT_' event ID's be stored in the same data structure in NotifierManager as all the current notifier events, or should we have a separate system for them? e.g. keep the existing $zco_notifier for normal events, and introduce a new $zcc_notifier dedicated to content hooks?

[neekfenwick]

@scottcwilson I tweaked things based on your comments, adding a functions_general.php function zen_get_define_page_content() which verifies the file on disk and gives NOTIFY_ZEN_PROCESS_DEFINE_PAGE a chance to process it to text content. If that observer does not exist or returns no result, then the file is require()'d as PHP code and the result returned as text content.

If the CONTENT_* event results in a define page being found, the result is wrapped in a <div> with classes define-page and define-the-name-of-the-define-page (with underscores replaced with hyphens, modern CSS style). If another observer has registered for the CONTENT_* event, it can output whatever it wants. At present there's no way for an observer to block the output of a define page by a matching name if found, not sure if we would want that.

I haven't introduced any new class like NotifierManager for now, just allowed any CONTENT_* events to be stored in the same data structure as the old NOTIFY_* events (in EventDto::observers). The inefficiency of brute force search can be addressed outside of this work.

[scottcwilson]

@neekfenwick Can you squash this and fix the conflicts? I'd like to get this in 2.1.0.

[neekfenwick]

@neekfenwick Can you squash this and fix the conflicts? I'd like to get this in 2.1.0.

I've rebased onto master, squashed my changes and force pushed the branch, should be good to go.

I imagine you guys will find things you want to tweak about it as we actually use the thing. I'm still interested in rewriting the core EventDto mechanism to remove the brute force array lookup, I'm pretty sure I wrote an implementation on a branch but can't find it now.

There's only one template file updated that calls the new system, tpl_account_history_info_default.php, I guess we need a separate piece of work to cover adding hooks in elsewhere, as @drbyte mentioned earlier in this thread at #6064 (comment)