magento / magento-semver

Magento Semantic Versioning Checker. Join #svc in our Community Slack: https://opensource.magento.com/slack. Static tests that identify required module version changes based on code diff, and look for backward incompatible changes.
Other
32 stars 25 forks source link

[False positive] Method parameter typing added #63

Closed ihor-sviziev closed 3 years ago

ihor-sviziev commented 3 years ago

Hi,

This is a follow-up on the https://github.com/magento/magento2/pull/33353#issuecomment-874976048.

While working on https://github.com/magento/magento2/pull/33353, there were added following changes:

diff --git a/lib/internal/Magento/Framework/Escaper.php b/lib/internal/Magento/Framework/Escaper.php
index dae830dd889d..cb23deb0be4a 100644
--- a/lib/internal/Magento/Framework/Escaper.php
+++ b/lib/internal/Magento/Framework/Escaper.php
@@ -289,7 +289,7 @@ public function escapeUrl($string)
      * @return string
      * @since 101.0.0
      */
-    public function encodeUrlParam($string)
+    public function encodeUrlParam(string $string)
     {
         return $this->getEscaper()->escapeUrl($string);
     }
@@ -328,7 +328,7 @@ function ($matches) {
      * @return string
      * @since 101.0.0
      */
-    public function escapeCss($string)
+    public function escapeCss(string $string)
     {
         return $this->getEscaper()->escapeCss($string);
     }

The SVC failure was failing:

Level Target/Location Code/Reason
MAJOR Magento\Framework\Escaper::encodeUrlParam/lib/internal/Magento/Framework/Escaper.php:292 V085 [public] Method parameter typing added.
MAJOR Magento\Framework\Escaper::escapeCss/lib/internal/Magento/Framework/Escaper.php:331 V085 [public] Method parameter typing added.

image

Basically, adding argument type shouldn't introduce any breaking changes since PHP 7.2 (thanks to https://wiki.php.net/rfc/parameter-no-type-variance).

Examples:

Here are two examples that works fine

  1. w/o strict types, pass string https://3v4l.org/IBGCK
    
    <?php

class A { public function encodeUrlParam(string $string) { echo $string;
} }

class extendedA extends A { public function encodeUrlParam($string) { echo $string; } }

$a = new extendedA(); $a->encodeUrlParam('test');

![image](https://user-images.githubusercontent.com/1873745/126656003-8ac12455-f768-42c9-8ede-6af8e3865cda.png)

2. ✔**with strict types, pass string** https://3v4l.org/eFu0n

```php
<?php

declare(strict_types=1);

class A
{
    public function encodeUrlParam(string $string)
    {
        echo $string;    
    }
}

class extendedA extends A
{
    public function encodeUrlParam($string)
    {
        echo $string;
    }
}

$a = new extendedA();
$a->encodeUrlParam('test');

image

  1. w/o strict types, pass int https://3v4l.org/KDom7
    
    <?php

class A { public function encodeUrlParam(string $string) { echo $string;
} }

class extendedA extends A { public function encodeUrlParam($string) { echo $string; } }

$a = new extendedA(); $a->encodeUrlParam(1);

![image](https://user-images.githubusercontent.com/1873745/127630380-a5b28064-513e-4636-b53b-20648ee29825.png)

4. ✔**with strict types, pass int** https://3v4l.org/cGn61
```php
<?php

declare(strict_types=1);

class A
{
    public function encodeUrlParam(string $string)
    {
        echo $string;    
    }
}

class extendedA extends A
{
    public function encodeUrlParam($string)
    {
        echo $string;
    }
}

$a = new extendedA();
$a->encodeUrlParam(1);

image

:exclamation: ❌ Note: it doesn't work like that for return types https://3v4l.org/ti9uU

<?php

class A
{
    public function encodeUrlParam(string $string): string
    {
        echo $string;    
    }
}

class extendedA extends A
{
    public function encodeUrlParam($string)
    {
        echo $string;
    }
}

$a = new extendedA();
$a->encodeUrlParam('test');

image

m2-assistant[bot] commented 3 years ago

Hi @ihor-sviziev. Thank you for your report. To help us process this issue please make sure that you provided sufficient information.

Please, add a comment to assign the issue: @magento I am working on this


ihor-sviziev commented 3 years ago

As we discussed with @sivaschenko in Slack - ✔ that works fine for the client code, in case if the class class doesn't have any inheritance and *strict types not declared in the client code: https://3v4l.org/NBiQU

<?php

class A
{
    public function encodeUrlParam(string $string)
    {
        echo $string;    
    }
}

$a = new A();
$a->encodeUrlParam(1);

image

❌ that doesn't for the client code, in case if the class doesn't have any inheritance and declared strict types in the client code: https://3v4l.org/3ovqX

<?php

declare(strict_types=1);

class A
{
    public function encodeUrlParam(string $string)
    {
        echo $string;    
    }
}

$a = new A();
$a->encodeUrlParam(1);

image