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

Hash matches strict compare numeric value #11690

Merged
merged 2 commits into from Feb 5, 2018

Conversation

Projects
None yet
4 participants
@o0h
Contributor

o0h commented Feb 4, 2018

What I did

$john = Hash::extract([
    ['value' => 0],
    ['value' => 'John']
], '{n}[value=John]');
return current($john);

What happened

get the value as bellow.

$john = ['value' => 0];

What I expected to happen

like this.

$john = ['value' => 'John'];

I think it's better that Hash::_matches() handles '0' and (string)some-value as another value, when non numeric character token are given.
To regard some string values as '0' is counterintuitive for me ;(

@codecov-io

This comment has been minimized.

codecov-io commented Feb 4, 2018

Codecov Report

Merging #11690 into 3.next will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             3.next   #11690      +/-   ##
============================================
- Coverage     92.11%   92.11%   -0.01%     
- Complexity    13242    13243       +1     
============================================
  Files           462      462              
  Lines         34005    34007       +2     
============================================
+ Hits          31324    31325       +1     
- Misses         2681     2682       +1
Impacted Files Coverage Δ Complexity Δ
src/Utility/Hash.php 97.7% <100%> (ø) 247 <0> (+1) ⬆️
src/Cache/CacheRegistry.php 95.83% <0%> (-4.17%) 11% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e7bdbc...e70013f. Read the comment docs.

@dereuromark dereuromark added the Defect label Feb 4, 2018

@dereuromark

This comment has been minimized.

Member

dereuromark commented Feb 4, 2018

While this makes sense, I think it would be safer to go into 3.next (3.6), as this can alter behavior for a lot of existing code that might rely on this non-strict check. So while it would fix your issue, it might break a few others in the process.

@o0h

This comment has been minimized.

Contributor

o0h commented Feb 4, 2018

Thanks!
I think the patch is reasonable, but am also afraid of existing products in the world will be broken..
So, I agree this pr should not be merged as current version(3.5).

@markstory markstory changed the base branch from master to 3.next Feb 5, 2018

@markstory markstory added this to the 3.6.0 milestone Feb 5, 2018

@markstory

This comment has been minimized.

Member

markstory commented Feb 5, 2018

@o0h You will need to rebase you changes onto 3.next.

@@ -267,15 +267,17 @@ protected static function _matches($data, $selector)
$prop = $prop ? '1' : '0';
} elseif ($isBool) {
$prop = $prop ? 'true' : 'false';
} elseif (is_numeric($prop)) {
$prop = (string)$prop;

This comment has been minimized.

@markstory

markstory Feb 5, 2018

Member

How will integer data be matched now?

This comment has been minimized.

@o0h

o0h Feb 5, 2018

Contributor

Ex:

$data = [
            ['value' => '1'],
            ['value' => 1],
            ['value' => 'string-value'],
        ];
$result = Hash::extract($data, '{n}[value=1]');

then

$result = [
    ['value' => '3'],
    ['value' => 3],
]

When $selector is '[value=3]', $val is given as string.
And $prop casting to string, so $prop === $val means '3' === '3', returns true.

@markstory markstory merged commit 2787d60 into cakephp:3.next Feb 5, 2018

4 checks passed

codecov/patch 100% of diff hit (target 92.11%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +7.88% compared to 9e7bdbc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.

markstory added a commit to cakephp/docs that referenced this pull request Feb 5, 2018

@o0h o0h deleted the o0h:hash-matches-strict-compare-numeric-value branch Feb 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment