Allow invokable classes as predicate (solves #270)#272
Allow invokable classes as predicate (solves #270)#272heyphilllie wants to merge 4 commits intozendframework:developfrom
Conversation
Ocramius
left a comment
Ocramius
left a comment
There was a problem hiding this comment.
Simple and clean: review comments are just about improvements
src/Sql/Predicate/PredicateSet.php
Outdated
| return $this; | ||
| } | ||
| if ($predicates instanceof \Closure) { | ||
| if ($predicates instanceof \Closure || is_callable($predicates)) { |
There was a problem hiding this comment.
Can be simplified to \is_callable($predicates)
|
|
||
| class WhereInvokable | ||
| { | ||
| private $value; |
| $this->value = $value; | ||
| } | ||
|
|
||
| public function __invoke($select) |
There was a problem hiding this comment.
Add a type declaration to $select
There was a problem hiding this comment.
The class is also used for Delete and Update test cases. If I add the declaration the UnitTest will fail, because an instance of Update and Delete is injected into the function.
There was a problem hiding this comment.
@I3ekka I've not read the code but I usually solve 'multi-type' by introducing an interface(with or without methods). If this commends totally misses the point, ignore it :P
| * WhereInvokable constructor. | ||
| * @param $value | ||
| */ | ||
| public function __construct($value) |
There was a problem hiding this comment.
Add a type declaration to $value
There was a problem hiding this comment.
I did not add the type declaration because the composer.json says this module is still on php 5.5. If this is an outdated information, I'll add the type.
There was a problem hiding this comment.
Hmm, weird that it is still on 5.5. develop should be on 7.1
There was a problem hiding this comment.
yeah, also travis runs with an old configuration
There was a problem hiding this comment.
@I3ekka we are going to support PHP 5.6+ with zend-db 2.9.0. We'll move to PHP 7.1 with zend-db 3.0.0
|
@Ocramius the status of this one is hard... do we have someone willing to update this library for PHP 7? Or at a minimum get the travis and composer config moving in the right direction? |
|
IMO we should hold back new features until 7.1 is the minimum version. Everything we add without explicit type declarations will just come back like a boomerang. |
|
@Ocramius I tagged this as 3.0 release. |
|
@Ocramius I was wondering if you could go into detail how explicit type declarations would help in this situation or conversely if there is any foreseeable boomerang? |
|
This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:
|
|
This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#62. |
8 participants
This PR solves #270 .
Predicates can now created from an invokable class. Like closures, the new predicates should directly added to the Select, Where or Update instance.