Enforcing string types for argument names call() scalar method
#2015
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #2015 +/- ##
==========================================
+ Coverage 78.94% 79.39% +0.44%
==========================================
Files 846 846
Lines 25235 25244 +9
==========================================
+ Hits 19923 20043 +120
+ Misses 5312 5201 -111
🚀 New features to boost your workflow:
|
| * @param null|Type<mixed> $returnType | ||
| */ | ||
| public function call(ScalarFunction|callable $callable, array $arguments = [], string|int $refAlias = 0, ?Type $returnType = null) : CallUserFunc | ||
| public function call(ScalarFunction|callable $callable, string $refAlias, array $arguments = [], ?Type $returnType = null) : CallUserFunc |
There was a problem hiding this comment.
I'm not sure how much I like that change.
When refAlias is required string you need to always provide argument name to call even if function has just one parameter.
For example ref('array')->call('count') needs to now become ref('array')->call('count', refAlias: 'value')
I just checked and in one project I have around 150 calls like this that I will need to manually one by one adjust (and sine those are different functions I would need to learn their argument names as well).
I need a really strong argument too approve that.
Instead I would rather prefer to check if refAlias was passed as string|integer, then check if how arguments are passed and throw an exception if one us using integer indexes and other is string, or other way around.
There was a problem hiding this comment.
After writing more tests, I had a similar feeling that enforcing will always be problematic, and I did the change on it, I think the same way as you pointed out in the comment ;)
There was a problem hiding this comment.
By the way, which calls do you have more of? So we can extend the tests to see what I missed?
There was a problem hiding this comment.
The original intention of allowing for int was to be able to pass arguments by name or position.
So for example:
['param_name_1' => 'param_value_1', 'param_name_2' => 'param_value_2']['param_value_1', 'param_value_2']
And ideally I would like to keep it this way.
However to be able to use it with positional parameter you need to do something lke this:
call(ref('column'), ['param_1, null, 'param_2'], refALias: 1])
So array merge would replace [1 => null] with [1 => ref('column')->eval()]
(this wasn't properly documented)
stloyd
changed the title
call() scalar methodcall() scalar method
2 participants
Resolves: #2014
Change Log
Added
Fixed
Changed
Removed
Deprecated
Security