Make Backdrop core compatible with PHP 8.2

[klonos]

Our Drupal brethren have started some work on this in a meta: https://www.drupal.org/project/drupal/issues/3304807

[klonos]

...should we have multiple issues/PRs, or a single PR with multiple commits, and then merge once tests pass?

[klonos]

...for those of us using Lando for our locals, php 8.2 is not supported yet. Follow lando/php#40 for updates.

[argiepiano]

I think a meta is always best, then developers can tackle each issue separately and update the meta as they do so

[klonos]

During this week's dev meeting we have decided to do the following:

• attempt to get a single PR, which (initially only) disables all tests against php 5.6 and 7.4, and switches the php 8 tests to be running against 8.2
• the goal is to have the tests pass, but if the PR gets unwieldy (indicatively, in the order of a few 100s of lines of non-trivial changes), then split off to multiple issues/PRs
• once tests pass against php 8.2, we can add back the php 5.6 and 7.4 checks

(point in time during the dev meeting when this issue was discussed: https://youtu.be/vvzsOmfyIsA?t=2624)

[klonos]

I have done most of the legwork, which is currently 148 changed files with mostly trivial-to-review changes. Still not ready for review, but there's heaps less tests failing now, and no obvious errors thrown in the frontend.

I will start documenting certain things that most likely also apply to us, but unsure how to handle. Starting with this:

https://www.drupal.org/project/drupal/issues/3280033

PHP has deprecated dollar brace string interpolation in version 8.2 https://wiki.php.net/rfc/deprecate_dollar_brace_string_interpolation

Commit against D9.4: https://git.drupalcode.org/project/drupal/-/commit/6496252

We do have a few such instances of ${, but not 100% sure how to best handle all of them. I guess that I'll leave that to someone else.

[klonos]

...OK, this feels pretty close now, but I'm starting to see red and blue dragons flying around my head ...they are spinning fast, so I'm starting to see some of them as purple 😅

I have resulted in using #[\AllowDynamicProperties] in some instances (mostly for views-related things, which is what the Drupal maintainers of Views 7.x ended up doing), but we can follow up on these to sort them properly later on.

Can someone else now please review my PR so far and take it from here? The majority of the changes are trivial (like adding protected to test variables etc.) and I've tried doing multiple smaller commits. I also have added links to respective d.org issues where applicable, so I hope that that will make things a bit easier.

Should we merge what we have now, and iterate with other PRs?

Pinging our @backdrop/core-committers

[klonos]

...there is a big amount of the following failures:

• Creation of dynamic property date_sql_handler::$placeholders is deprecated: not sure how to best address this, but it should knock out a big chunk of failures.
• Creation of dynamic property [WHATEVER]::$originalLanguage is deprecated: should we simply add a protected for each instance of $originalLanguage in each of the failing tests, or is there a "centralized" way to make this go away?
• Creation of dynamic property Layout::$is_new is deprecated and Creation of dynamic property Layout::$original_name is deprecated: I would like others with deeper knowledge of Layouts to chime in re these. @docwilmot and @argiepiano come to mind, but anyone else with thoughts really. That should also knock out a huge chunk of failures.

[klonos]

...for those of us using Lando for our locals, php 8.2 is not supported yet. Follow lando/php#40 for updates.

Lando v3.9.0 was just released, which adds support for PHP 8.2 (just tested it on my local). It's a prerelease, so you can't get it with brew at the time of writing - you'll need to manually download it from https://github.com/lando/lando/releases

[indigoxela]

One question, I can't answer, yet: will we need #[AllowDynamicProperties] for entities like Node?

In D7 Node is just a stdClass, and with stdClass dynamic properties are still allowed. However, in B Node (and User, Comment, File, TaxonomyTerm) are fully classed.

But... abstract class Entity extends stdClass implements EntityInterface - wait, does that mean that dynamic properties are allowed with core Entities? As they extend stdClass... 🤔

Why we need that? Take a look at the book module (it uses dynamic properties), same with contrib (poll module, for example). Heck it's even officially documented that way.

Who knows it? Can core entities have dynamic properties, or not?

[indigoxela]

@klonos I've left some questions on your PR.

[herbdool]

From reading https://wiki.php.net/rfc/deprecate_dynamic_properties I'd say yes. Any class that extends stdClass will allow dynamic properties.

[argiepiano]

After some basic testing in PHP 8.2 I can confirm there is no deprecation warning when you use dynamic properties in classes that extend stdClass. Since Backdrop's Entity extends stdClass, this means that all Backdrop core entities that extend Entity (i.e. User, Node, Comment, File and TaxonomyTerm) will not need any adjustments such as #[\AllowDynamicProperties] or others.

[argiepiano]

As for the other questions in @klonos comment above: I believe, in general there are three options:

• Either make the first parent extend stdClass
• or, declare the properties (e.g. it'd be easy to declare Layout::$is_new and Layout::$original_name)
• or, add #[\AllowDynamicProperties] to the top superclass (this may be the way to go with Views handler superclasses like views_object)

I think number 1 above may be the simpler way, but I really don't know if there may be any negative repercussions about changing classes like Layout and all views superclasses to extend stdClass.

EDIT:
According to this post, extending stdClass is harmless, and has the benefit of making dynamic properties available out of the box (the most recent comment there is from a few months ago).

There are classes in BAckdrop that AFAIK don't use dynamic properties - e.g. Config and all the entity controllers. So, in these cases we don't need to do anything.

[herbdool]

I prefer adding #[\AllowDynamicProperties] instead of extending stdClass because it's more obvious and we also don't know the downsides of extending stdClass.

[indigoxela]

So, we don't have to do anything with core Entity class and all classes that extend it. That's great! 👍

Re Simpletest - I think, it's clean (and safe) to declare all properties. A lot of legwork, but no #[\AllowDynamicProperties] necessary.

Re Layout - declaring all properties should be possible and fix the problem. Same for other non-views classes in core, I guess.

Re Views: I'd suggest to add #[\AllowDynamicProperties] to the base class(es). That reduces the amount of changes and also fixes contrib classes, that extend those base classes. What I read so far, this attribute won't get removed in PHP 9, which gives us time. For PHP 10 we might want to implement __get and __set magic methods in base classes, that still need dynamic properties. Didn't check details for that, yet.

@argiepiano I don't think, switching views base classes to extend stdClass provides more than the new attribute on classes, or the magic methods. Or do you see any extra benefit?

@klonos as very often, you've been ahead of us with starting this issue. Many thanks for your work so far. 🙏 I hope, you'll be able to further work on this. 😏

[klonos]

I am removing myself from the assignee of this issue, since as I also explained in a related thread in Zulip: I want to be helpful, but at the same time I need to be realistic. With the new job, a lot of my energy is being spent on new knowledge and my focus is on that at the moment. Because I am out of my normal routines, I end up being more drained at the end of my work day than before, so I need more time to relax and wind down than usual. I will eventually get back to things, however, if someone else has the energy/capacity, please feel free to either take over from my current PR, or create your own from scratch.

[indigoxela]

Friendly reminder that this PR still needs a final review (a lot has already happened a month ago). 🙌

If someone's concerned because of the failing phpcs check: that's just because our GHA can't handle such big changes (82 files). I'm pretty sure, all my changes comply with our coding standards. 😁

[herbdool]

A couple more tweaks and I think it's ready @indigoxela. Thanks for your work on this.

[indigoxela]

@herbdool that's great! Many thanks for reviewing, I've addressed your requests.

[herbdool]

I think it's good to go!

[quicksketch]

I code reviewed this and it looks great! Nice job @indigoxela defining all those missing properties! This will really substantially reduce the amount of warnings I get in PHPStorm, and help with type-hinting as well. All around a big set of improvements and clean-up!! This is a big effort and thank you so much for the work.

The only thing I found in my read-through was that we're not consistent with #[AllowDynamicProperties] vs #[\AllowDynamicProperties]. More in the code review here: backdrop/backdrop#4445 (review)

[quicksketch]

I added a follow-up issue regarding whether we should use a backslash or not when defining class attributes here: Coding Standards Policy: Unqualified or qualified global class names #6169

[indigoxela]

The only thing I found in my read-through was that we're not consistent with #[AllowDynamicProperties]

Right, that's an oversight and fixed now, to be consistent. Many thanks for pointing me to that and for reviewing. 🙏

[quicksketch]

Looks like @herbdool also re-approved this in the PR. Looks good to me! I'll let tests finish running to be safe.

[quicksketch]

I merged backdrop/backdrop#4445 into 1.x and 1.25.x (no conflicts!). Thank you so much @indigoxela, @herbdool, @argiepiano, @klonos, and @yorkshire-pudding for the extensive work put into this! I really like where this ended and the updates will have a lot of other peripheral benefits, like better code hinting and IDE support. Very nice!