Make \Orm\Model class more easily extended

[johnpc]

I have a class that extends the \Orm\Model class (called \MyNamespace\Model). One of the methods it overrides is Model::find which calls parent::find($id, $options) which always has func_num_args() === 2 even when the caller of the parent class left the $options field empty.

I ran a script that executed:

$model = \MyNamespace\Model::find(null);
$model->delete();

This caused an entire table to be truncated when I would have expected an error cannot call method delete on null.

[johnpc]

I realize this could have an impact on backwards compatibility (if users of this method were depending on this functionality) but I also feel this is dangerous enough that a change is warranted.

I'd also consider enforcing that a certain option was passed in in order call DELETE FROM table with no conditions.

The change could also wait until the next major version update when backwards compatibility is less expected.

What are your thoughts @WanWizard? I want to prevent issues like the one I experienced for other users of this project in the future, but if there's rationale for having it be the way it is I'm all ears.

[WanWizard]

The problem is not in the framework, the problem is in your code.

find() should return null when nothing is found, your code seems to return an object of some kind.

If your find() doesn't get an options array passed, why do you pass anything on to the parent?

[WanWizard]

To be complete, the check is explicitly on number of arguments and not on contents, because this is a valid call:

// returns null
$result = Model_Test::find(null);

// returns a Query object
$result = Model_Position::find(null, array());

[johnpc]

@WanWizard you might be misunderstanding the issue. This is the unexpected behavior:

MyModel extends \Orm\Model
{
    public function find($id = null, $options = array())
    {
        return parent::find($id, $options);
    }
}

// I would expect this to return null, actually returns a Query object (and calling $should_be_null->delete() truncates a table)
$should_be_null = MyModel::find(null);

Perhaps it should at least be documented somewhere that this method cannot be safely extended.

[WanWizard]

No. I don't.

As said before, it returns something different because you are altering the number of arguments passed.

You do have a point in that you as a developer overloading the method should not have to look into the code to check for unexpected behaviour, so I have removed the argument count check in my commit, but that requires a change in your code as the signature of the method has changed to:

public function find($id = null, $options = null)