2.3 unify object collection #961

Merged
merged 3 commits into from Dec 22, 2012

Projects

None yet

4 participants

@dereuromark
Member

personally I liked attach/detach and attached() better - especially for behaviors.
but since the consensus was to deprecate attach/detach in favor of load/unload we should not keep attached() then either. it is confusing.

so we should deprecate it in favor of loaded().

@dereuromark
Member

hm, travis build failed due to a second difference in a test case:

1) CakeEmailTest::testHeaders
110Failed asserting that Array (
111    'From' => 'CakePHP <cake@cakephp.org>'
112    'To' => 'cake@cakephp.org, CakePHP <php@cakephp.org>'
113    'X-Something' => 'very nice'
114    'X-Other' => 'cool'
115    'X-Mailer' => 'CakePHP Email'
116    'Date' => 'Fri, 16 Nov 2012 11:35:00 +0000'
117    'MIME-Version' => '1.0'
118    'Content-Type' => 'text/plain; charset=UTF-8'
119    'Content-Transfer-Encoding' => '8bit'
120) is identical to Array (
121    'From' => 'CakePHP <cake@cakephp.org>'
122    'To' => 'cake@cakephp.org, CakePHP <php@cakephp.org>'
123    'X-Something' => 'very nice'
124    'X-Other' => 'cool'
125    'X-Mailer' => 'CakePHP Email'
126    'Date' => 'Fri, 16 Nov 2012 11:35:01 +0000'
127    'MIME-Version' => '1.0'
128    'Content-Type' => 'text/plain; charset=UTF-8'
129    'Content-Transfer-Encoding' => '8bit'
130).

maybe the assert should look sth like this? dereuromark@cakephp:2.3...dereuromark:2.3-margin-test

@markstory markstory and 1 other commented on an outdated diff Nov 16, 2012
lib/Cake/Utility/ObjectCollection.php
*/
public function attached($name = null) {
+ return $this->loaded($name);
+ }
+
+/**
+ * Gets the list of loaded objects, or, whether the given object is loaded
+ *
+ * @param string $name Optional. The name of the behavior to check the status of. If omitted,
+ * returns an array of currently-loaded behaviors
+ * @return mixed If $name is specified, returns the boolean status of the corresponding behavior.
+ * Otherwise, returns an array of all loaded behaviors.
@markstory
markstory Nov 16, 2012 Member

Should probably remove references to 'behaviors' here as they could be anything. It was wrong in the old docs too, but we shouldn't carry that mistake forward to the new method.

@dereuromark
dereuromark Nov 16, 2012 Member

will fix that.

@predominant
Contributor

I still don't like loaded() as it doesn't really describe what you are checking. But I guess it's too late for that now.

On 16/11/2012, at 8:20 PM, Mark notifications@github.com wrote:

personally I liked attach/detach and attached() better - especially for behaviors.
but since the consensus was to deprecate attach/detach in favor of load/unload we should not keep attached() then either. it is confusing.

so we should deprecate it in favor of loaded().

You can merge this Pull Request by running:

git pull https://github.com/dereuromark/cakephp 2.3-unify-object-collection
Or view, comment on, or merge it at:

#961

Commit Summary

unify object load/unload and loaded methods
deprecate attached()
File Changes

M lib/Cake/Log/CakeLog.php (2)
M lib/Cake/Test/Case/Console/TaskCollectionTest.php (8)
M lib/Cake/Test/Case/Controller/ComponentCollectionTest.php (16)
M lib/Cake/Test/Case/Model/BehaviorCollectionTest.php (32)
M lib/Cake/Test/Case/Model/ModelIntegrationTest.php (8)
M lib/Cake/Test/Case/Utility/ObjectCollectionTest.php (12)
M lib/Cake/Test/Case/View/HelperCollectionTest.php (20)
M lib/Cake/Test/Case/View/ViewTest.php (4)
M lib/Cake/TestSuite/ControllerTestCase.php (2)
M lib/Cake/Utility/ObjectCollection.php (13)
Patch Links

https://github.com/cakephp/cakephp/pull/961.patch
https://github.com/cakephp/cakephp/pull/961.diff

Reply to this email directly or view it on GitHub.

@dereuromark
Member

As I mentioned I also favored the attached()/attach()/detach(). Back in the days I didn't have any say, though^^.
Since they just got deprecated and we didnt remove anything yet, and cake2.3 is still beta we could still hop on the other train.
But the others core devs seem to be happy with it so far. So it will probably just be load/unload then from here on.

@predominant
Contributor

Welcome to frown town

On 17/11/2012, at 8:01 AM, Mark notifications@github.com wrote:

As I mentioned I also favored the attached()/attach()/detach(). But back in the days I didn't have any say^^.
Since they just got deprecated and we didnt remove anything yet, and cake2.3 is still beta we could still hop on the other train.
But the others core devs seem to be happy with it so far. So it will probably just be load/unload then from here on.


Reply to this email directly or view it on GitHub.

@markstory
Member

While I like attached() as well. Using loaded() makes more sense with load() and unload(), as they fit better together. Similar to enabled(), enable() and disable().

@dereuromark
Member

yeah, either loaded() + load/unload - OR attached() + attach()/detach()
so we will stick to the first one then? all right.

@dereuromark
Member

If its ok - and its merged - I will then update the migration guide accordingly.

@dereuromark
Member

any objections then left?

@markstory
Member

I'm good with this. @lorenzo or @predominant ?

@lorenzo
Member
lorenzo commented Dec 7, 2012

My only concern is that we are already in RC and this changes the API, although in a non-breaking way as always

@dereuromark
Member

its part of the attached/loaded conversion that has been left out or forgotten during the process.

@lorenzo
Member
lorenzo commented Dec 22, 2012

Giving it a second thought, there is no actual BC breaking change in this, will merge

@lorenzo lorenzo merged commit a8bd7c6 into cakephp:2.3 Dec 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment