Fix Hash::maxDimensions #7040
Closed
Fix Hash::maxDimensions #7040
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
** Closed the 2.8 pull request and opened this one instead. ** The current Hash::maxDimensions function calls Hash::dimensions to try to get the maximum depth of the passed in array. However, this ends up only getting the depth of the first element of each 1st dimension element in the array passed to maxDimensions. The function needs to be called recursively in order to get the depth of ALL of the elements in all of the dimensions of the passed in array. I made the maxDimensions function more closely resemble the deprecated Set::countDim function in order to restore the correct functionality. Example: $data = array( 0 => array( 0 => 'Some Value', 1 => array( 0 => 'Some other Value' ) ), 1 => array( 0 => 'Some Value', ) ); Returns 2 under the current version of maxDimensions. Returns 3 with the changes proposed above.
|
Can you also add a test case for this please? |
Fixed whitespace issue caught by Travis CI
|
Hi @dereuromark, I am not sure what you mean by add a test case. I added an example array in the opening comment that counts incorrectly using the original method and correctly with this modification. Is there something else you would like me to do? |
| } else { | ||
| $depth[] = 1; | ||
| } | ||
| $depth[] = static::maxDimensions($value, $count + 1 ); |
There was a problem hiding this comment.
Are we OK with the self => static change for 2.7+?
There was a problem hiding this comment.
I am. composer.json says PHP>=5.3.0
|
@jadedcore you could add your example here so it will prevent future regression |
markstory
pushed a commit
that referenced
this pull request
Jul 21, 2015
Added test case for Hash::maxDimensions using the example array in pull request #7040.
markstory
added a commit
that referenced
this pull request
Jul 21, 2015
We don't need the additional parameter, and some of the tests weren't covering unique scenarios. Refs #7040
|
Merged into 2.7 in 27e40f0 |
|
Ported to 3.x in 742f85d |
markstory
added a commit
that referenced
this pull request
Jul 24, 2015
Replacing self with static due to PHP5.3+. Following #7040.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
** Closed the 2.8 pull request and opened this one instead. Issue #7022 **
The current Hash::maxDimensions function calls Hash::dimensions to try to get the maximum depth of the passed in array. However, this ends up only getting the depth of the first element of each 1st dimension element in the array passed to maxDimensions. The function needs to be called recursively in order to get the depth of ALL of the elements in all of the dimensions of the passed in array.
I made the maxDimensions function more closely resemble the deprecated Set::countDim function in order to restore the correct functionality.
Example:
Returns 2 under the current version of maxDimensions.
Returns 3 with the changes proposed above.