Magento CLI 2.4.0 - ErrorHandler is turning PHP errors into exceptions in production mode

Question

Preface: I prefer to develop in developer mode, have Magento catch all PHP E_NOTICE|E_WARNING|etc. errors, throw exception on them, and get these issues cleaned up before committing my code. The issue however resides in a third party extension, and I intend to report the problem to them.

Problem:

As the title states, Magento is still converting PHP level errors to exceptions in Magento\Framework\App\ErrorHandler::handler while in production mode. As part of my current development efforts, I am having to rely on a third party extension, and they have a logical bug which is causing the following E_NOTICE level error: Notice: Trying to access array offset on value of type null in /path/to/project/vendor/magento/module-catalog-import-export/Model/Import/Product.php on line 1281.

Anyone familiar with this particular area of code, may feel compelled to state that the bug needs to be cleaned up or else the import of products won't work due to a lack of data on the row's type_id and _attribute_set. Please know this is not the case, as further on in the third party extension, the third party module will properly set these important keys prior to actual import into the db.

Back to my issue at hand: Due to this problem in the third party extension, I am unable to test running imports locally to completion while in developer mode, because (as expected) Magento will convert this PHP E_NOTICE level error into an exception. Causing the import to fail. Therefore, locally, I have decided to switch Magento to production mode so as to prevent these PHP errors from being converted to exceptions. Unfortunately this does not seem to have solved the issue, and ErrorHandler continues to throw exceptions for this problems while Magento is in production mode.

I have decided to take a closer look at Magento's code, and I myself cannot seem to determine how (production mode) Magento is supposed to avoid throwing exceptions for PHP errors in the first place. Stepping through the code I can see that:

1. bin/magento will be the entry point of execution, as I am running the import from CLI
2. bin/magento will perform an include on app/bootstrap.php
3. bootstrap.php immediately sets PHP's error reporting level to E_ALL
4. bin/magento proceeds to set PHP's error handler to \Magento\Framework\App\ErrorHandler
5. bin/magento creates a Symfony console application and runs.

Because app/bootstrap.php sets the error reporting level to E_ALL, regardless what the current reporting level is, I can only assume that somewhere along the execution stack Magento would have to consult the current application mode. If running as "production" perform something along the lines of error_reporting(E_ALL & ~E_NOTICE & ~E_WARNING ...) so as to prevent production mode from throwing exceptions for such errors. However nowhere have I been able to find such logic. Therefore I am at a loss as to how production mode typically prevents the ErrorHandler from converting these PHP errors into exceptions in the first place.

If I directly modify ErrorHandler.php, just before the conditional which returns false, adding the following:

echo "errNo: $errorNo\nerror_reporting: ". error_reporting() . "\nbitwise result: " . ($errorNo & error_reporting()) . "\nE_ALL: " . E_ALL . "\n";

I get the following output in the console:

errNo: 8
error_reporting: 32767
bitwise result: 8
E_ALL: 32767

Therefore the return false; is never met, and the code proceeds to convert the E_NOTICE level error into an exception. If directly before this line: $errorNo = $errorNo & error_reporting(); I add the following: error_reporting(E_ALL & ~E_NOTICE); the error handler will return false as expected. Obviously this is not a solution, but what I have now temporarily resorted to in order to continue progress on my current task.

Some additional things to note:

• I have tried passing to the PHP binary -d display_errors=0, which yields no effect (as PHP will still report errors for the error_log)
• I have tried passing to the PHP binary -d log_errors=0, which yields no effect
• I have tried passing to the PHP binary -d error_reporting=0, which obviously yields no effect due to Magento's bootstrap.php changing it to E_ALL.
• I have tried all the above individually, as well as at once.
• I have flushed cache, as well as disabled cache, as well as blown away var/cache and generated/code
• I have blown away my entire application (backing up app/etc/env.php first), proceeded to perform a hard reset from HEAD, execute composer install to fetch dependencies fresh, so as to wipe away anything I may have missed that is cached, which yields no effect (restored env.php and re-compile production mode)
• Cache is currently configured to the out of the box filesystem cache under Magento's var/cache directory

Last few notes:

• For the most part this is still nearly an out of the box installation of Magento 2.4.0. There has been little in terms of customization done to it, certainly not any revising of the error_reporting() level by myself (or other devs, as I am the only dev touching the code at this point).
• The third party extension is firebear/importexport version 3.4.3. Review of this extension does not show that it is changing the error_reporting() level either.
• php version: 7.4.9

TL;DR: I don't know why Magento production mode continuing to allow the ErrorHandler to throw exceptions for minor PHP errors.

EDIT 2020.10.07

Response to FactoryAidan's answer: (doesn't cover original question, but explains why the E_NOTICE level error happens in the first place)

It is my personal opinion that occurrence of the the E_NOTICE level error is primarily to blame due to a logic bug in Firebear_ImportExport, as opposed to a logical bug in core Magento code. Let me also prefix this the following with a re-iteration that I agree with your statement "had Magento core code checked if null was returned from $this->skuProcessor->getNewSku($lastSku), this wouldn't be a problem in the first place when using Firebear's extension.

Now, as to my reasoning why I still point the finger at Firebear for this (included will also be some criticism towards Magento as well, but main problem here is with Firebear not following what Magento has done).

In Magento 2.4.0, if we look at \Magento\CatalogImportExport\Model\Import\Product lines 2481- 2488 we'll find the following:

if ($this->isSkuExist($sku) && Import::BEHAVIOR_REPLACE !== $this->getBehavior()) {
    // can we get all necessary data from existent DB product?
  check for supported type of existing product
    if (isset($this->_productTypeModels[$this->getExistingSku($sku)['type_id']])) {
        $this->skuProcessor->addNewSku(
            $sku,
            $this->prepareNewSkuData($sku)
        );

Verbalizing this logic, we can see that the code is doing the following (omitting non-important aspects):

• If the sku already exists
• If the existing sku's type_id is valid
• Then add this existing sku to the SkuProcessor's array of new skus.

-- Wait what? add the existing to the new skus array? why it is not new, we literally just confirmed that it already exists. (this is where my criticism towards Magento lies.

Picking through the methods invoked here, we essentially come to the conclusion that: "Magento is ensuring that when updating existing skus, we are not allowed to modify the type_id and the attribute_set_id". Magento goes about this in a somewhat odd way, by leaning on \Magento\CatalogImportExport\Model\Import\Product::_prepareNewSkuData() to build an array that has type_id and attr_set_code values from the existing, passing this array to the SkuProcessor as a "new sku" despite it actually being data of an existing.

Then, through these shenanigans, Magento has now tricked tricked itself to always specifically set the type_id and attr_set_code within the \Magento\CatalogImportExport\Model\Import\Product::_prepareRowForDb() method when handling existing skus.

As for firebear? They've overridden \Magento\CatalogImportExport\Model\Import\Product, and these lines 2481-2488 of the original model don't ever execute. Firebear (intentionally, per their documentation on their website) allows for changing of both type_idandattr_set_code` on existing products. Which, in my opinion, is fine. However, after a product has passed initial validation they neglect to do this same "add the existing sku data, to the array of new skus", and this is why the E_NOTICE level error now occurs.

Maybe they intentionally didn't do this, so that the type_id and attr_set_code aren't "reset" by the native _prepareRowForDb() method. However even if this is the case, it would be really simple for them simply call the parent _prepareRowForDb method, and afterwards set these two array key values with data provided from the import.

Ultimately, my solution was to take over the Model\Import\Product class once again, extending Firebear's, which extends Magento's. Here I revise the error reporting level prior to calling parent::_prepareRowForDb() the set the error reporting level back to it's original value afterwards.

My source code:

    /***
     * E_NOTICE level error is breaking import on updates of existing products
     * because \Magento\Framework\App\ErrorHandler is catching the error
     * and converting it into an exception.
     *
     * This error is occurring due to a logical bug in firebear/importexport
     *
     * Native magento/module-catalog-import-export will invoke
     * \Magento\CatalogImportExport\Model\Import\Product\SkuProcessor::addNewSku()
     * for rows that have passed validation, when the import behavior update.
     *
     * firebear/importexport does not, and so the SkuProcessor returns
     * null to \Magento\CatalogImportExport\Model\Import\Product::_prepareRowForDb()
     * when updating existing SKUs. Immediately afterwards, this error occurs:
     *
     * Notice: Trying to access array offset on value of type null in
     * vendor/magento/module-catalog-import-export/Model/Import/Product.php on line 1281
     *
     * Combinations of package versions that first discovered to yield this logic error:
     *      Magento Open Source 2.4.0
     *          firebear/importexport: 3.4.3
     *          magento/module-catalog-import-export: 101.1.0
     *
     * {@inheritDoc}
     * @see \Firebear\ImportExport\Model\Import\Product::_prepareRowForDb()
     * phpcs:disable PSR2.Methods.MethodDeclaration.Underscore
     */
    protected function _prepareRowForDb(array $rowData): array
    {
        $level = error_reporting();
        if (E_NOTICE & $level === 0) {
            return parent::_prepareRowForDb($rowData);
        }

        error_reporting($level & ~E_NOTICE);

        $rowData = parent::_prepareRowForDb($rowData);

        error_reporting($level);

        return $rowData;
    }

Answer 1

🔥 I'm also using Magento 2.4.0 and Firebear.

⚠️ This is a result of a Magento Core bug in:

• vendor/magento/module-catalog-import-export/Model/Import/Product.php::_prepareRowForDb

where:

• Line 1280: $this->skuProcessor->getNewSku($lastSku) can have a return type of array|null but null is not handled.

✅ Use the following .patch file on your existing installation which will update the bugged core module:

• M2.4.0 Patch to handle both array|null return types from getNewSku()

To install the patch with composer.json, see the first comment in gist linked ☝️.

ℹ️ This solution has successfully passed tesing in Magento Enterprise Cloud Edition environments.