OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
865 stars 436 forks source link

Method Mage_GiftMessage_Model_Observer::checkoutEventCreateOrder() does not exist #1154

Closed midlan closed 2 years ago

midlan commented 4 years ago

I have created test to test all observers and it screams:

Exception: Method Mage_GiftMessage_Model_Observer::checkoutEventCreateOrder() does not exist (observer: giftmessage; event: adminhtml_sales_order_create_create_order)

Event definition: app/code/core/Mage/GiftMessage/etc/config.xml

            <adminhtml_sales_order_create_create_order>
                <observers>
                    <giftmessage>
                        <type>model</type>
                        <class>giftmessage/observer</class>
                        <method>checkoutEventCreateOrder</method>
                    </giftmessage>
                </observers>
            </adminhtml_sales_order_create_create_order>

And it is right, there is no such method there: https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/GiftMessage/Model/Observer.php

colinmollenhour commented 4 years ago

Nice find! Just delete the XML I suppose..

midlan commented 4 years ago

Seems right.

Looks like the event name is built with fullActionName but createAction never existed in Mage_Adminhtml_Sales_Order_CreateController according my research.

tmotyl commented 4 years ago

@midlan btw can you share your test? It would be nice to get it integrated in openmage.

sreichel commented 4 years ago

@tmotyl I already work on a PR for N98-magerun ...

midlan commented 4 years ago

@tmotyl my test looks is this:

<?php

use Tester\Assert;

/**
 * @testCase
 */
class ObserversTest extends Tester\TestCase {

    public function testObservers() {

        $eventAreas = array(
            Mage_Core_Model_App_Area::AREA_GLOBAL,
            Mage_Core_Model_App_Area::AREA_FRONTEND,
            Mage_Core_Model_App_Area::AREA_ADMINHTML,
        );

        foreach ($eventAreas as $eventArea) {
            $eventConfig = Mage::app()->getConfig()->getNode(sprintf('%s/events', $eventArea));
            if ($eventConfig instanceof Mage_Core_Model_Config_Element) {
                foreach ($eventConfig->children() as $eventName => $event) {
                    foreach ($event->observers->children() as $observerName => $observer) {

                        /** create observer instance @see Mage_Core_Model_App::dispatchEvent */
                        $getInstanceMethod = 'getSingleton'; //singleton is default

                        switch($observer->type ?? '') {
                            case 'disabled':
                                continue 2; //do not check disabled observers

                            case 'object':
                            case 'model':
                            $getInstanceMethod = 'getModel';
                                break;
                        }

                        //check object exists
                        $modelClass = (string) $observer->class;

                        $observerInstance = Mage::{$getInstanceMethod}($modelClass);

                        Assert::type(
                            'object',
                            $observerInstance,
                            "Model '{$modelClass}' was not found. (observer: {$observerName}; event: {$eventName})"
                        );

                        //check method is public
                        $method = (string) $observer->method;

                        try {
                            $reflection = new ReflectionMethod($observerInstance, $method);

                        }
                        catch (ReflectionException $e) {
                            throw new Exception($e->getMessage() . " (observer: {$observerName}; event: {$eventName})");
                        }

                        Assert::true(
                            $reflection->isPublic(),
                            "Method '{$modelClass}::{$method}' is not public. (observer: {$observerName}; event: {$eventName})"
                        );

                    }
                }
            }
        }
    }

}

It is not phpunit but nette/tester; but I think migrations should be pretty easy

midlan commented 3 years ago

https://github.com/OpenMage/magento-lts/pull/1156 @colinmollenhour review pls?

Flyingmana commented 3 years ago

@midlan we could also add a testrun to our github action with nette/tester if this is more comfortable.

midlan commented 3 years ago

Phpunit is more stable (more users, more development). We are using nette-tester because it runs in multiple threads so the tests are finished in less time.

joshua-bn commented 3 years ago

@midlan do you have more tests? This is getting off-topic, but the community would benefit a lot from more tests.

midlan commented 3 years ago

I have lots of tests for our modules, but I have some for core:

tests/unit/app/code/core/Mage/Core/Model/App/ObserversTest.php
tests/unit/app/code/core/Mage/Core/Model/AbstractTest.php
tests/unit/app/code/core/Mage/Core/Model/ConfigTest.php
tests/unit/app/code/core/Mage/Catalog/controllers/CategoryControllerTest.php
tests/unit/app/code/core/Mage/Adminhtml/CatalogCategoryControllerTest.php
tests/unit/app/code/core/Mage/Adminhtml/Block/Widget/GridTest.php
tests/unit/app/code/core/Mage/Adminhtml/TinyMceFindAndReplaceTest.php
tests/unit/app/code/core/Mage/Adminhtml/Model/System/Config/Backend/FilenameTest.php
tests/unit/app/code/core/Mage/Adminhtml/SystemXmlTest.php
tests/unit/app/code/core/Mage/Cms/controllers/IndexControllerTest.php
tests/unit/app/code/core/Mage/CatalogSearch/controllers/ResultControllerTest.php
tests/unit/app/code/core/Mage/Admin/UserTest.php

Maybe we can discus it in discussion?

ADDISON74 commented 2 years ago

Can we close this issue?

midlan commented 2 years ago

yeah, it is fixed: https://github.com/OpenMage/magento-lts/pull/1156