Skip to content
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

PHP 8.2: explicitly declare properties #705

Merged

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Dec 18, 2021

Context

Dynamic (non-explicitly declared) property usage is expected to be deprecated as of PHP 8.2 and will become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

  • If it's an accidental typo for a declared property: fix the typo.
  • For known properties: declare them on the class.
  • For unknown properties: add the magic __get(), __set() et al methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods build in.
  • For unknown use of dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

Refs:

Commits

Locator: explicitly declare all properties

In this case, the $dom property is referenced multiple times throughout this class, so falls in the "known property" category.

Sanitize: explicitly declare all properties

In this case, the $registry property is referenced multiple times throughout this class, so falls in the "known property" category.

Autoloader: explicitly declare all properties

In this case, the $path property is referenced a couple of times throughout this class, so falls in the "known property" category.

🆕 SimplePie_Cache_Redis: fix bug for using undefined property

The $ttl property is not declared, not set on this cache, nor its parent, which means that it would always be passed as null.

As the call to the Redis::expire() method is within a if ($this->options['expire']) condition and a similar call to the Redis::expire() method in the SimplePie_Cache_Redis::save() method, also uses $this->options['expire'] for the "ttl" value, changing the passed parameter to $this->options['expire'] seems the correct fix.

QA

The existing unit tests already cover these changes.

A scan with @exakat reveals one more issue, but I'm not sure how to fix that one:

The SimplePie_Cache_Redis::touch() method refers to (uses) a $ttl property, but this property is not declared on the class, nor set anywhere, so it is unclear what the value of the property should be when explicitly declaring it on the class.

Dynamic (non-explicitly declared) property usage is expected to be deprecated as of PHP 8.2 and will become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:
* If it's an accidental typo for a declared property: fix the typo.
* For known properties: declare them on the class.
* For unknown properties: add the magic `__get()`, `__set()` et al methods to the class or let the class extend `stdClass` which has highly optimized versions of these magic methods build in.
* For unknown _use of_ dynamic properties, the `#[AllowDynamicProperties]` attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the `$dom` property is referenced multiple times throughout this class, so falls in the "known property" category.

Refs:
* https://wiki.php.net/rfc/deprecate_dynamic_properties
Dynamic (non-explicitly declared) property usage is expected to be deprecated as of PHP 8.2 and will become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:
* If it's an accidental typo for a declared property: fix the typo.
* For known properties: declare them on the class.
* For unknown properties: add the magic `__get()`, `__set()` et al methods to the class or let the class extend `stdClass` which has highly optimized versions of these magic methods build in.
* For unknown _use of_ dynamic properties, the `#[AllowDynamicProperties]` attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the `$registry` property is referenced multiple times throughout this class, so falls in the "known property" category.

Refs:
* https://wiki.php.net/rfc/deprecate_dynamic_properties
Dynamic (non-explicitly declared) property usage is expected to be deprecated as of PHP 8.2 and will become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:
* If it's an accidental typo for a declared property: fix the typo.
* For known properties: declare them on the class.
* For unknown properties: add the magic `__get()`, `__set()` et al methods to the class or let the class extend `stdClass` which has highly optimized versions of these magic methods build in.
* For unknown _use of_ dynamic properties, the `#[AllowDynamicProperties]` attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the `$path` property is referenced a couple of times throughout this class, so falls in the "known property" category.

Refs:
* https://wiki.php.net/rfc/deprecate_dynamic_properties
@jtojnar
Copy link
Contributor

jtojnar commented Dec 19, 2021

I think $this->ttl was supposed to be $this->options['expire'] – see the other instance of Redis::expire method being called.

@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 19, 2021

@jtojnar

I think $this->ttl was supposed to be $this->options['expire'] – see the other instance of Redis::expire method being called.

Good call, makes perfect sense. I've added an extra commit to address this.

The `$ttl` property is not declared, not set on this cache, nor its parent, which means that it would always be passed as `null`.

As the call to the `Redis::expire()` method is within a `if ($this->options['expire'])` condition and a similar call to the `Redis::expire()` method in the `SimplePie_Cache_Redis::save()` method, also uses `$this->options['expire']` for the "ttl" value, changing the passed parameter to `$this->options['expire']` seems the correct fix.
@jrfnl jrfnl force-pushed the feature/php-8.2-explicitly-declare-properties branch from cbee5e5 to ff2f3c8 Compare December 19, 2021 05:30
Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised var is still allowed in PHP 8, GitHub syntax highlighter does not seem to know it 🤣

@mblaney mblaney merged commit 76be63e into simplepie:master Dec 19, 2021
@mblaney
Copy link
Member

mblaney commented Dec 19, 2021

thanks @jrfnl

@jrfnl jrfnl deleted the feature/php-8.2-explicitly-declare-properties branch December 19, 2021 08:00
@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 19, 2021

I am surprised var is still allowed in PHP 8, GitHub syntax highlighter does not seem to know it 🤣

It is, though it is highly recommended to use visibility modifiers instead. Happy to change that throughout the codebase, but that was out of scope for this PR.

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

Successfully merging this pull request may close these issues.

None yet

3 participants