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
869 stars 436 forks source link

protected function _isApplicableAttribute #2829

Closed addison74 closed 1 year ago

addison74 commented 1 year ago

I am running in a test environment with DDEV the latest OM 20 + PHP 7.3 (requested by Composer). I installed an old extension which worked great with PHP 7.0 to update it to 8.2 but OM is not loading some sections that this extension adds to the Backend.

In the browser window I am getting a fatal error like this:

Fatal error: Uncaught TypeError: count(): Argument #1 ($value) must be of type Countable|array, null given in /var/www/html/app/code/core/Mage/Catalog/Model/Resource/Abstract.php:70 

Stack trace: 
#0 /var/www/html/app/code/core/Mage/Eav/Model/Entity/Abstract.php(622): Mage_Catalog_Model_Resource_Abstract->_isApplicableAttribute(Object(Ave_SizeChart_Model_Chart), Object(Ave_SizeChart_Model_Resource_Eav_Attribute)) 

#1 /var/www/html/app/code/core/Mage/Eav/Model/Entity/Abstract.php(1618): Mage_Eav_Model_Entity_Abstract->walkAttributes('backend/afterLo...', Array) 

#2 /var/www/html/app/code/core/Mage/Eav/Model/Entity/Abstract.php(952): Mage_Eav_Model_Entity_Abstract->_afterLoad(Object(Ave_SizeChart_Model_Chart)) 

#3 /var/www/html/app/code/core/Mage/Catalog/Model/Resource/Abstract.php(743): Mage_Eav_Model_Entity_Abstract->load(Object(Ave_SizeChart_Model_Chart), 1, NULL) 

#4 /var/www/html/app/code/core/Mage/Core/Model/Abstract.php(291): Mage_Catalog_Model_Resource_Abstract->load(Object(Ave_SizeChart_Model_Chart), 1, NULL)

#5 /var/www/html/app/code/community/Ave/SizeChart/controllers/Adminhtml/Sizechart/ChartController.php(42): Mage_Core_Model_Abstract->load(1) 

#6 /var/www/html/app/code/community/Ave/SizeChart/controllers/Adminhtml/Sizechart/ChartController.php(86): Ave_SizeChart_Adminhtml_Sizechart_ChartController->_initChart() 

#7 /var/www/html/app/code/core/Mage/Core/Controller/Varien/Action.php(429): Ave_SizeChart_Adminhtml_Sizechart_ChartController->editAction() 

#8 /var/www/html/app/code/core/Mage/Core/Controller/Varien/Router/Standard.php(256): Mage_Core_Controller_Varien_Action->dispatch('edit') 

#9 /var/www/html/app/code/core/Mage/Core/Controller/Varien/Front.php(190): Mage_Core_Controller_Varien_Router_Standard->match(Object(Mage_Core_Controller_Request_Http)) 

#10 /var/www/html/app/code/core/Mage/Core/Model/App.php(373): Mage_Core_Controller_Varien_Front->dispatch() 

#11 /var/www/html/app/Mage.php(739): Mage_Core_Model_App->run(Array) 

#12 /var/www/html/index.php(71): Mage::run('', 'store') 

#13 {main} thrown in /var/www/html/app/code/core/Mage/Catalog/Model/Resource/Abstract.php on line 70

Based on the error I checked the /app/code/core/Mage/Catalog/Model/Resource/Abstract.php file on line 70. Here is the PHP code:

    /**
     * Check whether the attribute is Applicable to the object
     *
     * @param Varien_Object $object
     * @param Mage_Catalog_Model_Resource_Eav_Attribute $attribute
     * @return boolean
     */
    protected function _isApplicableAttribute($object, $attribute)
    {
        $applyTo = $attribute->getApplyTo();
        return count($applyTo) == 0 || in_array($object->getTypeId(), $applyTo);
    }

If I change it as follows

        $applyTo = array($attribute->getApplyTo());

the fatal error disappears and there are no more errors in /var/log/system.log file from now on like I got with PHP 7.3.

As you may see I am editing an OpenMage file to make this extension working properly. Do you consider this an issue into the OM code related to new PHP versions we need to take in consideration? Maybe PHPStan could solve this mystery but I am not an expert, honestly speaking I never used it.

addison74 commented 1 year ago

I am starting to consider that it's not the extension that's to blame for the error but the OpenMage code related to the PHP version used.

7.0: is running fine. no errors appear in the system.log and in the browser window 7.2, 7.3, 7.4: all are running but errors appear in the system.log

Warning: count(): Parameter must be an array or an object that implements Countable in /var/www/html/app/code/core/Mage/Catalog/Model/Resource/Abstract.php on line 71

8.0: is not running. the fatal error appears in the browser, nothing in the system.log

I wanted to find out the value of the $applyTo variable. I insert var_dump()and this is what it displays in browser

Frontend: array(0) { } Backend: NULL

If I convert the variable to array this is what appears in browser

Frontend: array(1) { [0]=> array(0) { } } Backend: array(1) { [0]=> NULL }

It is obvious that using the count() function in these conditions will generate an error. Searching the Internet for the fatal error I found a lot of pages where the solution is to convert the type to array. Once done in the OpenMage code, the error disappear and extension works without issues.

addison74 commented 1 year ago

Here is what appears in system.log with PHP 8.2 when loading the product page in Frontend

