Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

3.0 remove deep merging from mergeVars #2922

Merged
merged 2 commits into from Mar 1, 2014

Conversation

Projects
None yet
4 participants
Owner

markstory commented Mar 1, 2014

As per #2914. This removes deep merging from the MergeVariablesTrait. Instead only the top level keys are merged for associative properties.
List style properties are still simply merged.

Hopefully, this new behavior makes merged properties simpler to understand with more predictable results.

markstory added some commits Mar 1, 2014

@markstory markstory Add test for merging associative properties.
Associated/hash properties should be merged like
dictionaries/array_merge.

Refs #2914
d2cde87
@markstory markstory Do not merge the contents of merged properties.
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 #2914
1a9a8fb

@markstory markstory added this to the 3.0.0 milestone Mar 1, 2014

Owner

lorenzo commented Mar 1, 2014

This makes sense to me, but I think it shows a more fundamental issue. We often use AppController and AppModel for settings global behavior, for example you want AuthComponent in all controllers, but one controller in particular needs an additional authorization adapter. This is almost imposible to achieve using the magic merged vars.

I think we should encourage another style of declaring common behavior in 3.0, that is not automagic variable merging.

@AD7six AD7six commented on the diff Mar 1, 2014

src/Utility/MergeVariablesTrait.php
}
$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) {
@AD7six

AD7six Mar 1, 2014

Member

Is this foreach loop any different than

$current += $parent

?

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

@markstory

markstory Mar 1, 2014

Owner

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.

Member

dereuromark commented Mar 1, 2014

That reminds me of my ticket with 2.x and merge issues when aliasing helpers and components in AppController. Once a child (or plugin child) controller has Form as $helpers value, the aliasing ('Form' => array('className' => 'PluginName.FormExt')) is lost/overwritten again.
It would be nice if there was some better way in general to allow proper merging with such a setup and without too much magic.
In such a case for instance, it would be preferred for the parent controller (AppController) to have precedence (as you cannot just modify third party plugins that easily). But that cannot always be assumed, of course.

Member

AD7six commented Mar 1, 2014

@dereuromark I'm more or less of the opinion that default merging logic should be as simple as possible (in principle: instance $var is $var += parent::$var), and for anything beyond that e.g. users are encouraged to override _mergeControllerVars (or class equivalent) - that would remove the "what's going to happen here"-ness of the existing implementation where users want different behavior even within the same nested array structure.

Member

dereuromark commented Mar 1, 2014

@AD7six I agree. That allows both ways and certainly makes most sense.

Owner

markstory commented Mar 1, 2014

@lorenzo Are you thinking an initialize() hook like we added in the orm?

Owner

lorenzo commented Mar 1, 2014

@markstory Yes, that is exactly what I'm thinking

Owner

lorenzo commented Mar 1, 2014

Merging this as there were no further comments and any future actions can be addressed in separate pull requests

@lorenzo lorenzo added a commit that referenced this pull request Mar 1, 2014

@lorenzo lorenzo Merge pull request #2922 from markstory/issue-2914
3.0 remove deep merging from mergeVars
eff4f0b

@lorenzo lorenzo merged commit eff4f0b into cakephp:3.0 Mar 1, 2014

1 check failed

default The Travis CI build failed
Details

@lorenzo lorenzo deleted the markstory:issue-2914 branch Mar 1, 2014

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