Allow the Terms query to accept arrays of strings, ints and floats #1872
Conversation
On it, actually i also would like to provide a test, but need a bit time => moving into draft state! |
|
Awesome, thanks! |
|
Hmm, a force-push doesn't seem to retrigger the build? |
|
Re-opening the PR does 😄 |
|
@ruflin technically all done / green, only BC break needs be discussed |
|
PS: great contributing guide, although I don't have much docker experience, was able to get this easily running and also execute the phpunit tests before pushing, thank you! 👍 |
Should be fine; I didn't see any existing
🤷♀️ The existing code base doesn't seem much to use such a pattern, couldn't find any. So I made the change as un-intrusive as possible I'm not sure if you want me to go ahead and change things, the feedback doesn't seem to much match the existing code base. I'm happy to change it, just wanted to make sure I don't to unnecessary work, let me know. Thank you. |
|
I'm personally comfortable with getting this breaking change in. I'm pretty sure we will have 1-2 users hit by it but I personally would even consider it a bug fix. The 1-2 users that will hit by will hopefully have an IDE in place that tells them there is a problem? In the changelog, lets put it under breaking change to make it more obvious. Or do I miss something here? No strong preference on my end on the way we put it in. I'm following the guidance of @deguif as he put a lot of effort into unifying the code base recently and probably knows best. I'm also ok with merging the PR as is and then follow up. @deguif WDYT? |
|
@ruflin I do not see this as a breaking change, as the method, is now accepting a wider range of types. |
Roger that! Suggestion applied 😄
Also done! |
|
@thePanz it's a technical one, see https://3v4l.org/Ws5bM => if someone extended |
src/Query/Terms.php
Outdated
| { | ||
| if (!\is_string($term) && !\is_int($term) && !\is_float($term)) { | ||
| throw new TypeError(\sprintf('Argument 1 passed to "%s()" must be of type float|int|string, %s given.', __METHOD__, \is_object($term) ? \get_class($term) : \gettype($term))); |
There was a problem hiding this comment.
Can we have a throw new \TypeError to avoid the use of it at the beginning of the class?
There was a problem hiding this comment.
So what's the rule when to import and when not?
There was a problem hiding this comment.
Not sure, IMHO I would try to not import something from the root namespace, but just use \TypeError. WDYT?
There was a problem hiding this comment.
Yes I'm 👍 with @thePanz comment ;)
@mfn you're right! This would open the discussion on making the query classes final, but let's not open the Pandora's vase now :) |
| /** | ||
| * @group functional | ||
| */ | ||
| public function testVariousDataTypesViaConstructor(): void |
There was a problem hiding this comment.
Can you add a mixed test too? Terms('some_numeric_field', ['9876', 1234])
There was a problem hiding this comment.
Done! => testVariousMixedDataTypesViaConstructor
As shown in ruflin#1765 (comment) , ElasticSearch accepts all of them
|
All done! |
|
Thanks everyone for getting this in with reviews and @mfn for the quick turn arounds on all the changes! |
|
I think it should also accept booleans since elasticsearch accepts those |
|
@romainneutron I think you're right. PS: Sorry. I just see it was already propsed as a bug fix by @romainneutron |
As shown in #1765 (comment) , ElasticSearch accepts all of them
Note: the change of
public function addTermwas not previously discussed, but is made for consistency, however poses a breaking change if someone extended this class previously => https://3v4l.org/Ws5bMTODO