Allow callbacks in Model->exists() method. #472

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@stefanozoffoli

Hi,

in my Models I add some condition in beforeFind() callback to filter the results.

If in Model->exists() I can't choose to call the callbacks or not, I can have wrong response.

In this patch I set the default argument to true, I don't know if it is correct to you.

Thank you

@AD7six

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Feb 10, 2012

Member

Why not use find count if exists doesn't do what you want?

Member

AD7six commented Feb 10, 2012

Why not use find count if exists doesn't do what you want?

@stefanozoffoli

This comment has been minimized.

Show comment Hide comment
@stefanozoffoli

stefanozoffoli Feb 10, 2012

Yes for now I use find count to solve this problem.

But since exists use find count too, I think that letting developers to choose if or not to enable callbacks is a more elegant solution.

Otherwise I'll have to overwrite that method in modellApp.

Yes for now I use find count to solve this problem.

But since exists use find count too, I think that letting developers to choose if or not to enable callbacks is a more elegant solution.

Otherwise I'll have to overwrite that method in modellApp.

@markstory

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Feb 10, 2012

Owner

This changes the default behavior too. If we're going to add a param, why not make the default the same?

Owner

markstory commented Feb 10, 2012

This changes the default behavior too. If we're going to add a param, why not make the default the same?

@stefanozoffoli

This comment has been minimized.

Show comment Hide comment
@stefanozoffoli

stefanozoffoli Feb 10, 2012

Yes I took advantage of the version change from 2.0 to 2.1 for setting the default value to true, but effectively I don't know how this change would affect other projects.

Yes I took advantage of the version change from 2.0 to 2.1 for setting the default value to true, but effectively I don't know how this change would affect other projects.

@AD7six

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Feb 10, 2012

Member

a minor version change is not an appropriate milestone to change (and especially invert) the api

Member

AD7six commented Feb 10, 2012

a minor version change is not an appropriate milestone to change (and especially invert) the api

@stefanozoffoli

This comment has been minimized.

Show comment Hide comment
@stefanozoffoli

stefanozoffoli Feb 10, 2012

Ok. Changing the default value to false is a good idea and can be accepted or i need to override exists method in appmodel?

Ok. Changing the default value to false is a good idea and can be accepted or i need to override exists method in appmodel?

@AD7six

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Feb 21, 2012

Member

Well, what's the benefit of adding callbacks to exists? That doesn't make much sense to me - a call to exists uses the primary key value - why would you need callbacks, or in what way does the absense of callbacks cause problems?

A test case would probably explain

Member

AD7six commented Feb 21, 2012

Well, what's the benefit of adding callbacks to exists? That doesn't make much sense to me - a call to exists uses the primary key value - why would you need callbacks, or in what way does the absense of callbacks cause problems?

A test case would probably explain

@xavier83ar

This comment has been minimized.

Show comment Hide comment
@xavier83ar

xavier83ar Feb 21, 2012

Hi AD7six,

I've had the same problem that stefanozoffoli. I've made soft deletable behavior (originally written by Mariano Iglesias) compatible with cakephp 2.0 and I'm using it in a project.
The thing is this behaviors handles a flag to know if a record was deleted or not, but no one record is deleted from database, so if exists() method doesn't allow callbacks, is showing wrong info.
As I've baked all my controllers, my app is checking in every place with exists method.
I've overwritten the exists method in AppModel and set callbacks to true there, and that's ok for me, but this is an example of when or why the absense of callbacks in exists method produces a wrong result, also I think that if behaviors are supposed to change the way a model behaves, it should affect all its methods in a consistently way, it doesn't have much sense to me that the particulary "exists" method ignores model's behaviors.

Sorry for my english,

Regards.

Hi AD7six,

I've had the same problem that stefanozoffoli. I've made soft deletable behavior (originally written by Mariano Iglesias) compatible with cakephp 2.0 and I'm using it in a project.
The thing is this behaviors handles a flag to know if a record was deleted or not, but no one record is deleted from database, so if exists() method doesn't allow callbacks, is showing wrong info.
As I've baked all my controllers, my app is checking in every place with exists method.
I've overwritten the exists method in AppModel and set callbacks to true there, and that's ok for me, but this is an example of when or why the absense of callbacks in exists method produces a wrong result, also I think that if behaviors are supposed to change the way a model behaves, it should affect all its methods in a consistently way, it doesn't have much sense to me that the particulary "exists" method ignores model's behaviors.

Sorry for my english,

Regards.

@stefanozoffoli

This comment has been minimized.

Show comment Hide comment
@stefanozoffoli

stefanozoffoli Feb 22, 2012

I had a CMS that filter a Content Model results by type.

I have contents of type Blog, Page, Image, etc...

So in my model BeforeFind() i filter the results by type like this:

$query['conditions'] = array(
  'Content.type' => $this->type,
);
return $query;

In my admin panel, I have one section per content type. When I edit a record, I first check if that record exists, but I only want to consider records of the right type.

If exists() doesn't call callbacks, a user could edit a record of a different type in the wrong section, and that is a bug for my CMS. So I had to ovverride the exists() method in my AppModel.

I had a CMS that filter a Content Model results by type.

I have contents of type Blog, Page, Image, etc...

So in my model BeforeFind() i filter the results by type like this:

$query['conditions'] = array(
  'Content.type' => $this->type,
);
return $query;

In my admin panel, I have one section per content type. When I edit a record, I first check if that record exists, but I only want to consider records of the right type.

If exists() doesn't call callbacks, a user could edit a record of a different type in the wrong section, and that is a bug for my CMS. So I had to ovverride the exists() method in my AppModel.

@bar

This comment has been minimized.

Show comment Hide comment
@bar

bar Oct 26, 2012

Contributor

I don't consider this a bug. In fact I think Model::exists() primary use is to test for existense in a deeper level, thus avoiding any callback modification made by a Behavior.

I don't think allowing to set $callback with a default value of false is a bad idea, though.

Contributor

bar commented Oct 26, 2012

I don't consider this a bug. In fact I think Model::exists() primary use is to test for existense in a deeper level, thus avoiding any callback modification made by a Behavior.

I don't think allowing to set $callback with a default value of false is a bad idea, though.

@ADmad

This comment has been minimized.

Show comment Hide comment
@ADmad

ADmad Oct 26, 2012

Member

👎 for me too. Model::exists() is a fine the way it it is as a quick check for existence of record. We already have tickets about exists() being called too many times than required (caching its results causes its own set of problems) and triggering callbacks would just increase overhead. Those who want this feature can easily override exists() and enable callbacks, its a pretty simple function anyway.

Member

ADmad commented Oct 26, 2012

👎 for me too. Model::exists() is a fine the way it it is as a quick check for existence of record. We already have tickets about exists() being called too many times than required (caching its results causes its own set of problems) and triggering callbacks would just increase overhead. Those who want this feature can easily override exists() and enable callbacks, its a pretty simple function anyway.

@markstory markstory closed this Oct 26, 2012

@markstory

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Oct 26, 2012

Owner

Closing as @ADmad makes a good point around the existing problems with exists() and this change just making those problems worse.

Owner

markstory commented Oct 26, 2012

Closing as @ADmad makes a good point around the existing problems with exists() and this change just making those problems worse.

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