search

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
863 stars 438 forks source link

Add new events for better integration with spatie-ignition #3957

Closed empiricompany closed 2 months ago

empiricompany commented 2 months ago

Description (*)

Add 3 new events around print exception and error handler to better integrate spatie-ignition via observer. I have created an external module: composer require empiricompany/openmage_ignition

Related Pull Requests

https://github.com/OpenMage/magento-lts/discussions/3955 https://github.com/OpenMage/magento-lts/pull/3954

Fixed Issues (if relevant)

Manual testing scenarios (*)

  1. checkout the PR or patch openmage/magento-lts
        "patches": {
            "openmage/magento-lts": {
                "Add events for integration with spatie-ignition #3957": "https://github.com/OpenMage/magento-lts/pull/3957.patch"
            }
        }

  1. install package: composer require empiricompany/openmage_ignition

  2. enable developer mode to print errors and exceptions

Questions or comments

Contribution checklist (*)

fballiano commented 2 months ago

This has potential, I'd wait for some more development of the module itself and to define a couple of details in the code of this PR, then it may totally replace my implementation

fballiano commented 2 months ago

I'd also backport this to main

fballiano commented 2 months ago

https://github.com/OpenMage/magento-lts/blob/main/docs/EVENTS.md also needs to be updated

fballiano commented 2 months ago

@empiricompany now that mage_run_installed_exception is available, could you test your module using mage_run_installed_exception instead of mage_print_exception_before?

for some reason your module is not working on my instance, I don't understand why but it's not calling the observers

empiricompany commented 2 months ago

i've tested with events:

<mage_run_exception>
    <observers>
        <mm_ignition>
            <class>MM_Ignition_Model_Observer</class>
            <method>handleIgnitionException</method>
        </mm_ignition>
    </observers>
</mage_run_exception>
<mage_run_installed_exception>
    <observers>
        <mm_ignition>
            <class>MM_Ignition_Model_Observer</class>
            <method>handleIgnitionException</method>
        </mm_ignition>
    </observers>
</mage_run_installed_exception>

and work the same as with mage_print_exception_before for exception but...

I encountered another issue, I can't override the default PHP error handler by dispatching this event core_app_init_environment_after in Mage_Core_Model_App::_initEnvironment() because the module is not ready at this point. so for example for a php error on undefined method like Mage_Catalog_CategoryController::nofunc() not works

