Dynamic property warnings in `Logger` class on PHP 8.2

[swissspidy]

Bug Report

• [x] Yes, I reviewed the contribution guidelines.
• [x] Yes, more specifically, I reviewed the guidelines on how to write clear bug reports.

Describe the current, buggy behavior

When running tests locally on PHP 8.2 (and with SQLite) I am getting the following errors:

Deprecated: Creation of dynamic property WP_CLI\Profile\Logger::$stage is deprecated in src/Logger.php on line 31

It's because of calls like $logger = new Logger( array( 'stage' => 'bootstrap' ) );. The Logger then tries to set the stage property that doesn't exist.

Describe how other contributors can replicate this bug

Run tests on PHP 8.2

I don't know why these warnings are not surfaced on CI. Is it a bug with our test suite? Error reporting disabled?

Describe what you would expect as the correct outcome

No

Let us know what environment you are running this on

(Paste the output of "wp cli info" into this box)

Provide a possible solution

If you happen to have a suggestion on how to fix this bug, please tell us in here.

Just leave this section out if you don't know how to fix it.

Provide additional context/Screenshots

Add any other context about the problem here.

If applicable, add screenshots to help explain (you can just drag&drop images into the Github issue).

[officialpiyush]

We could add the AllowDynamicProperties class to the Logger class to mark that it allows to set dynamic properties.

ref: https://www.php.net/manual/en/class.allowdynamicproperties.php

cc: @swissspidy

[thelovekesh]

The specific code lines in question:

https://github.com/wp-cli/profile-command/blob/df98093c4d28718e16ab095d77b0a82eb0351359/src/Logger.php#L31

Considering that the creation of dynamic properties is unavoidable in this context, the proposed solution seems reasonable. However, it's also important to understand why this error is not appearing in the CI.

[thelovekesh]

@swissspidy It appears that this repository doesn't contain any PHPUnit tests, and I'm not certain if the Behat tests convert deprecations into errors or at least highlight them.

[swissspidy]

I'm not certain if the Behat tests convert deprecations into errors or at least highlight them.

They do. At least locally I get 8 failed scenarios. Example:

008 Scenario: Error when SAVEQUERIES is defined to false # features/profile.feature:17
      Then STDERR should be:                             # features/profile.feature:31
        $ wp profile stage

        PHP Deprecated:  Creation of dynamic property WP_CLI\Profile\Logger::$stage is deprecated in src/Logger.php on line 31
        Deprecated: Creation of dynamic property WP_CLI\Profile\Logger::$stage is deprecated in src/Logger.php on line 31
        Error: 'SAVEQUERIES' is defined as false, and must be true. Please check your wp-config.php
        cwd: /var/folders/df/jtrbhjnd509cwxs8yb_2vw6000lhhc/T/wp-cli-test-run--6650490e451ca3.44182959/
        run time: 0.62100100517273
        exit status: 1 (Exception)

So maybe a configuration issue on CI?

Considering that the creation of dynamic properties is unavoidable in this context

Why unavoidable? We can store them differently.

[thelovekesh]

Why unavoidable? We can store them differently.

TBH I have avoided using magic method for few reasons when we have a way around it. Are there any disadvantages using #[\AllowDynamicProperties]?

[swissspidy]

AllowDynamicProperties is a very last resort if you can't avoid dynamic properties and don't want to use magic methods. I prefer avoiding such a last resort if possible :) And in this case it's very easy to avoid it.