PHPORM-100 Support query on numerical field names #2642
Conversation
|
Test failure to investigate: |
src/Query/Builder.php
Outdated
| @@ -961,8 +962,8 @@ public function where($column, $operator = null, $value = null, $boolean = 'and' | |||
| } | |||
| } | |||
|
|
|||
| if (func_num_args() === 1 && is_string($column)) { | |||
| throw new ArgumentCountError(sprintf('Too few arguments to function %s("%s"), 1 passed and at least 2 expected when the 1st is a string.', __METHOD__, $column)); | |||
| if (func_num_args() === 1 && is_scalar($column)) { | |||
There was a problem hiding this comment.
If we're expecting an array, I'd specifically check for that:
| if (func_num_args() === 1 && is_scalar($column)) { | |
| if (func_num_args() === 1 && ! is_array($column)) { |
src/Eloquent/Model.php
Outdated
| @@ -212,6 +216,8 @@ public function setAttribute($key, $value) | |||
| $value = $builder->convertKey($value); | |||
| } | |||
|
|
|||
| $key = (string) $key; | |||
There was a problem hiding this comment.
Do you want to move this to the top of the function for consistency with other methods? It won't affect the // Convert _id to ObjectID logic at all, and my understanding is this is the best we can do in lieu of adding a string type hint to the $key argument, which would be a BC break due to our use of strict types.
Also, perhaps that comment should be changed since Builder::convertKey() also converts strings to Binary UUIDs.
There was a problem hiding this comment.
Cast moved.
Also, perhaps that comment should be changed since
Builder::convertKey()also converts strings to Binary UUIDs.
I don't understand what you mean.
There was a problem hiding this comment.
The comment here said "Convert _id to ObjectID" but convertKey() also converts UUIDs. Small detail (and most people won't be using UUID identifiers), so no objection if it remains as-is.
tests/ModelTest.php
Outdated
| $this->assertInstanceOf(User::class, $found); | ||
| $this->assertEquals([3 => 'two.three'], $found[2]); | ||
|
|
||
| $found = User::where(2.3, 'two.three')->first(); |
There was a problem hiding this comment.
I hate everything about this. I understand the original request was to support integers, but do we really need to accept floats in this fashion? That seems like something we could outright reject, unless it's problematic.
This made me think of how MongoDB now supports field names with dots and dollars (related spec change in mongodb/specifications@851ca10). Querying still requires special operators, but inserting does not. I'm not sure this comes into play with this library, but I wouldn't want to allow someone to use a float to insert a field this way and would much rather force them to format it as a string.
There was a problem hiding this comment.
I changed the logic to throw an exception when the column is not a string or an int.
src/Query/Builder.php
Outdated
| } | ||
|
|
||
| if (! is_int($column) && ! is_string($column)) { | ||
| throw new InvalidArgumentException(sprintf('First argument of %s must be a column name as "string". Got "%s"', __METHOD__, get_debug_type($column))); |
There was a problem hiding this comment.
I assume you intentionally do not want to tell people that an "int" is accepted, since you silently convert it. SGTM.
This comment was marked as off-topic.
This comment was marked as off-topic.
Fix PHPORM-100
Fix #2639
Related to #2634
Query::where()to all scalar values instead of only string as column.str_*functions.This bug was existing before 4.0.0, this change could be considered a new feature for 4.1 or merged as a bugfix in 4.0