Large refactor and new features #6
Conversation
|
Ping @sreichel, let me know what you think. |
|
Actually, I have an idea that might make this all easier. Will test soon... |
|
@justinbeaty man, you r crazy 🚀 I'll test as fast as possbile. G R E A T 😄 |
|
Testing with OpenMage/magento-lts#4405 ... and looks rly good. 😄 |
|
Found a bug ... Thes methods belog to ... |
|
@sreichel It's not a bug. $store = $event->getDataObject();
\PHPStan\dumpType($store);
# Before: Dumped type: mixed
# After: Dumped type: Varien_ObjectBecause previously the plugin didn't read /**
* @method Varien_Object getDataObject()
*/
class Mage_Index_Model_Event extends Mage_Core_Model_Abstract
{
// ...This fixes it: $entity = $event->getEntity();
if ($entity == Mage_Core_Model_Store::ENTITY) {
/** @var Mage_Core_Model_Store */
$store = $event->getDataObject();
// ...
} elseif ($entity == Mage_Core_Model_Store_Group::ENTITY) {
/** @var Mage_Core_Model_Store_Group */
$storeGroup = $event->getDataObject();
// ...
}Edit: it also works to do this as long as we only use methods defined on both types: /**
* @method Mage_Core_Model_Store|Mage_Core_Model_Store_Group getDataObject()
*/
class Mage_Index_Model_Event extends Mage_Core_Model_Abstract
{
// ...Some other ideas to solve things you reported in OpenMage/magento-lts#687 "How to deal with observers?"
/** @phpstan-type MyEventType Varien_Object<loaded: bool, forward_module: string, ...> */And then import it in the observer with: /** @phpstan-import-type MyEventType from Mage_Core_Whatever_Class_Defined_It */But it needs to support custom Object Shapes for Varien_Object class. I also had the idea to do something like if ($someCondition) {
$customer = Mage::getModel('customer/customer');
} else {
/** @var Varien_Object<Mage_Customer_Model_Customer> */
$customer = new Varien_Object();
}
$customer->setFirstname('foo');
$customer->getFirstname(); // string instead of mixed |
|
You are right. Not a bug - its resolved correct. Ummm. Nice.
I think "custom" is the problem .... I tried with stubs, but stubs for code you control ... meh. The best solution ...imho ... would be to add pseudo classes that extend Varien_Object. Every observer passes its own class ... not only VO. |
It is possible using Custom PHPDoc Type Extension, this is how I supported |
|
What does takes more effort? Adding ObjectShapes or add classes - with real methods? In both cases you have to define the methods. Using real methods take more code, but see,s most clean to me. |
|
I think object shapes are better because they're more versatile. With real classes, how do you solve this? if ($someCondition) {
$customer = Mage::getModel('customer/customer');
} else {
// $customer = new Varien_Object();
$customer = new Mage_Customer_MockModel_Customer() // ??
}You can't have So, you'd have to keep both classes in sync with the same the Also, what about this? function doFoo($obj) {
$obj->getSomeValue(); // what is the type?
}
$foo = new Varien_Object();
$foo->setSomeValue('bar');
doFoo($foo);Do you create a new class The idea with object shape is that if you say |
if ($someCondition) {
$customer = Mage::getModel('customer/customer');
} else {
// $customer = new Varien_Object();
$customer = new Mage_Customer_MockModel_Customer() // ??
}Theoreticaly ... shouldnt that be something like
For observer, yes. (?) But that may require two classes ...
|
You'd have to have this: function getCustomer(): Mage_Customer_Model_Customer|Mage_Customer_MockModel_Customer
{
if ($someCondition) {
$customer = Mage::getModel('customer/customer');
} else {
$customer = new Mage_Customer_MockModel_Customer();
}
}But to me it's just cleaner to have: function getCustomer(): Mage_Customer_Model_Customer|Varien_Object<Mage_Customer_Model_Customer>
{
if ($someCondition) {
$customer = Mage::getModel('customer/customer');
} else {
/** @var Varien_Object<Mage_Customer_Model_Customer> */
$customer = new Varien_Object();
}
}And then PHPStan knows that if you call |
|
Its cleaner, but i dont know if it saves much lines of code. Btw ... Php7 has no union types :( (one reason for me to drop php7-support) |
It does though, because you don't need to create (hundreds?) of new classes to support every single event. I feel like real classes would also overcomplicate an already obscure event system in Magento.
Doesn't PHPStan still support it in |
|
Btw, see here: OpenMage/magento-lts#4411 In the Maho version of the plugin I already removed the whole |
|
Hi guys |
|
First we should finish this :) As long you have time to maintain it, it should stay here ... imho. |
|
The code can definitely stay here, just didn't know if it was too big of a change. The main difference between this PR, and the one in OpenMage/magento-lts#4411 is that I removed the two different "modes", i.e. This gets rid of the This means that running PHPStan can connect to the database if I also removed all of the autoloader stuff from the plugin in 4411 as PHPStan doesn't need an autoloader to work, it just scans the source files to find the appropriate classes. However we do need an autoloader to load and run 4411 also adds in support for some other dynamic return types, but that also required adding in some new methods to This is why I said that the new plugin wouldn't exactly be compatible with non-OM codebases, but the plugin could check the existence of some of these methods to not break backwards compatibility. I can update this PR with the newest code, but before I do that you can browse the new plugin here: https://github.com/justinbeaty/maho-phpstan-plugin/tree/topic-v3 |
|
@justinbeaty I'm ok with bigger refactorings and removing options which doesn't make sense now. |
justinbeaty
force-pushed
the
topic-om
branch
from
00f0bc6 to
c8ff866
Compare
|
@justinbeaty i'm testing it on latest OM w/o 4411 ... |
You must use 4411, or we can try to add: case 'Mage::getResourceHelper':
case 'Mage_Core_Model_Config::getResourceHelper':
case 'Mage_Core_Model_Config::getResourceHelperInstance':
return method_exists($this->getConfig(), 'getResourceHelperClassName')
? fn (string $alias) => $this->getConfig()->getResourceHelperClassName($alias)
: null; |
|
Pushed the change, not tested yet though. I'm trying to figure out the Varien_Date thing though... |
|
Idk what the Varien_Date error is. Maybe it's fixed with phpstan v2? |
|
It worked before your todays changes. I was just about to compare with topic-v3. |
On comes from ... Last lines from call stack ... |
Similiar stacktrace ... ... but the file is public function toOptionArray()
{
return [
['value' => 'text', 'label' => Mage::helper('eav')->__('Text Field')],
['value' => 'textarea', 'label' => Mage::helper('eav')->__('Text Area')],
['value' => 'date', 'label' => Mage::helper('eav')->__('Date')],
['value' => 'boolean', 'label' => Mage::helper('eav')->__('Yes/No')],
['value' => 'multiselect', 'label' => Mage::helper('eav')->__('Multiple Select')],
['value' => 'select', 'label' => Mage::helper('eav')->__('Dropdown')],
];
}Somehow the error comes from Name it "image", "crypt" or like another Varien_ class and error is back. |
I created a new PHPStan plugin, and I thought it would be nice for OM and Maho to be on the same page with regards to fixing errors. This is a rather large PR... I originally tried up using the Maho plugin on OM, but it doesn't work due to the differences in autoloaders, etc. I am making this PR with absolutely no licensing restrictions, so feel free to adapt as you see fit if the PR is too large or different than how things currently are.
With this plugin, on OM 20.0 branch I am seeing:
-76 false positive errors gone from the baseline
+589 new errors
There are three main areas of improvement:
Varien Object Handling
Currently, magic methods always returned mixed even when there is a
@methodannotation:Due to the magic methods always returning mixed, PHPStan doesn't detect certain errors:
Magic methods had no argument validation:
Without a
@methodannotation:There is also a new
enforceMagicMethodDocBlockoption that can be turned on eventually, requiring all classes that extend Varien_Object to define@methodannotations.Return Type Detector
Return type detector only supported literal strings as the argument:
New code even detects multiple possibilities:
Better invalid class detection:
Support for getResourceSingleton:
Template File
$thisReflection:Even though we use
@var ... $thisannotations in .phtml files, before we would get an error if we try to call protected/private methods or properties. Not many block classes have protected methods, but some do...