[PHP] CountOnNull on properties with PHPDoc shouldn't apply

[gnutix]

I've had the case in my code base that the following code change was triggered :

/** @var string[] $array */
$array = $someObject->someMethod();

- if (count($array) > 0) {
+ if (is_countable($array) ? count($array) > 0 : 0) {

}

I was quite surprised by this proposition, as IMO a string[] doesn't need to be surrounded by is_countable (if the developer is misusing type hints/doc blocks, PHPStan should warn him).

I digged into it and even noticed there's a test Fixture that seems to affirm this behavior :

packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Fixture/property_with_doc.php.inc

Isn't that weird ?

[TomasVotruba]

Hi, it seems weird.

Could you send failing test case to confirm?
Be sure to check PHP version in config for the test, so it's the same you use locally.

Also:

• What PHPStan version do you use?
• What Rector version do you use?

[gnutix]

I'm still having the issue using current master (on PHP 7.2) :

-        if (count($command->tags) > 0) {
+        if ((is_countable($command->tags) ? count($command->tags) : 0) > 0) {

Where $command->tags is :

    /**
     * @var Snowflake[]
     *
     * @Assert\Type("array")
     */
    public $tags = [];

I tried adding it as a failing test case, but it passed :

diff --git a/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Fixture/skip_countable_remote_property.php.inc b/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Fixture/skip_countable_remote_property.php.inc
index 7e58c01f0..025ba2e95 100644
--- a/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Fixture/skip_countable_remote_property.php.inc
+++ b/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Fixture/skip_countable_remote_property.php.inc
@@ -13,8 +13,13 @@ final class SkipCountableRemoteProperty
         $this->remote = new ClassWithCountableProperty();
     }
 
-    public function getTitle()
+    public function count(): int
     {
-        count($this->remote->countable);
+        return count($this->remote->countable);
+    }
+
+    public function collection(): int
+    {
+        return count($this->remote->collection);
     }
 }
diff --git a/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Source/ClassWithCountableProperty.php b/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Source/ClassWithCountableProperty.php
index cfb096ccc..04d4e2022 100644
--- a/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Source/ClassWithCountableProperty.php
+++ b/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Source/ClassWithCountableProperty.php
@@ -10,4 +10,7 @@ final class ClassWithCountableProperty
      * @var string[]
      */
     public $countable = ['test', 'test2'];
+
+    /** @var Snowflake[] */
+    public $collection = [];
 }
diff --git a/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Source/Snowflake.php b/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Source/Snowflake.php
new file mode 100644
index 000000000..2d1a6fda4
--- /dev/null
+++ b/packages/Php71/tests/Rector/FuncCall/CountOnNullRector/Source/Snowflake.php
@@ -0,0 +1,9 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Php71\Tests\Rector\FuncCall\CountOnNullRector\Source;
+
+final class Snowflake
+{
+}

[TomasVotruba]

Without failing test case, there is not much to do

[gnutix]

I managed to create a reproducible code set in my playground repository : gnutix/rector-playground#3 :)

Maybe a clue : running on src/ produces the error, running on src/CreateExpenseHandler.php does not.

[TomasVotruba]

Maybe a clue : running on src/ produces the error, running on src/CreateExpenseHandler.php does not.

Any idea why?
Is the class auto-loaded in composer.json?

[gnutix]

I guess it should be autoloaded properly, as Rector does not error out with autoloading messages as it often does. I'm deep in the rabbit hole, and it seems \Rector\NodeTypeResolver\NodeTypeResolver::isPropertyFetchWithArrayDefault() is the culprit :

if ($this->parsedNodesByType->findClass($propertyOwnerStaticType->getClassName()) === null) {

evaluates to false, and therefore the Rector is not skipped.

[millerf]

Reopening this convo as I was trying to migrate to 8.1 and had some concern about the fix applied for count($var), replaced with is_countable($var) ? count( $var ) : 0 whenever count() is used (I think).

The PHP documentation for count() says

Prior to PHP 8.0.0, if the parameter was neither an array nor an object that implements the Countable interface, 1 would be returned, unless value was null, in which case 0 would be returned.

So it fills the "real" fix should be something like

new_count( $var ) {

	if ( is_countable($var) ) {
		return count( $var );
	}

	if ( null === $var ) {
		return 0
	}

	return 1;
}

And the automated fix could introduce some issues?

[github-actions]

This issue has been automatically locked because it has been closed for 150 days. Please open a new issue if you have a similar problem.