PHP 8.2 | Indexable_*_Presentation: explicitly declare all properties

[jrfnl]

Context

• Improves compatibility with PHP 8.2

Summary

This PR can be summarized in the following changelog entry:

• Improves compatibility with PHP 8.2

Relevant technical choices:

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 each of these classes, the $pagination property is referenced multiple times throughout this class (and set via the Archive_Adjacent trait), so falls in the "known property" category.

Note: I'd be tempted to declare the property in the trait, but the trait contains a note not to do so, though I have to admit I find it hard to phantom what the problem in PHP 5.6 would be, as I use properties in traits elsewhere in other repos running on PHP 5.6 and lower, without any problems.

https://github.com/Yoast/wordpress-seo/blob/77485c4775c1421a24dd462811f022d28f7fb097/src/presentations/archive-adjacent-trait.php#L14-L15

Refs:

• https://wiki.php.net/rfc/deprecate_dynamic_properties

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

• N/A This is a code-only change and should have no effect on the functionality. If the build passes (linting, test runs), we're good.

[diedexx]

Note: I'd be tempted to declare the property in the trait, but the trait contains a note not to do so, though I have to admit I find it hard to phantom what the problem in PHP 5.6 would be, as I use properties in traits elsewhere in other repos running on PHP 5.6 and lower, without any problems.

I agree. I can't find a single reason why this wouldn't work. The original PR doesn't provide more details other than that they ran into issues with unittests on 5.6. So, I'd like to see if any tests would fail if we move the property declaration to the trait instead.

[jrfnl]

I've actioned this in commit https://github.com/Yoast/wordpress-seo/pull/18820/commits/c5491c5f836f0829ad6c5d3fb875cd3868c0ab67.

I've now also looked at the complete set of Presentations classes and their tests in more detail and when doing so I saw some more oddities in where properties were declared versus where they were actually used/set.

I've now cleaned up the property setting in this whole set of classes. See the additional commits.

Additionally, there is one class where things get really confusing - the Indexable_Static_Posts_Page_Presentation class extends the Indexable_Post_Type_Presentation class (in contrast to the Indexable_Presentation class which most of these classes extend).

The Indexable_Post_Type_Presentation class does not use the Archive_Adjacent trait and declares the $pagination property in the class itself and also has it's own generate_rel_prev() and generate_rel_next() methods (which are normally provided via the Archive_Adjacent trait) and it initializes the $pagination property in the __construct() method. So far, so good.

However, the Indexable_Static_Posts_Page_Presentation class does use the Archive_Adjacent trait, which effectively overloads the generate_rel_prev() and generate_rel_next() methods from the Indexable_Post_Type_Presentation parent class, but then doesn't call the Archive_Adjacent::set_archive_adjacent_helpers() method, but uses the $pagination property as set by Indexable_Post_Type_Presentation::__construct() in the parent class.

The Indexable_Static_Posts_Page_Presentation also (again) declared the $pagination property, while it already inherits it from both the parent class as well as the trait.

In short: the property declaration in the Indexable_Static_Posts_Page_Presentation class can definitely be removed and I was wondering whether the trait implementation in that class is correct or if it should be removed in favour of using the methods in the parent class.

The last commit - https://github.com/Yoast/wordpress-seo/pull/18820/commits/12e130616c41a520bac0a086ccce1a0909b2dd5b - removes the property, but should maybe also remove the trait use ?

[jrfnl]

The tests show an error on PHP 5.6:

1. Yoast\WP\SEO\Tests\Unit\Main_Test::test_surfaces Yoast\WP\SEO\Presentations\Indexable_Post_Type_Presentation and Yoast\WP\SEO\Presentations\Archive_Adjacent define the same property ($pagination) in the composition of Yoast\WP\SEO\Presentations\Indexable_Static_Posts_Page_Presentation. This might be incompatible, to improve maintainability consider using accessor methods in traits instead. Class was composed

😂 Kind of reads exactly like what I described above ....

I wonder if that was the reason for the extra/unneeded property declaration on the class itself, so I've just reverted the last commit to see if that makes a difference. Even so, I think a good look at the inheritance/composition of the Indexable_Static_Posts_Page_Presentation class would not be a bad thing.

[jrfnl]

Nope, doesn't solve that particular one. @diedexx Let's talk that one through when you have a moment.

[jrfnl]

We - @diedexx and me - discussed the above in a call.

To keep the changeset atomic - i.e. just address the original problem - the commit which moved the property from the classes to the trait has been removed due to the issues with PHP 5.6.

Once support for PHP 5.6 has been removed it is strongly recommended to re-submit that commit. I will keep a local copy of it for future (re-)use.