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

Removed unnecessary replacing of variable from parent #1495

Merged

Conversation

Sekiphp
Copy link
Contributor

@Sekiphp Sekiphp commented Mar 12, 2021

Description (*)

Mage_Directory_Model_Resource_Country_Collection extending Varien_Data_Collection where is $_items already defined:

    /**
     * Collection items
     *
     * @var array
     */
    protected $_items = array();

And there was diferrent initial value.

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)

@Sekiphp Sekiphp changed the title Removed replacing of variable from parent Removed unnecessary replacing of variable from parent Mar 12, 2021
@github-actions github-actions bot added the Component: Directory Relates to Mage_Directory label Mar 12, 2021
Comment on lines -36 to -39
/**
* @var Mage_Directory_Model_Country[]
*/
protected $_items;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will loose phpdoc (typehint) by this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 resolved in ea13c48

@colinmollenhour
Copy link
Member

On the vardocs, I like to add methods like this:

/**
 * @method Mage_Directory_Model_Country getFirstItem()
 * @method Mage_Directory_Model_Country getLastItem()
 * @method null|Mage_Directory_Model_Country getItemById($idValue)
 * @method Mage_Directory_Model_Country[] getItems()
 * @method Mage_Directory_Model_Country[] getIterator()

The last one in particular is helpful when iterating a collection so that the loop value has correct type hinting automatically, at least in PhpStorm.

@sreichel
Copy link
Contributor

sreichel commented Mar 12, 2021

The last one in particular is helpful when iterating a collection so that the loop value has correct type hinting automatically, at least in PhpStorm.

Which phpStorm version do you use? I thought this would already work?

Wait ... no time to check, but I solved in another way. I added something like @method SomeClass[] getCollection(), but not for all classes yet. See #784. I'd prefer my way, because all methods are covered. (if i'm right here)

@colinmollenhour
Copy link
Member

PhpStorm is smart enough to use the return type of getIterator() inside a loop to determine the iterator item type because Varien_Data_Collection implements IteratorAggregate. So if getIterator() hints Mage_Directory_Model_Country[] then this will work with no additional hints ($country will be detected as an instance of Mage_Directory_Model_Country):

foreach (Mage::getResourceModel('directory/country_collection') as $country) {
    $country->getIso2Code(); // returns string
}

Also this:

Mage::getResourceModel('directory/country_collection')->getFirstItem()->getIso2Code(); // returns string

My example with @method annotations is the only way I know to get this to work.

@sreichel
Copy link
Contributor

@colinmollenhour you're right. getIterator() is something i missed for all doc block update :/

@kkrieger85 kkrieger85 merged commit 3e24f15 into OpenMage:1.9.4.x Mar 25, 2021
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
2 runs  ±0  2 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 3e24f15. ± Comparison against base commit 48fe4d8.

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

Successfully merging this pull request may close these issues.

6 participants