zend-db
zend-db copied to clipboard
zendframework
Expression check $valueParameter !== null
This issue has been moved from the zendframework repository as part of the bug migration program as outlined here - http://framework.zend.com/blog/2016-04-11-issue-closures.html
Original Issue: https://api.github.com/repos/zendframework/zendframework/issues/7293 User: @marcelto Created On: 2015-03-04T15:50:41Z Updated At: 2015-03-21T20:13:24Z Body Adding an expression to a where object without any parameters will throw an exception unless an empty array is passed.
$select->where->expression('1 = 0', []);//works $select->where->expression('1 = 0');//will throw an exception
No parameters are being specified, it would be nice not to have to pass an empty array. Also, this was not an issue in earlier versions prior to the /[, $valueParameter, ... ]/ addition.
Comment
User: @Ocramius Created On: 2015-03-04T17:24:03Z Updated At: 2015-03-04T17:24:03Z Body Requires a test case
Comment
User: @Pittiplatsch
Created On: 2015-03-05T11:49:51Z
Updated At: 2015-03-05T11:50:40Z
Body
Sorry, but I really don't get you. As I already stated in #7289, there is NO problem with $valueParam == null.
$this->setParameters(is_array($valueParameter) ? $valueParameter : array_slice(func_get_args(), 1));
already results in
$this->setParameters(array());
when $valueParam == null, i.e. param is being left out.
func_get_args() ignores optional params being left out - that's what func_get_args() is all about: respect actually given function arguments!
Just an idea: Do you EXPLICITLY pass $valueParam as null? This obviously and correclty results in
$this->setParameters(array(null));
Comment
User: @Martin-P Created On: 2015-03-05T14:56:52Z Updated At: 2015-03-05T14:56:52Z Body
Adding an expression to a where object without any parameters will throw an exception unless an empty array is passed.
Please take a look at this unittest: ZendTest\Db\Sql\Predicate\ExpressionTest::testCanPassNoParameterToConstructor
That test passes, which suggests it is already possible to leave out the parameters. If there is an issue with your use case, it is not caused by Zend\Db\Sql\Predicate\Expression.
Comment
User: @Martin-P
Created On: 2015-03-05T15:08:49Z
Updated At: 2015-03-05T15:08:49Z
Body
Looking at the code shows the problem is not in Zend\Db\Sql\Predicate\Expression, but in Zend\Db\Sql\Predicate\Predicate::expression(). That method expects a second argument and should trigger a PHP error (not an exception). Perhaps you can post the exception you get?
Comment
User: @marcelto
Created On: 2015-03-05T15:21:44Z
Updated At: 2015-03-05T15:31:05Z
Body
Currently I'm passing an empty array for these calls. Otherwise I receive a fatal error and an exception. It looks like defaulting $parameters to an empty array in Zend\Db\Sql\Predicate\Predicate::expression() fixes the issue. Would this be the more agreeable solution?
Fatal error: Uncaught exception 'Zend\Db\Sql\Exception\RuntimeException' with message 'The number of replacements in the expression does not match the number of parameters' in '...vendor\zendframework\zendframework\library\Zend\Db\Sql\Expression.php on line 143
Zend\Db\Sql\Exception\RuntimeException: The number of replacements in the expression does not match the number of parameters in '...\vendor\zendframework\zendframework\library\Zend\Db\Sql\Expression.php on line 143
Comment
User: @Pittiplatsch Created On: 2015-03-05T16:35:58Z Updated At: 2015-03-05T16:35:58Z Body
It looks like defaulting $parameters to an empty array in Zend\Db\Sql\Predicate\Predicate::expression() fixes the issue.
Defaulting an empty array seems to be a valid, simple solution.
Defaulting to null could also work, but would be far more complex, as the same logic like in Zend\Db\Sql\Predicate\Expression (via func_get_args()) had to be implemented to get things work correctly.
IMO, an empty array is simple fine here.
Either possibility however needs new tests as well ;-)
Comment
User: @marcelto
Created On: 2015-03-05T16:53:27Z
Updated At: 2015-03-05T16:53:27Z
Body
Hmmm....If you wanted the functionality of /*[, $valueParameter, ... ]*/ to be accessible via the where wouldn't the func_get_args() logic be necessary?
Comment
User: @Pittiplatsch
Created On: 2015-03-05T17:04:00Z
Updated At: 2015-03-05T17:04:00Z
Body
Yepp, that's what I meant with "far more complex". However, I wouldn't expose this weird functionality to Predicate::expression(), as this kind of parameter handling in the long run usually causes more problems than it solves IMO. E.g. extending the parameter list later on due to some kind of extended behaviour is almost impossible when working with param lists of dynamic length. Dunno why this IMO weird param handling made it into Zend\Db\Sql\Predicate\Expression at all...
Comment
User: @marcelto Created On: 2015-03-18T00:57:51Z Updated At: 2015-03-18T00:57:51Z Body a test has been added
Comment
User: @weierophinney Created On: 2015-03-19T20:15:42Z Updated At: 2015-03-19T20:15:42Z Body I tried merging this just now, but the new test actually fails:
1) ZendTest\Db\Sql\Predicate\PredicateTest::testExpressionParametersDefaultToArray
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
- 0 => Array (...)
+ 0 => 'foo = bar'
)
tests/ZendTest/Db/Sql/Predicate/PredicateTest.php:242
(note: I separated the test into it's own method, per @Martin-P , placing it directly below the method in which @marcelto originally placed it in)
In other words, the patch is not working as expected. I'll delay it for a later release.
Comment
User: @marcelto Created On: 2015-03-20T02:05:03Z Updated At: 2015-03-20T02:05:03Z Body I added the new/separate test case. It passes locally and travis passed 5.3 and 5.4.
Comment
User: @Martin-P Created On: 2015-03-21T14:33:11Z Updated At: 2015-03-21T14:33:11Z Body
It passes locally and travis passed 5.3 and 5.4.
Please look again, because it does not pass on Travis at all.
Comment
User: @marcelto Created On: 2015-03-21T20:13:24Z Updated At: 2015-03-21T20:13:24Z Body I could have sworn the first two passed. My apologies, I must have been looking at the wrong build.
This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at https://github.com/laminas/laminas-db/issues/115.
