solariumphp / solarium

PHP Solr client library
Other
932 stars 301 forks source link

Broken support for Zend_Config (now Laminas) #1130

Closed thomascorthals closed 2 months ago

thomascorthals commented 3 months ago

Solarium version(s) affected: 5.x+ Solr version: N/A Solr mode: N/A

Description According to the docs, Zend_Config can be used when creating a Client instance.

https://github.com/solariumphp/solarium/blob/19838edfaa4717dc7b12fdaf49e1c964db606d58/docs/queries/select-query/building-a-select-query/building-a-select-query.md?plain=1#L97

However, since #662 the constructor and the methods such as createSelect() have an array $options type hint that no longer allows passing an object.

How to reproduce

new Client(new Curl(), new EventDispatcher(), new Laminas\Config\Config([]));

Results in:

TypeError: Solarium\Core\Client\Client::__construct(): Argument #3 ($options) must be of type ?array, Laminas\Config\Config given

Possible Solution We could go back and remove this type hint. But since it's been there for quite a while without anyone complaining, I doubt anyone still uses it like that.

I think it's better to completely remove the object support from Configurable::setOptions(). Don't even think deprecating it first is necessary if nobody noticed this was broken.

It would still be very easy to use config objects if you want. A very small effort on the user's part can cut the complexity out of Configurable.

// with Laminas
new Client(new Curl(), new EventDispatcher(), new Laminas\Config\Config([])->toArray());

// with your own object
new Client(new Curl(), new EventDispatcher(), json_decode(json_encode($object), true));

Additional context Was actually looking to update the documentation from Zend → Laminas and include a unit test that actually uses laminas/laminas-config when I noticed this no longer works. Even the object support in Configurable is partly broken, specifically if you try and rely on recursive calls of the internal toArray() method.

thomascorthals commented 3 months ago

@mkalkbrenner What do you think? Should I just remove object support from Configurable or do you prefer deprecating it first just to be safe?

mkalkbrenner commented 2 months ago

I agree with your opinion. We should remove the support entirely.