Merge controller properties correctly #746

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Member

dereuromark commented Jul 31, 2012

fixes ticket #3079

it basically boils down to Hash::merge and how it works.
It needs an (empty) array to properly merge - null doesnt do it.

Member

dereuromark commented Jul 31, 2012

I just added a test case to show that you can use FALSE to disable merging.
but the whole point of the different controller levels is that merging should be possible. otherwise you cannot declare the properties DRY. and that is also consistent to other merging operations - like $uses (false = no merge) for models etc

also - some 3rd party plugins declare that they need certain helpers (Html for instance). but if you use aliasing you want those plugin controllers to use your extended classes, as well. otherwise they would behave differently than the rest of the application (e.g. using the wrong Html::url() method which would result in broken links etc)

but should this go into the master branch (2.2.1) or better in 2.3? for the latter I can close and resubmit.

Owner

lorenzo commented Aug 1, 2012

There are way too much cases were I would not like the parent to override children properties. I think the only case where it kind of make sense is with aliasing. I guess this is just a good hint for us to finally take out helpers declaration out of controllers or go with a more advanced solution for replacing dependencies

Owner

lorenzo commented Aug 1, 2012

So, it is -1 for me

Owner

markstory commented Oct 23, 2012

👎 from me as well. Changing how mergeVars works is probably a bad idea in an API compatible release. We can re-examine in 3.0 though. I still don't think that a parent class should prevent merging though.

markstory closed this Oct 23, 2012

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