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.

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

Bug in beforehandler, on deleting multiple rows #210

Merged
merged 2 commits into from
Mar 17, 2017
Merged

Bug in beforehandler, on deleting multiple rows #210

merged 2 commits into from
Mar 17, 2017

Conversation

BarryDam
Copy link
Contributor

@BarryDam BarryDam commented Mar 16, 2017

Hi there,

We're allmost there, I found 2 bugs, one I solved with this pull request:

The first one: on deleting multiple rows at once and applying my soft delete see example 209 I got an 404 errror : Subject caused by the updateObject method

The other you will find here 209, this needs to be discussed first.

Hi there,

We're allmost there, I found 2 bug, one I solved by this pull request:

The first one: when deleting multiple rows at once and applying my soft delete [see example 209](#209) I got an 404 errror : Subject cuased by the [updateObject method](https://github.com/mevdschee/php-crud-api/blob/master/api.php#L1533-L1535) 

The other you will find here [209](#209), this needs to be discussed first.
@mevdschee
Copy link
Owner

Can you verify that the "deleting multiple rows issue" still exists in v1.0.9?

@mevdschee mevdschee self-assigned this Mar 16, 2017
@mevdschee mevdschee added the bug label Mar 16, 2017
@BarryDam
Copy link
Contributor Author

BarryDam commented Mar 16, 2017

Nope it still throws "Not found (subject) " since the $inputs holds only 1 object.. and it should be multiple on a multiple deletion

@mevdschee
Copy link
Owner

mevdschee commented Mar 16, 2017

Ah.. I get it.. so you should really modify inputs so that it holds multiple values.

$inputs = array_map(function($v){
  return (object)array('deleted'=>date('Y-m-d H:i:s', time());
},explode(',',$id));

Something like that?

@BarryDam
Copy link
Contributor Author

BarryDam commented Mar 16, 2017

Uh yes I guess,:

when you do

DELETE http://localhost/api.php/categories/1,2

In the current situation

'before'=>function(&$cmd, &$db, &$tab, &$id, &$in) {  }

$id = '1,2'; and the $in parameter holds an array with ONE object.

which shoud be 2 objects.. one for $id 1 and one for $id 2..

since in the updateObject method you check the $inputs count agains the multiple count..

(count($inputs)!=count($keys))

Which in the current situation would evaluate to (1!-2) which will throw the error

@mevdschee
Copy link
Owner

So, shouldn't we just change the example?

@BarryDam
Copy link
Contributor Author

BarryDam commented Mar 16, 2017

... uhhhhh .. .well ... not really sure..

first off, I think the functionality for the end user should be as "easy to use" as possible... now the end user has to do something that feels complex in my opinion.

Second.. on a multiple deletion the $in param has one object.. which has no use at all.. so it either holds 2 values (on 2 deletions), or null., since you can't do anything with it...
In comparison with a before multiple (2) update cmd.. the $in param will also hold 2 objects..

Also I found another bug this foreach wil overwrite the inputs returned by applyBeforeHandler..

it will kick out de delete column since $fields does not have this column

@BarryDam
Copy link
Contributor Author

BarryDam commented Mar 16, 2017

Makes me think, wouldn't it be better .. if on a multiple Update (or other command).. for example

PUT http://localhost/api.php/categories/1,2
[{"name":"Internet"},{"name":"Programming"}]

the "before" => function() { ... } would get called twice

first time

'before'=>function(&$cmd, &$db, &$tab, &$id, &$in) {  
$id; // 1
$in; //{"name":"Internet"}
}

second time
first time

'before'=>function(&$cmd, &$db, &$tab, &$id, &$in) {  
$id; // 2
$in; //{"name":"Programming"}
}

Makes it more end user friendly, also the end user will have the same expectations as if it would be only 1 update.

@mevdschee
Copy link
Owner

I agree, I was doubting whether or not that would be a good idea, maybe it is indeed..

@BarryDam
Copy link
Contributor Author

I think it is! And maybe this would also be better for the after callback.. .

@BarryDam
Copy link
Contributor Author

BarryDam commented Mar 16, 2017

like this?

Includes the multiple before call and fix for : "
Also I found another bug this foreach wil overwrite the inputs returned by applyBeforeHandler..
"

@mevdschee mevdschee merged commit 68a4043 into mevdschee:master Mar 17, 2017
@mevdschee
Copy link
Owner

Thank you. I did some refactoring in 919e820

@mevdschee
Copy link
Owner

Can you verify that the issue is gone in v1.1.3?

@BarryDam
Copy link
Contributor Author

No it's not, see #212

@BarryDam BarryDam deleted the patch-1 branch March 17, 2017 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants