Skip to content
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

Allowed/accepted changes for phpstan compatibility? #687

Closed
sreichel opened this issue Jun 1, 2019 · 2 comments
Closed

Allowed/accepted changes for phpstan compatibility? #687

sreichel opened this issue Jun 1, 2019 · 2 comments
Labels

Comments

@sreichel
Copy link
Contributor

sreichel commented Jun 1, 2019

#365

To make phpstan (or PHPStorm) work properbly, you have to ...

  • add annotions to every method
  • add magic methods to class annotation
  • change return types in some Varien classen (Varien_ to $this)

I guess this could be merged w/o problems, but there are some other things that need to be changed ... and I don't know what will be accepted.

1. Magic methods on Varien_Object

I started to change it to setter/getter, but this would cause 1000s of changes .... no. Other idea is to add some pseudo classes that extend Varien_Object. See 088d028

2. How to deal with observers?

Since getEvent() isn't defined it could return anything ... like ->getEvent()->getProduct(). Change to getter/setter as started doen't resolve child methods ("cannot find declaration"). Useless changes ...

Idea to fix it ...

Example: autocomplete/checks for controller_action_noroute event:

/**
 * Modify No Route Forward object
 *
 * @param Varien_Event_Observer$observer
 * @return $this
 */
public function noRoute(Varien_Event_Observer $observer)
{
    $observer->getEvent()->getStatus()
        ->setLoaded(true)
        ->setForwardModule('cms')
        ->setForwardController('index')
        ->setForwardAction('noRoute');
    return $this;
}
  1. we need to define a return type for observers $observer->getEvent(). So add a pseudo class that just contains the "correct" new definded return type.
/**
 * Class Mage_Core_Helper_Object_Observer_Controller_Action
 *
 * @method Mage_Core_Helper_Object_Observer_Controller_Action_Event getEvent()
 */
class Mage_Core_Helper_Object_Observer_Controller_Action extends Varien_Event_Observer
{
}
  1. change noRoute() DOC to
@param Varien_Event_Observer|Mage_Core_Helper_Object_Observer_Controller_Action $observer
  1. since getEvent() should retrun Varien_Event we have to add another pseudo class where we access the passed data ...
/**
 * Class Mage_Core_Helper_Object_Observer_Controller_Action_Event
 *
 * @method Mage_Core_Controller_Varien_Action getAction()
 * @method Mage_Core_Helper_Object_Request getStatus()
 */
class Mage_Core_Helper_Object_Observer_Controller_Action_Event extends Varien_Event
{
}

This would cover Mage_Core_Controller_Varien_Action::norouteAction()

Mage::dispatchEvent('controller_action_noroute', array('action'=>$this, 'status'=>$status));
  1. last step would be antother pseudo class to replace Varien_Object with Mage_Core_Helper_Object_Request
    public function norouteAction($coreRoute = null)
    {
        $status = ( $this->getRequest()->getParam('__status__') )
            ? $this->getRequest()->getParam('__status__')
            : new Varien_Object();
  1. Add Varien_Object replacement
/**
 * Class Mage_Core_Helper_Object_Request
 *
 * @method bool getLoaded()
 * @method $this setLoaded(bool $value)
 * @method string getForwardModule()
 * @method $this setForwardModule(string $value)
 * @method string getForwardController()
 * @method $this setForwardController(string $value)
 * @method string getForwardAction()
 * @method $this setForwardAction(string $value)
 */
class Mage_Core_Helper_Object_Request extends Varien_Object
{
}

3. Missing call to parent method

Example 088d028

phpstan errors that return types are diffent and signature doest fit to parent method. The parent just returns $this, so it could be solved (and make code more consistent) with returning parent method and change DOCs to @inheritDoc

4. Undefined methods for Mage_Core_Model_Abstract in resource models befor/after calls

phpstan/IDE doesn't know about child methods ... so there are 2 approches ...

  • "workaround": replace @param Mage_Core_Model_Abstract with Mage_Core_Model_Abstract|Mage_Some_Model_Name
  • change: add type checks like reverted here 51e4b6a. The object should not be anything else as the related model, not?

However, the workaround should be okay and needs less changes ...

5. Undefined methods for concrete blocks in _prepareLayout()

More an IDE improvement ... e.g. head block that is loaded in many classes, like Mage_Cms_Block_Page

    $head = $this->getLayout()->getBlock('head');
    if ($head) {
        $head->setTitle($page->getTitle());
        $head->setKeywords($page->getMetaKeywords());
        $head->setDescription($page->getMetaDescription());
    }

head is defined in layout XML as page/html_head, so it would be correct to check instance of Mage_Page_Block_Html_Head as it is done some line above for breadcrumbs ...

        if ($breadcrumbs instanceof Mage_Page_Block_Html_Breadcrumbs) {
            foreach ($breadcrumbsObject->getData('crumbs') as $breadcrumbsItem) {
                $breadcrumbs->addCrumb($breadcrumbsItem['crumbName'], $breadcrumbsItem['crumbInfo']);
            }
        }

Changing this should break nothing and looks like unconsistent code too.


Before I continue my PRs please let me know whats accectable and whats not. :)

@sreichel
Copy link
Contributor Author

sreichel commented Jun 3, 2019

1. Magic methods on Varien_Object

I started to change it to setter/getter, but this would cause 1000s of changes .... no. Other idea is to add some pseudo classes that extend Varien_Object. See 088d028

  1. Instead renaming Varien_Object it should work a @param annotation for a pseudo class eg.
/** @var OpenMageLts_Object_<path_pattern> $params */
$params = new Varien_Object();
  1. Set @var annotation where it is used

  2. Add classes that inherit from Varien_Object to lib/OpenMageLts/<path_pattern>

So no changes in code just some @var and helper classes for PHPStorm/PHPStan ...?

@fballiano
Copy link
Contributor

I'm closing this since it's not a real issue, although it will stay here available as a great source of documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants