Update TP: 45_static_method_from_variable

[felix-20]

Testability pattern

Pattern 45_static_method_from_variable

Problem statement

The original code:

<?php
  class A {
      public $one = 1;

      function __construct($b){
        $this->one = $b;
      }
    
      static function show_one() {
          echo $this->one;
      }
  }
  
$b = $_GET["p1"];
$a = new A($b);
$a::show_one();

When trying to run this code on my machine with PHP 8.1.2 and Zend Engine v4.1.2 it throws a Fatal error.
In my opinion that is correct. The instance code does not work, because $this is accessed in the static function show_one() which should not be possible as $this refers to a specific instance of a class, while the static function show_one() refers to the class itself.

Proposed changes

In my understanding, this pattern targets, that a static function can be called from an instance of the class.
If that is the right understanding, changing the variable $one from public to static should fix the problem.
The changed code could look like this:

<?php
  class A {
      static $one = 1;

      function __construct($b){
        self::$one = $b;
      }
    
      static function show_one() {
        return self::$one;
      }
  }
  
$b = $_GET["p1"]; // source
$a = new A($b);
$c = $a::show_one();
echo $c; // sink

[compaluca]

@enferas : what do you think?

[enferas]

Thank you for the suggestion.

Yes I agree that the current code will throw an error.

The only issue that the suggested solution will mix this testability pattern with static property pattern.

But I don't see other solution than creating multiple instances one with static property and one with global variable.

[felix-20]

@enferas Thank you for your answer.

Indeed, the pattern 29 (static properties) looks very similar to the proposed change. But when looking at pattern 28 (static method) it is actually the same as the proposed change. So I think I have not yet fully understood, what this pattern 45 should stress. The discovery rules of pattern 28 and 45 are very similar as well.

Pattern 45:

 cpg.call.name(".*INIT_STATIC_METHOD_CALL.*").argument.order(1).code("CV.*|T.*|V.*").location.toJson

Pattern 28:

cpg.call(".*INIT_STATIC_METHOD_CALL.*").location.toJson

So does it mean, that pattern 45 expects an argument for INIT_STATIC_METHOD_CALL, while pattern 28 does not care?
Another possiblity could be, that this pattern wants to stress the static method, taking a variable as an argument, so the code could look something like this:

<?php
  class A {    
      static function show_one($a) {
        return $a;
      }
  }
  
$b = $_GET["p1"]; // source
$a = new A();
$c = $a::show_one($b);
echo $c; // sink

But in my opinion, this code could just be an instance of pattern 28. What am I missing?

[compaluca]

@enferas : maybe this pattern is just a duplication and we can remove it?

[enferas]

First, if I understand correctly we are comparing between patterns 28 and 45.

45 requires type inference to know the object is created from which class to know which static method will be called.

28 doesn't require type inference.

28 discovery rule will look for all the static method calls, while 45 will look for all static method calls when the call from a class instance.

Thus, 28 cover 45 and I think we will need to improve the discovery rule for 28 to not cover 45. While, I think they are two different cases and it is better to not merge them.