Expose config.xml//events/args node to observers

[justinbeaty]

There was at sometime support in config.xml for an <args> node when defining event observers. Currently, this node is read from the XML file and stored in a property, but not available to the event observers in any way.

People have come up with ways to get around this, such as using xpath to read the values in the observer, and patching Mage_Core_Model_App. Magento 2 removed the feature entirely.

However, I think it's useful and can be enabled without any breaking changes. My first thought was since it's a Varien Object, we could just set whatever we decided to name the property:

$event->setConfigArgs($obs['args']);

But the $event object was already initialized with the $args from Mage::dispatchEvent('event_name', [ ...$args ]), so no matter what we named the data key, there is possibility to conflict.

Instead I just used a public array $args, so there shouldn't be any conflicts.

Example

In #109, we could do this:

             <controller_action_predispatch_checkout_onepage_savebilling>
                 <observers>
                     <maho_captcha>
                         <class>maho_captcha/observer</class>
-                        <method>verifyAjax</method>
+                        <method>verify</method>
+                        <args><is_ajax>1</is_ajax></args>
                     </maho_captcha>
                 </observers>
             </controller_action_predispatch_checkout_onepage_savebilling>

And then in the Observer, consolidate both verifyAjax() and failedVerification() into a single verify() method:

class Maho_Captcha_Model_Observer
{
    public function verify(Varien_Event_Observer $observer): void
    {
        $helper = Mage::helper('maho_captcha');
        if (!$helper->isEnabled()) {
            return;
        }

        /** @var Mage_Core_Controller_Front_Action $controller */
        $controller = $observer->getControllerAction();
        $data = $controller->getRequest()->getPost();
        $isAjax = (bool) ($observer->getEvent()->args['is_ajax'] ?? null); // <--- Read config.xml args

        $token = $data['altcha'] ?? '';
        if ($helper->verify((string) $token)) {
            return;
        }

        $controller->setFlag('', Mage_Core_Controller_Varien_Action::FLAG_NO_DISPATCH, true);
        $errorMessage = Mage::helper('maho_captcha')->__('Incorrect CAPTCHA.');

        if ($isAjax) {
            $controller->getResponse()->setBodyJson(['error' => true, 'message' => $errorMessage]);
            return;
        }

        Mage::getSingleton('core/session')->addError($errorMessage);
        $request = $controller->getRequest();
        $refererUrl = $request->getServer('HTTP_REFERER');
        if ($url = $request->getParam(Mage_Core_Controller_Varien_Action::PARAM_NAME_REFERER_URL)) {
            $refererUrl = $url;
        } elseif ($url = $request->getParam(Mage_Core_Controller_Varien_Action::PARAM_NAME_BASE64_URL)) {
            $refererUrl = Mage::helper('core')->urlDecodeAndEscape($url);
        } elseif ($url = $request->getParam(Mage_Core_Controller_Varien_Action::PARAM_NAME_URL_ENCODED)) {
            $refererUrl = Mage::helper('core')->urlDecodeAndEscape($url);
        }
        $controller->getResponse()->setRedirect($refererUrl);
    }

    public function verifyAdmin(Varien_Event_Observer $observer): void
    {
        // Same as before...
    }
}

[justinbeaty]

Although I'm dumb because the example above could have just been this without the need for observer args at all.

if ($controller->getRequest()->isAjax()) {
    $controller->getResponse()->setBodyJson(['error' => true, 'message' => $errorMessage]);
    return;
}

But I think it's still a useful feature, so let's keep it open?

[justinbeaty]

If it's a useful feature, here's two things to discuss:

1. The $args property could be on either Varien_Event, Varien_Event_Observer, or both. It's currently on the first. The differences are this:

    $observer->getEvent()->args; // Varien_Event
    $observer->args; // Varien_Event_Observer

2. Should the $args property be of type Varien_Object $args so that we use:

    $observer->args->getMyProperty(); 

It might feel more "Magento" to access them on a Varien Object...? You can still use array access with Varien Objects, so $observer->args['my_property'] is still valid.

[fballiano]

I think it may be useful so I'd merge it as it is :-)

[fballiano]

@justinbeaty if you give me your green light I'll merge

[justinbeaty]

@justinbeaty if you give me your green light I'll merge

But first, which one looks best?

public function verify(Varien_Event_Observer $observer): void
{
    // A
    $myVal = $observer->args['my_val'];

    // B
    $myVal = $observer->getEvent()->args['my_val'];

    // C
    $myVal = $observer->args->getMyVal();

    // D
    $myVal = $observer->getEvent()->args->getMyVal();

}

[fballiano]

😅 I would say D?

isn't it possible to get observer params both via $observer->getXXX() and also $observer->getEvent()->getXXX() (which is usually what's used lately)?

[fballiano]

not sure if I'd remove this, wouldn't it break anybody's code that does $observer->getXXX() instead of $observer->getEvent()->getXXX()?

[justinbeaty]

It's added here:

maho/app/code/core/Mage/Core/Model/App.php

Line 1438 in 977a78f

...$args, // Mage::dispatchEvent() $args

[justinbeaty]

isn't it possible to get observer params both via $observer->getXXX() and also $observer->getEvent()->getXXX() (which is usually what's used lately)?

You are right. I had originally avoided that to not conflict with the args provided in Mage::dispatchEvent(), but it makes more sense to merge the config.xml values along with them.

Bonus that the PR now only affects one file, Mage_Core_Model_App. Also I think this snippet is more readable when it comes to the "shape" of the object passed to the observer.

maho/app/code/core/Mage/Core/Model/App.php

Lines 1431 to 1439 in 977a78f

	$observer = new Varien_Event_Observer([
	'event' => new Varien_Event([
	...$obs['args'], // Default config.xml <args>
	...$args, // Mage::dispatchEvent() $args
	'name' => $eventName,
	]),
	...$obs['args'], // Default config.xml <args>
	...$args, // Mage::dispatchEvent() $args
	]);

Note it's possible to define an arg named event in both config.xml and Mage::dispatchEvent(). Don't do that as it will overwrite the Varien_Event object. This was possible before so I didn't change the behavior.

I'm pretty sure this all works fine with the various ways developers pass Varien_Objects to the observers to get data back. I tried stuff like:

Mage::dispatchEvent('...', ['obj' => new Varien_Object]);

// In observer:
$observer->getObj()->setFoo('123');
$observer->getEvent()->getObj()->setBar('456');

[fballiano]

mmmm I've a ton of code that I've wrote without the getEvent() (I never understood why the data was duplicated so I chose the shortest notation to get the data) and it would also be very tricky for people to understand the change. is it worth it?

[justinbeaty]

mmmm I've a ton of code that I've wrote without the getEvent() (I never understood why the data was duplicated so I chose the shortest notation to get the data) and it would also be very tricky for people to understand the change. is it worth it?

I don't understand why the data is duplicate either, other than $observer and $observer->getEvent() are different classes. But there should be no breaking change here. You can still access data both ways.

[fballiano]

ah ok ok it you can still access it both way then :-)

[justinbeaty]

ah ok ok it you can still access it both way then :-)

Yeah, unless there's some caveat on object reference that breaks when using the splat ... operator, there shouldn't be anything breaking here and we can merge as is.

[fballiano]

tested with a couple of modules that use events and couldn't find any problem, let's merge :-)