Hooks for on page content generation similar to Notifier system #6053

neekfenwick · 2023-11-16T03:09:55Z

Raised in Issue #6050, it would be nice to have a way for observers to add content to pages in strategic places, as well as be able to take action on data which the Notifier system provides. We have the Define Page system which simply lets an admin modify a static PHP file on disk, which are then hardcoded into the codebase in various places. It's useful that these notifier hooks pass in data relevant to the action on hand, e.g. the current order, products, customer details, whatever is relevant, that could also be used when generating content by an addon using the content generation hooks.

Is your feature request related to a problem? Please describe.
If I have a Notifier that adds columns to the Account History Info page (showing a single order's products with their statuses), then I'd like to be able to put content above or below the table of products to show a specific informational or warning message.

Describe the solution you'd like
A hook looking something like this:

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

I'm playing around with some ideas in my local codebase, currently it hangs on the existing NotifierManager with a separate function to notify() called insertContent() which looks for registered event handlers beginning with 'CONTENT_' and either automatically inserts a define page (see later), or runs a snake cased method found beginning with 'content_' like 'public function content_account_history_info_intro()`.

It seems there are two main things you'd want to do:

output a simple define page without having to write any code.
run a custom function that can take logical actions, perhaps also outputting a define page.

So, I've also introduced an associative array on NotifierManager called $contentDefinePageMap so the observer can define an element in this array and have a define page output when the registered CONTENT_* event is fired, e.g.

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

With this, the NotifierManager's insertContent() function will simply call outputDefinePage() for that define page, rather than having to implement a handler function in the observer. The observer ends up looking like this:

auto.example_observer.php

class zcObserverExampleObserver extends base
{

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

    /* EITHER define a map entry to spit out a define page when the Event is fired */
    public array $contentDefinePageMap = [
        'CONTENT_ACCOUNT_HISTORY_INFO_INTRO' => 'define_account_history_info_intro'
    ];

    /* OR, define a snake cased function to do some logic and optionally also output a define page */
    public function content_account_history_info_intro(&$callingClass, $notifier, $order, &$extra_headings) {
        if (some logic here based on $order) {
            $this->outputDefinePage('define_account_history_info_intro');
        }
    }

In a real world case, it's likely that the single Observer above would handle multiple NOTIFY_* events as well as CONTENT_* events.

Describe alternatives you've considered
We could simply have a helper function to output define pages, and call that instead of firing a notifier-style event, but I think having the flexibility of a handler function as above is useful.

Additional Context
I'm not familiar with a range of Zen Cart templates. When inserting a define page onto a page it seems wise to wrap it in a container <div> with suitable classes, so that template CSS can style it. Here's the NotifierManager->outputDefinePage() function I have at the moment:

    protected function outputDefinePage (string $define_page_name, ?array $classList = [], ?string $params = null) {
        $define_page = zen_get_file_directory(DIR_WS_LANGUAGES . $_SESSION['language'] . '/html_includes/', $define_page_name, 'false');
        if (file_exists($define_page)) {
            if (empty($classList)) {
                $classList = [];
            }
            if (!empty($params)) {
                $params = ' ' . $params;
            }
            $classList[] = 'define-page';
            ob_start();
            echo '<div class="' . implode(' ', $classList) . '"' . $params . '>';
            require($define_page);
            echo '</div>';
            ob_end_flush();
        }
    }

The ob_start() stuff may be redundant. With the $classList stuff, I'm trying to ensure that a wrapper div has at least the define-page class, plus any other classes passed in, then any other params.

If all this seems useful, I can put it on a branch for critique and am open to any suggestions to modify the approach.

The text was updated successfully, but these errors were encountered:

neekfenwick · 2023-11-22T08:46:56Z

I had a go at improving the lookup done in NotifierManager, which currently does a for loop over all registered observers, testing each one's eventID values in a rather inefficient manner, apparently out of technical debt. If a new system could do away with all that it seems more efficient to have an associative array of eventIDs to registered observers and use PHP's built in array access functions to quickly test for and retrieve registered observers. Because of this structural difference in the data structure used to store registered observers, I'm tending towards having a totally separate member store content observers, but this gets tied up in the Singleton EventDto shenannigans, e.g.

zencart/includes/classes/EventDto.php

Line 25 in 99edde6

$this->observers[$eventHash] = $eventParameters;

There are comments indicating that observers in the Session caused duplicate registrations, which is why a hash is currently generated and tested for each observer registration, though I don't really see how this would be a problem. It seems using an associative array could support this mechanism if it is still required.

Digging around so deep in the base classes isn't something for a 20 minute prototyping session so my work on this has been halted until some more time and/or political will surfaces.

lat9 · 2023-11-27T12:11:12Z

IMO, better to add an auto-loaded new class that extends the base class to keep those operations separate.

neekfenwick · 2023-11-27T16:28:59Z

IMO, better to add an auto-loaded new class that extends the base class to keep those operations separate.

That would be nice but I found EventDto's Singleton nature rather difficult to work around. It's the first time I've approached it so I might be missing something. Seems rather poorly named if, as I recall, all it does is maintain a collection of observers. We'd want to unwrap that design approach and make something more multifaceted. Will think about having another look at it, as I mentioned in my PR I tried rewriting some of the code on another branch but it didn't feel very nice.

lat9 · 2023-11-27T17:48:47Z

Since you're proposing to (essentially) add a notification extension, you could use a class like (haven't tested!):

class MyClass extends base
{
    public function insertContent( ... parameters here ...)
    {
         $this->notify(... parameters here ...);
    }
}

neekfenwick · 2023-11-28T01:24:48Z

Since you're proposing to (essentially) add a notification extension, you could use a class like (haven't tested!):

I had thought you meant to have a totally separate registration system (attach()) but here you derive from base and so use the EventDto/ObserverManager mechanism, so that's good. If it's just insertContent() then yes, this would be easier to implement. It wouldn't call notify() as in your example though, right? That would trigger the NotificationManager::notify() function and attempt to do normal notify stuff. The aim here is to have the new CONTENT_* event observers mingled in with the old NOTIFY_* ones in EventDto::$observers, such that NotificationManager::notify() ignores them due to their name, and instead the new insertContent() function finds them and acts or calls their observers.

neekfenwick · 2023-12-04T06:49:07Z

@lat9 I had another look at it on branch https://github.com/neekfenwick/zencart/tree/content-notifiers-take-two-issue-6053

Our existing base class mixes in two traits, NotifierManager (ability to call notify()) and ObserverManager (ability to attach and detach). Following your suggestion, this new branch of mine declares a new class ContentNotifier extends base and adds the insertContent() function (instead of modifying core files as in my previous branch).

I feel this functionality is not really an 'extension' of the core functionality, it really belongs side-by-side with it. So, I think I'd prefer to convert my ContentNotifier into a trait, and use it in base, just like how NotifierManager is brought in as a trait.

No-one else has commented so I cannot tell how much support there is for the ability to generate custom content in this way. Maybe people are just registering Notifiers and echo'ing output from them, achieving the same goal? Or people just don't modify their shop this much from the core ZC features?

scottcwilson · 2024-01-06T12:23:05Z

Moving to discussion.

neekfenwick mentioned this issue Nov 16, 2023

Account History Info order product list, new notifiers so additional data columns can be provided by Observer #6050

Closed

neekfenwick mentioned this issue Nov 24, 2023

Add NotifierManager::insertContent for easy content insertion. #6064

Open

zencart locked and limited conversation to collaborators Jan 6, 2024

scottcwilson converted this issue into discussion #6123 Jan 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

This issue was moved to a discussion.

Hooks for on page content generation similar to Notifier system #6053

Hooks for on page content generation similar to Notifier system #6053

neekfenwick commented Nov 16, 2023

neekfenwick commented Nov 22, 2023

lat9 commented Nov 27, 2023

neekfenwick commented Nov 27, 2023

lat9 commented Nov 27, 2023

neekfenwick commented Nov 28, 2023

neekfenwick commented Dec 4, 2023

scottcwilson commented Jan 6, 2024

This issue was moved to a discussion.

This issue was moved to a discussion.

Hooks for on page content generation similar to Notifier system #6053

Hooks for on page content generation similar to Notifier system #6053

Comments

neekfenwick commented Nov 16, 2023

neekfenwick commented Nov 22, 2023

lat9 commented Nov 27, 2023

neekfenwick commented Nov 27, 2023

lat9 commented Nov 27, 2023

neekfenwick commented Nov 28, 2023

neekfenwick commented Dec 4, 2023

scottcwilson commented Jan 6, 2024

This issue was moved to a discussion.