->findOne->delete() smells

[dereuromark]

We can currently (and have that in existing code) write:

        ->filterById($id)
        ->findOne()
        ->delete();

But since findOne() can be nullable return (IDE also warns here), we must not use this to avoid a fatal error.

@method     ExampleName|null findOne(ConnectionInterface $con = null)

The docblock hints on this already

I think findOne() should not have the option to allow delete(). Refs https://github.com/propelorm/Propel2/issues/1642

Any ideas on how to enforce e.g.

        $somethingOrNull = $query->findOne();
        if ($somethingOrNull !== null) {
            $somethingOrNull->delete();
        }

Alternatively, we could also only allow

        ->filterById($id)
        ->findOneOrFail()
        ->delete();

[SpikedCola]

I would agree that ->findOne()->delete() doesn't make sense for the reason you stated.

Does ->requireOne()->delete() provide the required behaviour?

Then you still have the option of using

$somethingOrNull = $query->findOne();
if ($somethingOrNull !== null) {
    $somethingOrNull->delete();
}
else {
    ...
}

in the case where you need the "else" condition.

[dereuromark]

Yes, the occurrences we found in code we are fixing to this null check pattern. My point was that the method shouldn't even work in the first place in a future version - as it is a time bomb.

->requireOne()->delete()

I didn't know yet, I like it :)

[mringler]

I don't think it is possible to disallow a statement like ->findOne()->delete(). Since findOne returns the model object, you are not chaining on the query object, but on the model. As long as the model can do delete(), you can write ->requireOne()->delete(). Doing->requireOne()->delete() probably can only avoid the call to delete() by throwing an exception, which leaves you with an exception nonetheless.

I tend to just use ->find()->delete(). This always works, since ->find() returns an ObjectCollection, which can handle delete even when it is empty. You can delete more than one object though, but doing ->findOne()->delete() when the query returns more than one object is dangerous as well.

If you want to solve it with findOne, you would have to have it return an Optional, like Java offers it for such cases. But that is not available in PHP. A simple solution could return a dummy object. Something like:

public function findOneOrDummy() {
    $one = $this->findOne();
    if ( $one !== null) {
        return $one;
    }
    return new class() {
        public function __call($method, $arguments)
        {
            return $this;
        }
    };
}

Like this, you can even keep chaining on the object, as in ->findOneOrDummy()->setByName('MyColumn', 42)->save(). You probably would want to know if you are not saving to an actual object though.

[dereuromark]

Doesnt this mean that we would not need findOne()->delete()? We could deprecate this part and let users know they should be using find()->delete() then?

The alternative could be to provide a findOneAndDelete() wrapper that handles this internally, clean and without issues for static analyzers as well as humans reading the code.

[mringler]

Doesnt this mean that we would not need findOne()->delete()?

With all the new type hints, findOne()->delete() should give a warning, since findOne() can return null, and you obviously cannot call methods on null. There is really nothing else we can do, but I think it sufficiently resolves the issue. If users call a method on a return value that might be null, we can warn them, but we cannot stop them.

Recently, I stumbled over the query method findOneOrCreate(), which does exactly what the above outlined findOneOrDummy() does, just better, as it applies default values. So that means, instead of doing findOne()->delete(), there are two other options, either find()->delete() or findOneOrCreate()->delete(). Oh, and then there is obviously just delete(), which is the same as find()->delete(), just more efficient (could have realized that earlier).

So I guess if we add a method findOneAndDelete(), the benefit would be that it could make sure that at most one row is deleted. But I am not sure if this is necessary, if you want to delete at most one row, you generally don't write a query that could delete several rows.