Skip to content

Commit

Permalink
Do not merge the contents of merged properties.
Browse files Browse the repository at this point in the history
Merging the contents/configuration in merged properties often results in
the wrong answer as non-associative values will be duplicated N times
where N is the number of ancestors with the same value. Instead, only
merge missing top level items. The config for each item will be taken
from the top most class it is defined in.

Refs cakephp#2914
  • Loading branch information
markstory committed Mar 1, 2014
1 parent d2cde87 commit 1a9a8fb
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 10 deletions.
30 changes: 24 additions & 6 deletions src/Utility/MergeVariablesTrait.php
Expand Up @@ -80,19 +80,37 @@ protected function _mergeProperty($property, $parentClasses, $options) {
}
foreach ($parentClasses as $class) {
$parentProperties = get_class_vars($class);
if (!isset($parentProperties[$property])) {
if (empty($parentProperties[$property])) {
continue;
}
$parentProperty = $parentProperties[$property];
if (empty($parentProperty) || $parentProperty === true) {
if (!is_array($parentProperty)) {
continue;
}
if ($isAssoc) {
$parentProperty = Hash::normalize($parentProperty);
}
$thisValue = Hash::merge($parentProperty, $thisValue);
$thisValue = $this->_mergePropertyData($thisValue, $parentProperty, $isAssoc);
}
$this->{$property} = $thisValue;
}

/**
* Merge each of the keys in a property together.
*
* @param array $current The current merged value.
* @param array $parent The parent class' value.
* @param boolean $isAssoc Whether or not the merging should be done in associative mode.
* @return mixed The updated value.
*/
protected function _mergePropertyData($current, $parent, $isAssoc) {
if (!$isAssoc) {
return array_merge($parent, $current);
}
$parent = Hash::normalize($parent);
foreach ($parent as $key => $value) {
if (!isset($current[$key])) {
$current[$key] = $value;
}
}
return $current;
}

}
6 changes: 2 additions & 4 deletions tests/TestCase/Utility/MergeVariablesTraitTest.php
Expand Up @@ -66,7 +66,6 @@ class Grandchild extends Child {

public $nestedProperty = [
'Red' => [
'apple' => 'mcintosh',
'citrus' => 'blood orange',
],
'Green' => [
Expand Down Expand Up @@ -105,7 +104,7 @@ public function testMergeVarsAsAssoc() {
$expected = [
'Red' => null,
'Orange' => null,
'Green' => ['lime', 'apple'],
'Green' => ['apple'],
'Yellow' => ['banana'],
];
$this->assertEquals($expected, $object->assocProperty);
Expand All @@ -123,7 +122,6 @@ public function testMergeVarsAsAssocWithKeyValues() {

$expected = [
'Red' => [
'apple' => 'mcintosh',
'citrus' => 'blood orange',
],
'Green' => [
Expand All @@ -144,7 +142,7 @@ public function testMergeVarsMixedModes() {
$expected = [
'Red' => null,
'Orange' => null,
'Green' => ['lime', 'apple'],
'Green' => ['apple'],
'Yellow' => ['banana'],
];
$this->assertEquals($expected, $object->assocProperty);
Expand Down

0 comments on commit 1a9a8fb

Please sign in to comment.