FuelPHP Forums

daidais

When I generate a crud by a oil generate scaffold command the result in the controller is a little weird.
It generates different way to verify the $id param is not null ?

In action_view it checks $id after a Model_XXX::find,
in action__edit the check before and
in delete the check is not present.

Is there an explication for that ?

Thanks.

Harro

What version of Fuel are you using?

If I check the template for the view action (they are in oil/views), I see

is_null($id) and Response::redirect('<?php echo $controller_name ?>');
$data['<?php echo $singular_name ?>'] = Model_<?php echo $model_name ?>::find_by_pk($id);

Which checks first, then loads the model object. And edit does exactly the same.

daidais

I'm using FuelPHP 1.4

Sorry for my mistake I speak about the oil/views/scaffold/orm not the crud one. If you see you have :
In view :

$data['<?php echo $singular_name ?>'] = Model_<?php echo $model_name ?>::find($id);
is_null($id) and Response::redirect('<?php echo $controller_name ?>');

In edit :

is_null($id) and Response::redirect('<?php echo $controller_name ?>');
$<?php echo $singular_name; ?> = Model_<?php echo $model_name; ?>::find($id);

XCrackz0r

Actually and to complete a bit about id check... there is a error when you try to edit/view a non existing record, in example:

mydomain.co/controller/edit/id

If the id doesnt exists in the database, it throw a exception like this, for the default view:

ErrorException [ Notice ]: Trying to get property of non-object

I have changed this like

is_null($id) and Response::redirect('user/');

to

$user = Model_User::find($id);

!($user and $id) and Response::redirect('user/');

And yes, you are right, in the edit it first check if the id is null, then it does the query, while in the view, it first does the query then check if the id is setted... i think to avoid that just change the order to first run the query, then check if the id is not null or if the id exists.... this, to save some code... otherwise, try a if statement before the query and if the id is not null run the query, and if it returns a record (The id exits) ... continue with the function.

It could be implemented for the default scaffold, changing the oil template, to avoid that exception when trying to access a non existing record. ;)

daidais

I think if id is null at first it's because the id is not present in the url so I don't see why we should check a possibly null id in the database.

So for me this is the good solution :

is_null($id) and Response::redirect('<?php echo $controller_name ?>');
$<?php echo $singular_name; ?> = Model_<?php echo $model_name; ?>::find($id);

I simply ask if the difference is a little mistake for the action_view or if there is a good reason for that.

I understand the exception problem but it's another problem.
After check if there is an id we should test the result of the find to don't give a bad data to the view.

XCrackz0r

Well, the first line check if the id is null, the thing come when you pass a id which doesnt exist in the database.... then the exception is thrown because in the view, you are calling the returned id from the query, so if there is not such id, the object wont be available to the view, throwing the error...

So, 1 solution to this is what i did in my last post, other, do not call $query->id in your view...

Now why should we use a id which doesnt exist in the db? well maybe we know it doesnt exist, but all our users, ie, do know it?

Harro

The correct way is to test the result of the find, like delete() does.

If $id === null, or if $id is not valid, in both cases the find() will fail. And that should be captured here.

I'll fix it.

daidais

You can't because if id is null the find method will return an Orm/Query object which will be interpreted as true by the if statement.
In
the delete action if you don't pass the id param you've got this query
DELETE FROM `accounts` which will try to delete all items in the table.

Harro

That's why I wrote "if $id === null or $id is not valid"...

The delete action does:

if ($yourname = Model_Yourname::find($id))
{
$yourname->delete();
}

I don't see how that can translate into "DELETE FROM yourname"...

Harro

Ah, chips. It will call delete() on the Query object!

I desparately need to kick a former team member in the but!!!

Added a null check to the delete template too.

daidais

Ok sorry about that.

For the sql request if id is null a Orm/Query is return by the find method so you call the Omr/Query->delete() which try to execute "DELETE FROM yourname", but I haven't take the time to debug this part.

Well, thanks for the help Harro.

daidais

Ok I just see yout last post we are agree :)

FuelPHP Forums

Ask your question about FuelPHP in the appropriate forum, or help others by answering their questions.

Howdy, Stranger!

Categories

In this Discussion