Dynamic property warnings in `Logger` class on PHP 8.2
Closed this issue · 6 comments
Bug Report
- Yes, I reviewed the contribution guidelines.
- 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).
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
The specific code lines in question:
profile-command/src/Logger.php
Line 31 in df98093
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.
@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.
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.
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]?
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.