zendframework/zend-db

Update with where clossure is not working as expected

Opened this issue · 2 comments

In Zend\Db\TableGateway\AbstractTableGateway update method if you send where as clossure is sent as a parameter in update->where
if ($where !== null) { $update->where($where); }
So the closure end up being called with a PredicateInterface object but in the select and delete method
the code is
if ($where instanceof \Closure) { $where($delete); } else { $delete->where($where); }

if ($where instanceof \Closure) { $where($select); } elseif ($where !== null) { $select->where($where); }

And there the closure is called with a SqlInterface object and I can't understand why the update method is not the same as select and delete ? Because if you want to make a generic clossure for select,update and delete as I made the it won't work because of the different interface object passed in closure.

Tl;Dr, what is your usecase of reusing closure?

Played with this for a while. At first appeared as careless implementation needing a fix. Did the fix in code, first in favor of passing PredicateSet to closure to give opportunity to modify it using a reusable closure for convenience across all 3 operation types. But when got to updating documentation on closure usage, realized it gives opportunity to fully customize Select object in ways beyond what TableGateway offers out of the box. So I tried to inverse the parameter passing full Update and Delete into closure. This caused a worse dilemma. It makes good sense to modify Select in more details beyond just where, such as order by, limit etc. But Update and Delete will want whole different usage. In fact, allowing only PredicateInterface is a pretty good restriction to have. Then, there is interface difference. SqlInterface is pretty limited, so having it serve all 3 different objects (or 2 different categories, to be more exact - one for detailed querying and another for restricted changes).

Usage does look ugly but I am not seeing how to improve it.

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#89.