Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Use getEncoding() instead of accessing options array directly #7159

Closed

Conversation

Martin-P
Copy link
Contributor

Fix for issue #7147. Both Zend\Filter\StringToLower and Zend\Filter\StringToUpper extend Zend\Filter\AbstractUnicode.

No new tests, but that is because I don't see a way to test this change. If anyone got suggestions on how to test it, I will add the tests.

@@ -53,7 +53,7 @@ public function filter($value)
}
$value = (string) $value;

if ($this->options['encoding'] !== null) {
if (null !== $this->getEncoding()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

no yoda condition for consistencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not really a consistency when I do a search in my IDE:

  • Found 403 matches of !== null in 205 files
  • Found 265 matches of null !== in 129 files

There are even PR's where conditions are being changed to yoda conditions. To mention a few: #3520, #3521, #6750.

Copy link
Contributor

Choose a reason for hiding this comment

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

the #3521 and #3520 , the yoda condition is not accepted. anyway, the current purpose of this pr is changing $this->options['encoding'] to $this->getEncoding(), so changing right to left is a cosmetic imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The yoda condition is cosmetic indeed, but so is PSR2 😉 Personally I prefer yoda conditions, because I think defensive programming is good practice.

@Ocramius
Copy link
Member

Ocramius commented Feb 1, 2015

@Martin-P this requires testing, IMO. Mocking the getEncoding method is also "meh", but required for the test to work :-\

@Ocramius Ocramius added this to the 2.3.5 milestone Feb 1, 2015
@Ocramius Ocramius self-assigned this Feb 1, 2015
@Martin-P
Copy link
Contributor Author

Martin-P commented Feb 2, 2015

How can I mock the method of the same class? This does not work:

public function testFilterUsesGetEncoding()
{
    $filterMock = $this->getMock('StringToLowerFilter');
    $filterMock->expects($this->once())
               ->method('getEncoding')
               ->with();
    $filterMock->filter('foo');
}

Fatal error: Call to undefined method Mock_StringToLowerFilter_0ae23a04::filter()

Adding the filter() method to the mocked object does not help either, because then getEncoding() is never called.

@Ocramius
Copy link
Member

Ocramius commented Feb 2, 2015

@Martin-P add $this->getMock('StringToLowerFilter', array('getEncoding')). The class of the mock must be the FQCN.

*/
public function testFilterUsesGetEncodingMethod()
{
$filterMock = $this->getMock('\Zend\Filter\StringToLower', array('getEncoding'));
Copy link
Member

Choose a reason for hiding this comment

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

Note that in strings you always deal with an absolute namespace, therefore the leading \ is not needed and should be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay, I have removed them.

@weierophinney weierophinney modified the milestones: 2.3.5, 2.3.6 Feb 18, 2015
@weierophinney weierophinney modified the milestones: 2.4.0, 2.3.6 Feb 19, 2015
weierophinney added a commit that referenced this pull request Feb 24, 2015
Use getEncoding() instead of accessing options array directly
weierophinney added a commit that referenced this pull request Feb 24, 2015
@weierophinney
Copy link
Member

Merged to develop for release with 2.4.

@Martin-P Martin-P deleted the hotfix/abstract-unicode-filters branch February 28, 2015 19:22
weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015
…fix/abstract-unicode-filters

Use getEncoding() instead of accessing options array directly
weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015
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