Skip to content
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.

Sign up for GitHub

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

Jump to bottom

3.0 remove deep merging from mergeVars #2922

Merged
merged 2 commits into from Mar 1, 2014
Merged

3.0 remove deep merging from mergeVars #2922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2cde87
Add test for merging associative properties.
markstory Mar 1, 2014
1a9a8fb
Do not merge the contents of merged properties.
markstory Mar 1, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 24 additions & 6 deletions src/Utility/MergeVariablesTrait.php
View file
Open in desktop
Expand Up @@ -80,19 +80,37 @@ protected function _mergeProperty($property, $parentClasses, $options) {
} }
foreach ($parentClasses as $class) { foreach ($parentClasses as $class) {
$parentProperties = get_class_vars($class); $parentProperties = get_class_vars($class);
if (!isset($parentProperties[$property])) { if (empty($parentProperties[$property])) {
continue; continue;
} }
$parentProperty = $parentProperties[$property]; $parentProperty = $parentProperties[$property];
if (empty($parentProperty) || $parentProperty === true) { if (!is_array($parentProperty)) {
continue; continue;
} }
if ($isAssoc) { $thisValue = $this->_mergePropertyData($thisValue, $parentProperty, $isAssoc);
$parentProperty = Hash::normalize($parentProperty);
}
$thisValue = Hash::merge($parentProperty, $thisValue);
} }
$this->{$property} = $thisValue; $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) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this foreach loop any different than

$current += $parent

?

After reading the discussion of removing merging from inherited properties (:+1:) do we need a trait? Is this doing any more than "normalize and += parent/defaults" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only difference vs. + would be around nulls. I think a trait is still useful as this behavior is still used in shells as well.

if (!isset($current[$key])) {
$current[$key] = $value;
}
}
return $current;
}

} }
47 changes: 44 additions & 3 deletions tests/TestCase/Utility/MergeVariablesTraitTest.php
View file
Open in desktop
Expand Up @@ -44,6 +44,15 @@ class Child extends Base {
'Orange' 'Orange'
]; ];


public $nestedProperty = [
'Red' => [
'apple' => 'gala',
],
'Green' => [
'citrus' => 'lime'
],
];

} }


class Grandchild extends Child { class Grandchild extends Child {
Expand All @@ -54,6 +63,15 @@ class Grandchild extends Child {
'Green' => ['apple'], 'Green' => ['apple'],
'Yellow' => ['banana'] 'Yellow' => ['banana']
]; ];

public $nestedProperty = [
'Red' => [
'citrus' => 'blood orange',
],
'Green' => [
'citrus' => 'key lime'
],
];
} }


/** /**
Expand All @@ -76,7 +94,7 @@ public function testMergeVarsAsList() {
} }


/** /**
* Test merging vars as an assoc list. * Test merging vars as an associative list.
* *
* @return void * @return void
*/ */
Expand All @@ -86,22 +104,45 @@ public function testMergeVarsAsAssoc() {
$expected = [ $expected = [
'Red' => null, 'Red' => null,
'Orange' => null, 'Orange' => null,
'Green' => ['lime', 'apple'], 'Green' => ['apple'],
'Yellow' => ['banana'], 'Yellow' => ['banana'],
]; ];
$this->assertEquals($expected, $object->assocProperty); $this->assertEquals($expected, $object->assocProperty);
} }


/**
* Test merging variable in associated properties that contain
* additional keys.
*
* @return void
*/
public function testMergeVarsAsAssocWithKeyValues() {
$object = new Grandchild();
$object->mergeVars(['nestedProperty'], ['associative' => ['nestedProperty']]);

$expected = [
'Red' => [
'citrus' => 'blood orange',
],
'Green' => [
'citrus' => 'key lime',
],
];
$this->assertEquals($expected, $object->nestedProperty);
}

/** /**
* Test merging vars with mixed modes. * Test merging vars with mixed modes.
*
* @return void
*/ */
public function testMergeVarsMixedModes() { public function testMergeVarsMixedModes() {
$object = new Grandchild(); $object = new Grandchild();
$object->mergeVars(['assocProperty', 'listProperty'], ['associative' => ['assocProperty']]); $object->mergeVars(['assocProperty', 'listProperty'], ['associative' => ['assocProperty']]);
$expected = [ $expected = [
'Red' => null, 'Red' => null,
'Orange' => null, 'Orange' => null,
'Green' => ['lime', 'apple'], 'Green' => ['apple'],
'Yellow' => ['banana'], 'Yellow' => ['banana'],
]; ];
$this->assertEquals($expected, $object->assocProperty); $this->assertEquals($expected, $object->assocProperty);
Expand Down