Relax value type in SetProcessor to allow mixed

[franmomu]

It is defined as string, but in tests there are a couple of places where another type is used:

Elastica/tests/Processor/SetProcessorTest.php

Line 20 in 732ccf3

$processor = new SetProcessor('field1', 582.1);

Elastica/tests/PipelineTest.php

Line 26 in 8ecc5cd

$set = new SetProcessor('field4', 324);

Looking at the documentation: https://www.elastic.co/guide/en/elasticsearch/reference/7.17/set-processor.html#set-processor looks like any type can be used.

[deguif]

That's would be a BC break for anyone who reimplemented this method and kept the string $value signature.
But it should be very limited as extending this processor is probably not widespread at all.

[ruflin]

Agree, but I also hope this is very rare. I suggest we mark it as a breaking change in the changelog with a note on when it would be breaking (someone inheriting SetProcessor).

It is one of these scenarios I think where in theory it is a breaking change and I'm pretty sure someone is going to bring up that because of that we (I) don't follow semver but working around it would not bring benefits to the large majority of users. I'm +1 on the change.

[franmomu]

I'll update the PR later adding it in the BC Break section, it wasn't intentional, I changed it in the constructor which is not BC break and then changed it in the setValue without thinking about that.

[franmomu]

There is also another similar issue with Setting::setMergePolicy():

Elastica/src/Index/Settings.php

Lines 262 to 266 in ff1a6e0

	* @param string $key Merge policy key (for ex. expunge_deletes_allowed)
	*
	* @see https://www.elastic.co/guide/en/elasticsearch/reference/current/index-modules-merge.html
	*/
	public function setMergePolicy(string $key, string $value): Response

It expects a string, but in tests:

Elastica/tests/Index/SettingsTest.php

Lines 239 to 243 in 4321f37

	$settings->setMergePolicy('expunge_deletes_allowed', 15);
	$this->assertEquals(15, $settings->getMergePolicy('expunge_deletes_allowed'));
	$settings->setMergePolicy('expunge_deletes_allowed', 10);
	$this->assertEquals(10, $settings->getMergePolicy('expunge_deletes_allowed'));

and looking at https://github.com/elastic/elasticsearch/blob/56d89577a033c15b518e2d74e55d9605eb90b78a/server/src/main/java/org/elasticsearch/index/MergePolicyConfig.java

seems like it could accept at least int as well.

should we proceed the same way?

[ruflin]

Yes, I would say lets do the same.

[ruflin]

Can you expand this a bit more and explain in which scenarios it is breaking? Basically only if someone inherits from SetProcessor