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

phpDocumentor Return tag must be an int #10760

Merged
merged 2 commits into from
Sep 4, 2017

Conversation

reindert-vetter
Copy link
Contributor

@reindert-vetter reindert-vetter commented Sep 4, 2017

Because count() gives an int, hasShipment returns an int. Now i can't do this:
if ($view->getOrder()->hasShipments() === true) {

Because count() gives an int, hasShipment returns an int. Now i can't do this:
```if ($view->getOrder()->hasShipments() === true) {```
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Sep 4, 2017

CLA assistant check
All committers have signed the CLA.

@ishakhsuvarov ishakhsuvarov self-assigned this Sep 4, 2017
@ishakhsuvarov ishakhsuvarov added this to the September 2017 milestone Sep 4, 2017
@orlangur orlangur self-assigned this Sep 4, 2017
@orlangur
Copy link
Contributor

orlangur commented Sep 4, 2017

@reindert-vetter code like
if ($view->getOrder()->hasShipments() === true) {
should be always written as
if ($view->getOrder()->hasShipments()) {
actually.

I do agree that current implementation of the method could be a bit confusing but I believe we should not change contract here but just change implementation.

Please change all has* methods in this class so that they always return boolean value if you wish:

public function hasShipments()
    {
        return (bool)$this->getShipmentsCollection()->count();
    }

To match implementation with PHPDoc correctly.

@reindert-vetter
Copy link
Contributor Author

Done

@orlangur
Copy link
Contributor

orlangur commented Sep 4, 2017

Great, thanks for the quick change!

@magento-team magento-team merged commit c785afb into magento:develop Sep 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants