New PHPStan Plugin Test

[justinbeaty]

Description (*)

This is test to use a new PHPStan plugin with OM instead of radically changing the macopedia one. Presumably you'd fork the module into a new namespace.

Some notable changes:

• I cleaned up a lot of the Mage::getModel() etc methods in app/Mage.php, Mage_Core_Model_Config, Mage_Core_Model_Layout, etc classes. I did change the ordering of some of those methods, so please use split diff view.

• Mage::helper('foo') now returns false instead of throwing an error. This is technically a breaking change, but in reality it's okay because you'd just get an error on the next line of code where you try to call some method on that helper. This is in line with all of the other methods like Mage::getModel() which can return false.

• I removed Mage_Core_Model_Config::getModuleSetup() method. It was from a really old Magento version and not applicable anymore.

• I removed some empty string default params, so Mage::getModel($modelClass = '', $arguments = []) is now Mage::getModel($modelClass, $arguments = []). Because calling getModel with no value just returns false.

• I added some @throws annotations.

Related Pull Requests

macopedia/phpstan-magento1#6

Manual testing scenarios (*)

1. Checkout the branch and composer install. Then run phpstan.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[sreichel]

• I removed some empty string default params, so Mage::getModel($modelClass = '', $arguments = []) is now Mage::getModel($modelClass, $arguments = []). Because calling getModel with no value just returns false.

Please not on plublic methods.

https://onlinephp.io/c/d4544

[justinbeaty]

• I removed some empty string default params, so Mage::getModel($modelClass = '', $arguments = []) is now Mage::getModel($modelClass, $arguments = []). Because calling getModel with no value just returns false.

Please not on plublic methods.

https://onlinephp.io/c/d4544

You have it backwards. It's okay to remove the empty string default value on the base class:

https://onlinephp.io/c/c0165

[sreichel]

Mhh, no checks on draft PR?

[sreichel]

This is test to use a new PHPStan plugin with OM instead of radically changing the macopedia one. Presumably you'd fork the module into a new namespace.

@tmotyl s plugin work well for years. Id prefer to follow yor PR there and update it - instead of replacing.

[justinbeaty]

This is test to use a new PHPStan plugin with OM instead of radically changing the macopedia one. Presumably you'd fork the module into a new namespace.

@tmotyl s plugin work well for years. Id prefer to follow yor PR there and update it - instead of replacing.

I can update the PR there with the latest updates. But I didn’t know how @tmotyl feels about such a big change to the codebase.

The reason I have changes in this PR to Mage, Mage_Core_Model_Config, etc is to enable more features in the PHPStan plugin. So the new plugin won’t work with non-OM codebases. Will this be a problem? If the plugin gets released as a new major version then it’s okay, but going forward?

[sreichel]

to enable more features in the PHPStan plugin. So the new plugin won’t work with non-OM codebases. Will this be a problem?

I have (roughly) testesd your PR at macppedia plugin and i looked good.

The changes made here are to "enable more features for the plugin" or the "plugin won’t work with non-OM codebases"?

[sreichel]

If the plugin gets released as a new major version then it’s okay, but going forward?

Going forward? dont know. ists on @tmotyl. Maybe moving it to OM namespace?

[sreichel]

@justinbeaty we still need support for php7.4 :(

[justinbeaty]

Hm, I found an error. Go to Admin Dashboard > Promotions > Catalog Price Rules and edit any rule. You get error: Invalid block type: rule/conditions.

This is because of an instanceof Mage_Core_Block_Abstract check but the block doesn't extend it:

class Mage_Rule_Block_Conditions implements Varien_Data_Form_Element_Renderer_Interface
{

We can revert the behavior with the following patch, but the phpdoc is wrong because it doesn't always return Mage_Core_Block_Abstract.

diff --git a/app/code/core/Mage/Core/Model/Layout.php b/app/code/core/Mage/Core/Model/Layout.php
index fe27d08466..1acfa2ea6e 100644
--- a/app/code/core/Mage/Core/Model/Layout.php
+++ b/app/code/core/Mage/Core/Model/Layout.php
@@ -590,18 +590,25 @@ class Mage_Core_Model_Layout extends Varien_Simplexml_Config
     /**
      * @param string $type
      * @return Mage_Core_Block_Abstract
      * @throws Mage_Core_Exception
      */
     public function getBlockSingleton($type)
     {
         if (!isset($this->_helpers[$type])) {
-            $helper = $this->_getBlockInstance($type);
-            $helper->setLayout($this);
+            $className = Mage::getConfig()->getBlockClassName($type);
+            if ($className === false || !class_exists($className)) {
+                Mage::throwException(Mage::helper('core')->__('Invalid block type: %s', $type));
+            }
+            // phpcs:ignore Ecg.Classes.ObjectInstantiation.DirectInstantiation
+            $helper = new $className();
+            if ($helper instanceof Mage_Core_Block_Abstract) {
+                $helper->setLayout($this);
+            }
             $this->_helpers[$type] = $helper;
         }
         return $this->_helpers[$type];
     }
 
     /**
      * Retrieve helper object
      *