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

Update getCustomer method in order class #26423

Conversation

sertlab
Copy link
Contributor

@sertlab sertlab commented Jan 16, 2020

Description (*)

Fixed Issues (if relevant)

  1. $order->getCustomer() returns NULL for registered customer #25268 $order->getCustomer() returns NULL for registered customer

Manual testing scenarios (*)

  • Create an observer with sales_order_save_after event
  • In execute method of the observer try to use getCustomer() method

Expected Result
A customer object should be returned

Actual Result
Null Returned

Questions or comments

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 are green)

@m2-assistant
Copy link

m2-assistant bot commented Jan 16, 2020

Hi @sertlab. 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 give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

Copy link
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.

Such change is not backward compatible, see https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/

What problem are you trying to solve by such change?

@ghost ghost assigned orlangur Jan 16, 2020
@ihor-sviziev
Copy link
Contributor

I think this is related issue #25268
I think the most correct fix will be updating phpdoc block for getCustomer method to show that it could return null.
#25268 (comment)

@sertlab
Copy link
Contributor Author

sertlab commented Jan 20, 2020

I think this is related issue #25268
I think the most correct fix will be updating phpdoc block for getCustomer method to show that it could return null.
#25268 (comment)

I tested and it returns only null.
By using this fix we get the correct customer object from order.

@ihor-sviziev
Copy link
Contributor

@sertlab it might return customer object in some specific case.
Getting customer is not direct responsibility of data Order interface, it should contains only data about order, and ids of related entities.
Also both methods getCustomer and getCustomerId could return null in case if order was placed as guest, and that’s absolutely valid case.
I raised this discussion in the original issue that you wanted to fix. For now will put this PR on hold

@sertlab
Copy link
Contributor Author

sertlab commented Jan 30, 2020

@sertlab it might return customer object in some specific case.
Getting customer is not direct responsibility of data Order interface, it should contains only data about order, and ids of related entities.
Also both methods getCustomer and getCustomerId could return null in case if order was placed as guest, and that’s absolutely valid case.
I raised this discussion in the original issue that you wanted to fix. For now will put this PR on hold

Hi . @ihor-sviziev Tested with registered and logged in customer and getCustomer() still returns null in observer

@ihor-sviziev
Copy link
Contributor

Hi . @ihor-sviziev Tested with registered and logged in customer and getCustomer() still returns null in observer

Yeah, this is expected behavior! Data object should just be a container for data, it should not contains any business logic, for example retrieving customer model.

Fix actually should be just changing phpdoc block that method getCustomer could return null. In ideal situation would be great just remove it from that model at all, but I believe it will be treated as backward incompatible change, so let’s not do that.

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @sertlab,
Could you rollback your changes in OrderInterface, Order classes and just update phpdoc block in Order model in order to highlight that it could return null?

@sertlab
Copy link
Contributor Author

sertlab commented Feb 5, 2020

@ihor-sviziev Hi. It's now updated as per your instructions

@ihor-sviziev ihor-sviziev added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Award: bug fix labels Feb 5, 2020
@ihor-sviziev ihor-sviziev changed the title update getCustomer method in order class Update getCustomer method in order class Feb 6, 2020
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Failing functional & unit tests are not related to changes from this PR, so I'm approving this PR

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-6809 has been created to process this Pull Request

@engcom-Delta
Copy link
Contributor

✔️ QA passed

@m2-assistant
Copy link

m2-assistant bot commented Feb 9, 2020

Hi @sertlab, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@sdzhepa sdzhepa mentioned this pull request May 9, 2022
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