instead with your hardcoded methods still works

    /**
     * Initialize PHP environment
     *
     * @return $this
     */
    protected function _initEnvironment()
    {
        if (Mage::getIsDeveloperMode() && class_exists('\Spatie\Ignition\Ignition')) {
            \Spatie\Ignition\Ignition::make()
                ->applicationPath(Mage::getBaseDir())
                ->register();            
        } else {
            $this->setErrorHandler(self::DEFAULT_ERROR_HANDLER);
        }

This is a big problem that I currently don't know how to solve.

I've also tried to add a depend but not help:

<?xml version="1.0"?>
<config>
    <modules>
        <MM_Ignition>
            <active>true</active>
            <codePool>community</codePool>
            <depends>
                <Mage_Core />
            </depends>
        </MM_Ignition>
    </modules>
</config>
empiricompany commented 2 months ago

These are the dispatched events:

2024-05-01T15:15:28+00:00 DEBUG (7): resource_get_tablename
2024-05-01T15:15:28+00:00 DEBUG (7): resource_get_tablename
2024-05-01T15:15:28+00:00 DEBUG (7): resource_get_tablename
2024-05-01T15:15:28+00:00 DEBUG (7): core_collection_abstract_load_before
2024-05-01T15:15:28+00:00 DEBUG (7): core_collection_abstract_load_after
2024-05-01T15:15:28+00:00 DEBUG (7): core_collection_abstract_load_before
2024-05-01T15:15:28+00:00 DEBUG (7): core_collection_abstract_load_after
2024-05-01T15:15:28+00:00 DEBUG (7): core_collection_abstract_load_before
2024-05-01T15:15:28+00:00 DEBUG (7): core_collection_abstract_load_after
2024-05-01T15:15:28+00:00 DEBUG (7): controller_front_init_before
2024-05-01T15:15:28+00:00 DEBUG (7): controller_front_init_routers
2024-05-01T15:15:28+00:00 DEBUG (7): resource_get_tablename
2024-05-01T15:15:28+00:00 DEBUG (7): resource_get_tablename
2024-05-01T15:15:28+00:00 DEBUG (7): model_load_after
2024-05-01T15:15:28+00:00 DEBUG (7): core_abstract_load_after
2024-05-01T15:15:28+00:00 DEBUG (7): session_before_renew_cookie
2024-05-01T15:15:28+00:00 DEBUG (7): resource_get_tablename
2024-05-01T15:15:28+00:00 DEBUG (7): resource_get_tablename
2024-05-01T15:15:28+00:00 DEBUG (7): core_locale_set_locale
2024-05-01T15:15:28+00:00 DEBUG (7): core_locale_set_locale
2024-05-01T15:15:28+00:00 DEBUG (7): resource_get_tablename
2024-05-01T15:15:28+00:00 DEBUG (7): resource_get_tablename
2024-05-01T15:15:28+00:00 DEBUG (7): controller_action_predispatch
2024-05-01T15:15:28+00:00 DEBUG (7): controller_action_predispatch
2024-05-01T15:15:28+00:00 DEBUG (7): customer_session_init
2024-05-01T15:15:28+00:00 DEBUG (7): customer_session_init
2024-05-01T15:15:28+00:00 DEBUG (7): controller_action_predispatch_catalog
2024-05-01T15:15:28+00:00 DEBUG (7): controller_action_predispatch_catalog
2024-05-01T15:15:28+00:00 DEBUG (7): controller_action_predispatch_catalog_category_view
2024-05-01T15:15:28+00:00 DEBUG (7): controller_action_predispatch_catalog_category_view
empiricompany commented 2 months ago

I've done some tests, and it seems that we need an event at this point, immediately after initialization init

This is the minimum requirement to be able to log the error with an external module that needs to be initialized, but we potentially lose any errors earlier in the call stack, but I think even with the hard-coded solution.

2024-05-01T15:44:37+00:00 DEBUG (7): resource_get_tablename 2024-05-01T15:44:37+00:00 DEBUG (7): resource_get_tablename 2024-05-01T15:44:37+00:00 DEBUG (7): resource_get_tablename 2024-05-01T15:44:37+00:00 DEBUG (7): core_collection_abstract_load_before 2024-05-01T15:44:37+00:00 DEBUG (7): core_collection_abstract_load_after 2024-05-01T15:44:37+00:00 DEBUG (7): core_collection_abstract_load_before 2024-05-01T15:44:37+00:00 DEBUG (7): core_collection_abstract_load_after 2024-05-01T15:44:37+00:00 DEBUG (7): core_collection_abstract_load_before 2024-05-01T15:44:37+00:00 DEBUG (7): core_collection_abstract_load_after 2024-05-01T15:44:37+00:00 DEBUG (7): core_app_run_init_currentstore 2024-05-01T15:44:37+00:00 DEBUG (7): controller_front_init_before 2024-05-01T15:44:37+00:00 DEBUG (7): controller_front_init_routers

or we can attach to controller_front_init_before without add a new event, i think will be the same

fballiano commented 2 months ago

I'd kinda lean towards using controller_front_init_before then

empiricompany commented 2 months ago

I have changed the events used in my module: https://github.com/empiricompany/openmage_ignition/blob/main/app/code/community/MM/Ignition/etc/config.xml

So, there is no longer need for this PR.

fballiano commented 2 months ago

does your module works 100% also my previous PR about Ignition stays in the core?

empiricompany commented 2 months ago

does your module works 100% also my previous PR about Ignition stays in the core?

From my test module works correcty (I invite you to test it), and since we now have an external module, there's no longer a need for the hard-coded loading of Ignition in the core. So we can close this PR and revert this one: https://github.com/OpenMage/magento-lts/pull/3954.