-
-
Notifications
You must be signed in to change notification settings - Fork 436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new events for better integration with spatie-ignition #3957
Conversation
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 |
I'd also backport this to main |
https://github.com/OpenMage/magento-lts/blob/main/docs/EVENTS.md also needs to be updated |
Co-authored-by: Fabrizio Balliano <[email protected]>
Co-authored-by: Fabrizio Balliano <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that also if we have a couple more events in the printexception it's no big deal, since it should never happen anyway.
the "init" one is necessary for sure in my opinion, much more clear than the controller_predispatch IMHO
so for me this is a yes
ps: i'll backport this to "main" too
@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 |
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 I encountered another issue, I can't override the default PHP error handler by dispatching this event 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> |
These are the dispatched events:
|
I'd kinda lean towards using controller_front_init_before then |
I have changed the events used in my module:
So, there is no longer need for this PR. |
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. |
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
#3955
#3954
Fixed Issues (if relevant)
Manual testing scenarios (*)
openmage/magento-lts
install package:
composer require empiricompany/openmage_ignition
enable developer mode to print errors and exceptions
Questions or comments
Contribution checklist (*)