Strict types in next major version?

[franmomu]

Analyzing the code with PHPStan level to 5, there are about 656 errors and most of them (~95%) are like this:

 ------ -----------------------------------------------------------------------------------------
  38     Parameter #1 $id of class Elastica\Document constructor expects string|null, int given.
  73     Parameter #1 $id of class Elastica\Document constructor expects string|null, int given.
  157    Parameter #1 $id of class Elastica\Document constructor expects string|null, int given.
 ------ -----------------------------------------------------------------------------------------

because in tests when creating Document, it is used an int like:

$id = 1;
$data = ['id' => $id, 'name' => 'Item 1'];
$doc = new Document($id, $data);

and Document constructor has string has parameter type:

Elastica/src/Document.php

Lines 36 to 43 in 682b78d

	/**
	* Creates a new document.
	*
	* @param string|null $id The document ID, if null it will be created
	* @param array|string $data Data array
	* @param Index|string $index Index name
	*/
	public function __construct(?string $id = null, $data = [], $index = '')

So should we ignore those errors? or are we going to add declare(strict_types = 1); in next major version?

IMHO adding strict types is the way to go (and changing all int ids to string in tests).

[ruflin]

I think the following should stay valid:

$doc = new Document(1, $data);

I know it is not strict but it is one of these nice convenience things about script languages. What we send in the end to Elasticsearch is a string. But my biggest concern is breaking all the users that might use it today and force them to change the code. Is this really worth it? What if we modify to allow int?

[franmomu]

I think the following should stay valid:

$doc = new Document(1, $data);

yep, I agree with that.

I know it is not strict but it is one of these nice convenience things about script languages. What we send in the end to Elasticsearch is a string. But my biggest concern is breaking all the users that might use it today and force them to change the code. Is this really worth it? What if we modify to allow int?

Yeah, adding int is ok to me, I'll create a PR after reaching level 4 👍

[ruflin]

Great, I assume this is aligned with your strict type goal?

[franmomu]

yeah, there are some other inconsistencies, we'll see them

[thePanz]

Should we close this issue as PR #2093 has been merged?

[franmomu]

Should we close this issue as PR #2093 has been merged?

👍