Fix maxDimensions #7022
Fix maxDimensions #7022
Conversation
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.
Added comments for $count param
|
Shouldn't this be a bug fix for 2.6? As positioned now, this fix won't be released for 4-5 months as it is going into 2.8. |
| } else { | ||
| $depth[] = 1; | ||
| } | ||
| $depth[] = static::maxDimensions($value, $count + 1 ); |
There was a problem hiding this comment.
Won't this recursively walk every branch in a nested array and count the maxDimensions each time? While that might yield the right answer, I'm not sure it is the most efficient way to derive that answer.
There was a problem hiding this comment.
I am not sure if this is the most efficient way. I pieced the code together from the deprecated Set::countDim function primarily because the current Hash::maxDimensions function is not returning the correct value. I discovered this while trying to migrate from the deprecated Set functions to the new Hash functions.
There was a problem hiding this comment.
Thinking about this more, there isn't really a more efficient way. To find the max depth we have to do a full tree traversal as there is no way of knowing where the deepest leaf is.
|
Sorry @markstory I am new to contributing to this repository. I chose the 2.8 branch because it was the newest version of the 2.x branch. Should I close this request and start it in the 2.6 branch? |
|
Targetting 2.7 might be best, I'm going to do the final 2.6 release tonight. |
|
Alright, I'll close this pull request. I'll resubmit it to the 2.7 branch tomorrow. |
*** This also effects the 3.x branch. ***
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.