Skip to content
This repository has been archived by the owner on Apr 29, 2019. It is now read-only.

Removed create_function() function usage #32

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

joni-jones
Copy link
Contributor

@joni-jones joni-jones commented Feb 1, 2018

Removed create_function() function usage

Fixed Issues

  1. Deprecated: Function create_function() is deprecated #8: Deprecated: Function create_function() is deprecated

Changes for EE in the internal branch https://github.com/magento-mpi/magento2ee/tree/G%238

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)

$mock = $this->getMockBuilder('Foo')
->setMethods(['getValue'])
->getMock();
$mock->method('getValue')
Copy link
Contributor

@ihor-sviziev ihor-sviziev Feb 2, 2018

Choose a reason for hiding this comment

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

Looks like section ->expects($this->once()) is missing here

Copy link
Contributor Author

@joni-jones joni-jones Feb 2, 2018

Choose a reason for hiding this comment

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

It doesn't matter how many times getValue will be called, that's why expects is not needed.

Copy link
Contributor

@ihor-sviziev ihor-sviziev Feb 2, 2018

Choose a reason for hiding this comment

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

In this case would be better to use ->expects($this->any()) to make it clear

Copy link
Contributor Author

@joni-jones joni-jones Feb 2, 2018

Choose a reason for hiding this comment

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

For this test case, in general, it doesn't matter how many times the method will be called, so any() is redundant. The expects() may sense to use when we need to know if the method is called or not (corner cases, like business logic conditionals) or how many times it was called - for performance reasons.
I don't see, how any() makes test a cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Good point. Let's leave it as is

@buskamuza buskamuza self-assigned this Feb 7, 2018
@buskamuza buskamuza added this to the February 2018 milestone Feb 7, 2018
@magento-engcom-team magento-engcom-team merged commit 7c79007 into magento-engcom:2.3-develop Feb 8, 2018
magento-team pushed a commit that referenced this pull request Aug 27, 2018
magento-engcom-team pushed a commit that referenced this pull request Sep 11, 2018
magento-engcom-team pushed a commit that referenced this pull request Sep 11, 2018
magento-engcom-team pushed a commit that referenced this pull request Sep 11, 2018
magento-engcom-team pushed a commit that referenced this pull request Sep 11, 2018
magento-engcom-team pushed a commit that referenced this pull request Sep 11, 2018
magento-engcom-team pushed a commit that referenced this pull request Oct 13, 2018
…ions #32

 - Merge Pull Request magento/bulk-api-ce#32 from magento/bulk-api-ce:amqp-optional-consummers
 - Merged commits:
   1. 9a757cb
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants