How to find all classes that are getting dynamic properties assigned?

sneakyimp

I'm working on a site implemented in CodeIgniter 3. We are trying to upgrade the server because it's running Ubuntu 16 / PHP 7 which are both now EOL. The website code works pretty great with PHP 8.1 but the latest release is PHP 8.2.1 and we'd like to get as up-to-date as possible if we are going through the trouble of upgrading. Unfortunately, PHP 8.2 deprecates dynamic properties for a class object, and CI3 rampantly assigns such dynamic properties.

I know we can address this problem by A) creating magic __set and __get methods or by B) adding this new attribute before each class that endures this abuse:

#[\AllowDynamicProperties]
class FooClass {
    // blah blah blah
}
$foo = new FooClass();
$foo->some_new_property = 42;

Question 1: Which solution do you guys prefer for this sort of problem?
The magic methods approach, A, might let you include some logic to respond to possibly abusive behaviors, Whereas the attribute approach, B, is briefer.

Question 2: Is there some efficient way to identify all the classes that undergo this abuse?
I wonder if there is some IDE tool that might detect this problem? Or can anyone suggest an approach to identifying all the poor classes that are subject to this abominable mistreatment?

EDIT: Note that there's a discussion of this very question on github. If anyone is interested in chiming in over there, it would probably draw more attention to the issue, which would be nice.

NogDog

Had some time on my hands this afternoon, so I threw this together, and it seems promising...but not rigorously tested. 🙂 (Probably need to add something for self::$something = ..., too.)

#!/usr/bin/env php
<?php
/**
 * Look for undeclared property assignments
 * 
 * USAGE:
 *   deprecated.php ( file | directory )
 */

if(empty($argv[1])) {
  die("USAGE: deprecated.php ( file | directory )\n");
}

$files = [];
$input = rtrim($argv[1], '/');
if (is_dir($input)) {
  $files = glob("$input/*.php");
  if (!$files) {
    die("No PHP files found in $input\n");
  }
} elseif (is_file($input)) {
  $files[] = $input;
} else {
  die("Nothing found for $input\n");
}

$assignment_regex = '/\$this->(\w+)\s*=/';
$find_regex = '/(private|protected|public)(\s+\w+)?\s+\$%s\b/';

foreach ($files as $file) {
  echo "$file\n";
  $text = file_get_contents($file);
  if (trim($text) === '') {
    die("File not found/loaded\n");
  }

  preg_match_all($assignment_regex, $text, $matches);
  foreach ($matches[1] as $name) {
    $regex = sprintf($find_regex, $name);
    if (!preg_match($regex, $text)) {
      echo "  * No declaration found for \$this->$name\n";
    }
  }
}

benanamen

Answer 1: The decision between the two solutions depends on your specific use case and your team's preferences. The magic methods approach, A, may be more flexible as it allows for more complex logic to be executed when the dynamic properties are set or accessed. However, the attribute approach, B, is simpler and less prone to errors, and it would work just as well for most use cases.

Answer 2: One way to identify all the classes that undergo this abuse would be to search the codebase for instances of dynamic property assignments, such as $foo->some_new_property = 42;. This can be done using a text editor with a "find in files" feature or using a command line tool such as grep. Additionally, some IDEs, such as PhpStorm, have built-in features to search for specific patterns in the codebase, which could be used to identify the dynamic property assignments.

Another approach is to use a code analysis tool that can identify instances of deprecated or problematic code. Some popular examples include Psalm, PHPStan, and Phan. These tools can often be integrated with your development environment, such as with a plugin for your text editor or IDE, and can help you identify potential issues with the codebase, including instances of dynamic property assignments.

Overall, it's important to keep in mind that upgrading to PHP 8.2 may involve changes to your codebase, so it's essential to thoroughly test your application before deploying it to a production environment.

NogDog

FYI: I tweaked my script so that it would recursively search through the app I'm looking at, found over a 1000 suspect lines of code, then realized it raises a false positive if the property is declared in a parent class that it inherits from. 🙁

Weedpacket

NogDog Yeah, that's why I'd prefer to go with static analysis. I hope you've declared your variable types, so that something looking at $foo->property has some chance of knowing what class $foo is supposed to be to know whether it has a property.

sneakyimp

NogDog FYI: I tweaked my script so that it would recursively search through the app I'm looking at, found over a 1000 suspect lines of code, then realized it raises a false positive if the property is declared in a parent class that it inherits from. 🙁

I'm grateful for the code you offer, and applaud your efforts. I've not yet been able to dig in with your code but it I think it would also miss instances where some object gets assigned a dynamic property by code outside the scope of the class itself, e.g.:

include './classes/SomeClass.php';
$foo = new SomeClass();
$foo->dynamic = 42;

sneakyimp

Weedpacket
Your point about declaring types is more on point than I realized. I reported an issue about PHPStan not recognizing objects returned by a factory method and it was immediately closed with this comment:

It's telling you you're missing a return type. If you add it, it works as expected
https://phpstan.org/r/ba5f1399-ea4e-44d8-bd75-5c37be8a64a1

NogDog

sneakyimp it would also miss instances where some object gets assigned a dynamic property by code outside the scope of the class itself

Yeah, I was wondering that, too (with step one being to find out if PHP allows it or not 😉 ). Just started poking around yesterday to figure out which static analysis tool(s) might be best to play with. Fortunately, this is not a high priority, as being on 8.1 is well ahead of where we were a year ago, still on 5.x. 😉

NogDog

PS: Just answered my own question:

$ php -a
Interactive shell

php > echo phpversion();
8.2.1
php > class test {};
php > $test = new test();
php > error_reporting(E_ALL);
php > ini_set('display_errors', true);
php > $test->foo = 'foo';
PHP Deprecated:  Creation of dynamic property test::$foo is deprecated in php shell code on line 1

Deprecated: Creation of dynamic property test::$foo is deprecated in php shell code on line 1
php >

sneakyimp

i did a bit of google searching and saw PHPStan (which was mentinoed by @benanamen ) and decided to give it a try. You can install it with composer:

composer require --dev phpstan/phpstan

If you just run it, it will not detect undeclared properties, but if you add the -l flag with a level of 2 or higher, it will report 'undefined properties'. I created these files in a subdir, foo:

foo.php

<?php
class FooClass {
    // blah blah blah
}

file.php

<?php
require_once 'foo.php';
$foo = new FooClass();
$foo->some_new_property = 42;

and ran it:

$ vendor/bin/phpstan analyse -l 2 foo
 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ --------------------------------------------------------------- 
  Line   file.php                                                       
 ------ --------------------------------------------------------------- 
  4      Access to an undefined property FooClass::$some_new_property.  
 ------ --------------------------------------------------------------- 

 [ERROR] Found 1 error

This looks quite helpful. If anyone has experience with PHPStan or other static analysis tools, I'd be curious to hear any war stories.

EDIT: I got to wondering if PHPStan was smart enough to deal with instantiation of classes via factory methods. I seem to recall seeing this pattern a lot. To be extra sneaky I have my factory method return an instance of self rather than referring to the class name. Looks like PHPStan isn't smart enough to recogize the undeclared property assignment.
file2.php

<?php
require_once 'foo2.php';
$foo = FooClass2::factory();
$foo->some_new_property = 42;
var_dump($foo);

foo2.php

<?php
require_once 'foo.php';
class FooClass2 extends FooClass {
    // let's see if phpstan can detect factory methods
    public static function factory(){
        return new self();
    }
}

The analysis doesn't catch the undeclared property assignment in file2.php. If you crank up the reporting level to 6, it does complain that you haven't declared a return type for the factory method in foo2.php:

$ vendor/bin/phpstan analyse -l 6 foo
 4/4 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ --------------------------------------------------------------- 
  Line   file.php                                                       
 ------ --------------------------------------------------------------- 
  4      Access to an undefined property FooClass::$some_new_property.  
 ------ --------------------------------------------------------------- 

 ------ ----------------------------------------------------------- 
  Line   foo2.php                                                   
 ------ ----------------------------------------------------------- 
  5      Method FooClass2::factory() has no return type specified.  
 ------ ----------------------------------------------------------- 

 [ERROR] Found 2 errors