ruflin / Elastica

Elastica is a PHP client for elasticsearch
http://elastica.io/
MIT License
2.26k stars 736 forks source link

Remove tests with using wrong type #2100

Closed franmomu closed 2 years ago

franmomu commented 2 years ago

For reference: https://github.com/ruflin/Elastica/pull/2096#discussion_r924191185

https://github.com/ruflin/Elastica/blob/0015bf0805712aba78da81cc59f6a28c0c12ca3d/src/Search.php#L80-L100

I was going to deprecate passing something different than string and Index to Search::addIndex() method and saw that Search::hasIndex() method does not have any extra check for scalar:

https://github.com/ruflin/Elastica/blob/0015bf0805712aba78da81cc59f6a28c0c12ca3d/src/Search.php#L215-L225

Something similar happens to MatchQuery::setFieldBoost()

https://github.com/ruflin/Elastica/blob/0015bf0805712aba78da81cc59f6a28c0c12ca3d/src/Query/MatchQuery.php#L100-L103

which already has float as parameter type.

But there are a couple of tests that check these calls with different types, so I've chosen the easy path 😬 removing those tests.

If in 8.0 we are going to add strict types to all files these tests will throw a TypeError (if we use PHP 8.0).

So is this fine? or should we add a deprecation for Search::addIndex

thePanz commented 2 years ago

We should add a deprecation notice (check other places where we have them to have an example) to the hasIndex() when not used with a Index class, and add a hasIndeByName(string $indexName) to cover both cases.

WDYT?

franmomu commented 2 years ago

I'll do it tomorrow I think, we should also add a addIndicesByName like:

https://github.com/ruflin/Elastica/blob/0015bf0805712aba78da81cc59f6a28c0c12ca3d/src/Search.php#L102-L114

thePanz commented 2 years ago

I'll do it tomorrow I think, we should also add a addIndicesByName like: Yes, I think we mentioned that in another PR :)