Skip to content

[False positive] Method parameter typing added #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ihor-sviziev opened this issue Jul 22, 2021 · 2 comments
Closed

[False positive] Method parameter typing added #63

ihor-sviziev opened this issue Jul 22, 2021 · 2 comments

Comments

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jul 22, 2021

Hi,

This is a follow-up on the magento/magento2#33353 (comment).

While working on magento/magento2#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

  1. with strict types, pass string https://3v4l.org/eFu0n
<?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

  1. with strict types, pass int https://3v4l.org/cGn61
<?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

❗ ❌ 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
Copy link

m2-assistant bot commented Jul 22, 2021

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
Copy link
Contributor Author

ihor-sviziev commented Jul 30, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant