Skip to content

Fix docBlock for hasInvoices(), hasShipments(), hasCreditmemos()#16554

Merged
magento-engcom-team merged 2 commits intomagento:2.2-developfrom
comwrap:docBlock-order
Jul 13, 2018
Merged

Fix docBlock for hasInvoices(), hasShipments(), hasCreditmemos()#16554
magento-engcom-team merged 2 commits intomagento:2.2-developfrom
comwrap:docBlock-order

Conversation

@nuzil
Copy link
Copy Markdown
Contributor

@nuzil nuzil commented Jul 5, 2018

Description

Small improvements of docBlock for app/code/Magento/Sales/Model/Order.php docBlock description.
hasInvoices(), hasShipments(), hasCreditmemos() returns int and not bool

Manual testing scenarios

No testing scenarios, just docBlock changed

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team magento-engcom-team added Partner: Comwrap partners-contribution Pull Request is created by Magento Partner labels Jul 5, 2018
@magento-engcom-team
Copy link
Copy Markdown
Contributor

Hi @nuzil. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

Copy link
Copy Markdown
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

They should be boolean even though they actually returning count. It's better to change implementation.

@nuzil
Copy link
Copy Markdown
Contributor Author

nuzil commented Jul 6, 2018

@orlangur
Agree, though so also, but then I though about IF there are a case when developers used this method in wrong way and used them for receive count() of invoices. That will crash their implementation.
Not sure should we think about that cases or if code logic is correct we have to do it?

@VladimirZaets
Copy link
Copy Markdown
Contributor

Hi @nuzil.
Yes, we understand it. We consider and make a decision for each case separately.
We had the discussion about this case and we think that the correct way to fix this problem it is changing implementation.

@nuzil
Copy link
Copy Markdown
Contributor Author

nuzil commented Jul 6, 2018

@VladimirZaets deal 👍

@magento-engcom-team
Copy link
Copy Markdown
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-2194 has been created to process this Pull Request

@magento-engcom-team
Copy link
Copy Markdown
Contributor

Hi @nuzil. Thank you for your contribution.
We will aim to release these changes as part of 2.2.6.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

@ihor-sviziev
Copy link
Copy Markdown
Contributor

Hi @nuzil @VladimirZaets @orlangur,
It's better to use (bool)$variable than boolval($variable) because it's construction of language and no overhead on function execution.

@sreichel
Copy link
Copy Markdown
Contributor

sreichel commented Aug 10, 2018

@orlangur this should be moved to "Port to 2.3 Merged" on board.

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.

6 participants