2022-12-21T10:41:32+00:00 ERR (3): Warning: Invalid argument supplied for foreach()  in /var/www/html/app/design/frontend/rwd/default/template/catalog/product/view/media.phtml on line 42
2022-12-21T10:41:32+00:00 ERR (3): Warning: count(): Parameter must be an array or an object that implements Countable  in /var/www/html/app/design/frontend/rwd/default/template/catalog/product/view/media.phtml on line 53

Changing in both lines from

$this->getGalleryImages()

to

 (array)$this->getGalleryImages()

the errors disappear from system.log. In my case it's value was NULL having no product images loaded.

It seems this error is a known one. I certainly wouldn't have found out about it if I hadn't tested this extension. There are a lot of people asking for help in the last year in different PHP forums. Here is one resource

https://stackoverflow.com/questions/53020833/count-parameter-must-be-an-array-or-an-object-that-implements-countable-in-lar

addison74 commented 1 year ago

Let's execute the code bellow on https://onlinephp.io/

<?php
$variableTest = NULL;
var_dump(count($variableTest));

PHP 7.0.33, 7.1.33

int(0)

PHP 7.2.34, 7.3.33, 7.4.33

Warning: count(): Parameter must be an array or an object that implements Countable in /home/user/scripts/code.php on line 3
int(0)

PHP > 8.0

Fatal error: Uncaught TypeError: count(): Argument #1 ($value) must be of type Countable|array, null given in /home/user/scripts/code.php:3
Stack trace:
#0 {main}
  thrown in /home/user/scripts/code.php on line 3

Because the minimum requirements of the OM are already for PHP set to version 7.3 I would propose to modify the OpenMage code in the case of the count() function to deal specifically with the situation when a variable has the NULL value.

Using array($variable) is not quite suitable as a solution but one just to see an immediate effect, although if it is NULL it becomes an empty array. Also, the replacement with !empty($variable) or isset($variable) doesn't seem right to me.

In PHP 7.3 the is_countable() function was introduced, but we already have from end of May a polyfill introduced here https://github.com/OpenMage/magento-lts/pull/2163. I would propose that the change be as follows

count($variable) => count((is_countable($variable) ? $variable : [] ))

We can also check the variable directly in an if condition with !is_null($variable). I searched OM for this topic and I found a few discussions and PRs but not fixing this one.

fballiano commented 1 year ago

maybe some module is overriding the getGalleryImages() method or something like that?

addison74 commented 1 year ago

At this moment the extension is disabled by editing its module file. I was able to reproduce this issue pretty simple, by renaming the media directory then creating an empty one. OM thinks the products have images but there are not physically accessible. This issue is not related to the one reported here, I discovered it by chance.

Using count function with NULL values is really an important issue. Even we fix this one there a lot in the code. Now I realize that many do not upgrade the operating system and for this reason we do not have reports, but when they will do it we will definitely identify others count related issues.

sreichel commented 1 year ago

I was able to reproduce this issue pretty simple, by renaming the media directory then creating an empty one. OM thinks the products have images but there are not physically accessible.

Cant reproduce it with 1.9.4.x / PHP8.1

When media directory is empty and i go to product page, placeholder images are added.

addison74 commented 1 year ago

No problem. I will spend some time on this issue. I have in plan more tests.

sreichel commented 1 year ago

I wanted to find out the value of the $applyTo variable

Can you add a xdebug breakpoint there an post stacktrace? (or share code?)

addison74 commented 1 year ago

When I last checked the extension it was in PHP 7.0.33 and it was functional, but since then almost 1 year has passed and the developer from Ukraine no longer responds. I hope with all my heart that he is in good health because surely the war started by an idiot either created problems for him or led him to war.

sreichel commented 1 year ago

I hope with all my heart that he is in good health because surely the war started by an idiot either created problems for him or led him to war.

:heart:

@Flyingmana https://github.com/OpenMage/Web_Notifications/pull/4 ... not?

addison74 commented 1 year ago

I used XDebug together with the extension and the value of the $applyTo variable is null. Using the null value in the count() function creates trouble starting with PHP 7.2 (warnings in system.log) and PHP 8.0 (fatal error in browser). For this reason I would insert a line before return to prevent this particular case, if the value is null then I set it to an empty array.

In the OM file /app/code/core/Mage/Catalog/Model/Resource/Abstract.php in the _isApplicableAttribute method (line 68) here is the change

    /**
     * Check whether the attribute is Applicable to the object
     *
     * @param Varien_Object $object
     * @param Mage_Catalog_Model_Resource_Eav_Attribute $attribute
     * @return bool
     */
    protected function _isApplicableAttribute($object, $attribute)
    {
        $applyTo = $attribute->getApplyTo();
        if(is_null($applyTo)) $applyTo = [];
        return count($applyTo) == 0 || in_array($object->getTypeId(), $applyTo);
    }

The use of the is_countable() function is not appropriate in this case. With my change the extension works, there are no more warnings or errors.

sreichel commented 1 year ago

After some investigation ...

IMHO ... this problem comes from this extension. (edit: and OM)

$attribute->getApplyTo() return null b/c $attribute is not instance of Mage_Catalog_Model_Resource_Eav_Attribute as docs say. In this case Mage_Catalog_Model_Resource_Eav_Attribute::getApplyTo() would return an array, but for this extension $attribute is ...

class Ave_SizeChart_Model_Resource_Eav_Attribute extends Mage_Eav_Model_Entity_Attribute, not having getApplyTo() method and calling Varien_Object getter.

Adding this to Ave_SizeChart_Model_Resource_Eav_Attribute seems to fix it (stopped here with testing, at least install works for now)

    /**
     * @return array
     */
    public function getApplyTo()
    {
        return [];
    }

edit: added PR