backdrop
PHP 8.4 Support
Description of the bug
PHP 8.4 was released last month and continues to tighten the screws on less-than-perfect PHP code. Immediately upon switching to PHP 8.4, the homepage of Backdrop has the following errors:
Deprecated: Constant E_STRICT is deprecated in /var/www/html/core/includes/bootstrap.inc on line 742
Deprecated: Constant E_STRICT is deprecated in /var/www/html/core/includes/errors.inc on line 28
Deprecated: backdrop_get_query_parameters(): Implicitly marking parameter $query as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/common.inc on line 497
Deprecated: _format_date_callback(): Implicitly marking parameter $matches as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/common.inc on line 2541
Deprecated: actions_execute(): Implicitly marking parameter $entity as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/actions.inc on line 85
Deprecated: comment_access(): Implicitly marking parameter $comment as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/comment/comment.module on line 1513
Deprecated: entity_access(): Implicitly marking parameter $entity as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/entity/entity.module on line 191
Deprecated: entity_access(): Implicitly marking parameter $account as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/entity/entity.module on line 191
Deprecated: entity_build_content(): Implicitly marking parameter $entity as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/entity/entity.module on line 225
Deprecated: taxonomy_vocabulary_static_reset(): Implicitly marking parameter $ids as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/taxonomy/taxonomy.module on line 853
Deprecated: user_has_role(): Implicitly marking parameter $account as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/user/user.module on line 576
Deprecated: UpdateQuery::expression(): Implicitly marking parameter $arguments as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/database/query.inc on line 1157
Deprecated: MergeQuery::expression(): Implicitly marking parameter $arguments as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/database/query.inc on line 1420
Deprecated: SelectQueryInterface::getArguments(): Implicitly marking parameter $queryPlaceholder as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/database/select.inc on line 177
Deprecated: SelectQueryInterface::preExecute(): Implicitly marking parameter $query as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/database/select.inc on line 505
Deprecated: SelectQueryExtender::getArguments(): Implicitly marking parameter $queryPlaceholder as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/database/select.inc on line 812
Deprecated: SelectQueryExtender::preExecute(): Implicitly marking parameter $query as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/database/select.inc on line 826
Deprecated: SelectQuery::getArguments(): Implicitly marking parameter $queryPlaceholder as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/database/select.inc on line 1549
Deprecated: SelectQuery::preExecute(): Implicitly marking parameter $query as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/includes/database/select.inc on line 1567
Deprecated: EntityControllerInterface::resetCache(): Implicitly marking parameter $ids as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/entity/entity.controller.inc on line 27
Deprecated: EntityControllerInterface::resetStaticCache(): Implicitly marking parameter $ids as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/entity/entity.controller.inc on line 36
Deprecated: DefaultEntityController::resetCache(): Implicitly marking parameter $ids as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/entity/entity.controller.inc on line 217
Deprecated: DefaultEntityController::resetStaticCache(): Implicitly marking parameter $ids as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/entity/entity.controller.inc on line 235
Deprecated: system_menu_tree_prune_tree(): Implicitly marking parameter $parent_item as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/core/modules/system/system.menu.inc on line 370
There are probably many other situations where this occurs. To fix this, our goal should be to bump the test runners from PHP 8.3 to 8.4 and have 100% tests passing.
Steps To Reproduce
To reproduce the behavior:
- Install Backdrop normally on PHP 8.3 within DDEV.
- Edit the
.ddev/config.ymlfile and change the php line tophp: 8.4 - Restart DDEV to take effect
ddev restart - Visit the homepage, i.e.
https://backdrop.ddev.site
Additional information
Corresponding Drupal 10/11 issue is here: https://www.drupal.org/project/drupal/issues/3427903 Corresponding Drupal 7 issue: ?? There may not be one
Add any other information that could help, such as:
- Backdrop CMS version: 1.x (1.30.x)
- Web server and its version: Apache 2.4.62
- PHP version: 8.4.1
- Database sever (MySQL or MariaDB?) and its version: 10.11.10
- Operating System and its version: Ubuntu 24.04
- Browser(s) and their versions: all
Initial PR filed at https://github.com/backdrop/backdrop/pull/4930 to run the tests.
So we have to make them not nullable, or make the minimum version PHP 7.1? https://wiki.php.net/rfc/deprecate-implicitly-nullable-types. Seems like the explicit nullable assignment was only introduced in 7.1.
Implicitly marking parameter $ids as nullable is deprecated, the explicit nullable type must be used instead
This problem (and all the similar ones) aren't easy to solve. For a function declaration like this: function foo(array $bar = NULL), we have the following options:
- Make all parameters with a type not have a default of
= NULL(that's what my PR started doing initially):function foo(array $bar = array()) - Remove the type from the parameter:
function foo($bar = NULL). - Bump the minimum PHP version to 7.1 and declare nullable with
?:function foo(?array $bar)
Unfortunately both the first and second options will cause API breaks, especially where classes have declared interfaces or base classes as types in their method declarations. Removing the type cast causes an API break because PHP 7.2 requires declarations to be identical between parent and child classes.
It may be that our only option for PHP 8.4 support is to drop support for PHP 7.1 and below? It does look like about 2% of sites are on those older versions.
Here's the corresponding issue for Drupal: https://www.drupal.org/project/drupal/issues/3427903 but it's only for Drupal 10/11. Considering the Drupal 7 EOL is next month, PHP 8.3 may be the last version of PHP that can run Drupal 7? I'm curious what the LTS support vendors are going to do about this situation.
I looked to see what WordPress did. There's a thread here https://core.trac.wordpress.org/ticket/60786. There are new commits which declare nullable. And they have also bumped up the minimum version to 7.2. https://core.trac.wordpress.org/ticket/58719#comment:48, I think for other reasons.
Hm. We knew, something like this would happen at some point. But it still feels earlier than expected. :wink:
So, let's start evaluating: Backdrop core on PHP 8.4 works. What I do get on every page:
Deprecated function: session_set_save_handler(): Providing individual callbacks instead of an object implementing SessionHandlerInterface is deprecated in backdrop_session_initialize() (line 248 of ...core/includes/session.inc).
But none of the other nagging, actually.
There is nagging about Constant E_STRICT in php logs, but that's it.
I installed PHP 8.4.1 on Debian Bookworm via Sury repo, and I'm testing with the latest Backdrop dev. Logging and errors fully turned on (admin/config/development/logging).
Out of curiosity I ran one functional test (I can't run them all at once on my dev box). "EntityAPITestCase" runs with 20 passes, 0 fails, 1 exception, and 1 debug message. The exception is caused by the session_set_save_handler().
So, it might be that Debian (Sury) compiled PHP differently than DDev, or maybe it depends on the version? According to php.net version 8.4.1 is the latest.
Before starting to put actual work in some PR, it might make sense to first evaluate our options.
Looking at the rfc I'm seriously considering to drop PHP 7.0 and below, and go with Nullable Types syntax.
But doesn't that deprecation mean that we only have to take care of params with type declaration like array $foobar = NULL but don't have to change anything if we stick with $foobar = NULL ( :point_left: no type hint)?
If my current assumption is wrong (which might be the case), and we really have to crawl through all of our code to fix function params, I'm leaning towards dropping support for PHP 7.0 and below. I'm aware, this would be a big step. And for sure, that's no topic for Backdrop 1.30.
If we only have to do some smaller tweaks for now (like with session_set_save_handler) it might eventually be a topic for 1.30.
But doesn't that deprecation mean that we only have to take care of params with type declaration like array
$foobar = NULLbut don't have to change anything if we stick with$foobar = NULL( 👈 no type hint)?
That's correct. If there's no type hint, there's no problem. The scope of this change is limited to where type hints are in use, which is probably in ~100 places but it's not literally everywhere. We have = NULL all over the place for strings and integers, but fortunately we have no type hints on those because they weren't PHP 5.x compatible. The only place we see this is where we're passing objects or arrays as parameters that accept a NULL default.
Deprecated function: session_set_save_handler(): Providing individual callbacks instead of an object implementing SessionHandlerInterface is deprecated in backdrop_session_initialize() (line 248 of ...core/includes/session.inc).
I hadn't realized this change. But honestly I think this is a good thing. session.inc using individual functions instead of a session object has been sort of ridiculous. It's been an available option for a very long time. We should check with the contrib Redis module to see if we can switch it to objects. I don't think the memcache module for Backdrop (which I maintain) ever had session ability.
If there's no type hint, there's no problem. The scope of this change is limited to where type hints are in use, which is probably in ~100 places
Probably less... Hard to know, as all the tools (phpcs, php-cs-fixer...) don't seem to be ready yet, to search for PHP 8.4 deprecations.
Playing with old-school grep: grep -r '(.*[a-zA-Z]\+ $[a-z_]\+ = NULL' - that finds 31 lines.
Looks sort of feasible for Backdrop 1.31.
We might be able to prevent raising minimal PHP version by dropping type hints, right?
Because how could we fix something like Layout $layout = NULL without removing the "Layout" hint here?
We might be able to prevent raising minimal PHP version by dropping type hints, right?
Except that dropping type hints affects parent/child definitions in classes, and so any child classes in contrib would start throwing errors in PHP 7.2+ by removing type hints. Though I am not clear on whether ?Layout $layout is compatible with Layout $layout = NULL defined by child classes. I hope PHP granted us this grace.
Except that dropping type hints affects parent/child definitions in classes...
Ouch... right. :thinking: Food for thought after 1.30 is out.
I didn't see this mentioned above: the other problem with dropping the type hints is that it makes them optional, rather than implicitly nullable. And they would have to be the last parameter, which might not be the case.
For context, CiviCRM is dropping support for PHP 7.4 https://civicrm.org/blog/eileen/php-74-going-end-life-php-84-ramping.
I did a little digging re our deprecated usage of session_set_save_handler() (in core/includes/session.inc).
We'll have to implement SessionHandlerInterface, but we might benefit from return type declarations, which have been added in PHP 7.0.
Otherwise we end up with 6 more #[\ReturnTypeWillChange]. :wink: (We currently have 20 of those in core.)
To clarify the difference (class BackdropSessionHandler implements SessionHandlerInterface):
Also works in PHP 5.6:
#[\ReturnTypeWillChange]
public function open($savePath, $sessionName) {
return _backdrop_session_open();
}
Requires at least PHP 7.0:
public function open($savePath, $sessionName): bool {
return _backdrop_session_open();
}
^^ given, that the return types really are the same (didn't verify that...).
To me this seems like yet another argument for raising the PHP version requirement. If we don't do it now for PHP 8.4 support, we will definitely have to do it when supporting PHP 9.
And I suspect, we might not be able to avoid it with 8.4, as otherwise we run into trouble with some of our functions, for which dropping the type hint isn't an option (optional param would be followed by a required one then...).
An example for that:
function layout_condition_edit_title(Layout $layout = NULL, Block $block = NULL, LayoutMenuItem $menu_item = NULL, $handler_id) {
^^ That situation seems rare, though...
The situation, that dropping a type hint requires to also handle that change in classes extending core classes seems to be quite as rare.
I found EntityControllerInterface::resetCache(), though. And SelectQueryInterface::preExecute(). :thinking:
Though I am not clear on whether ?Layout $layout is compatible with Layout $layout = NULL defined by child classes. I hope PHP granted us this grace.
I tested it and I think we've been granted this grace.
I fixed resetCache(?array $ids) for EntityControllerInterface but left TaxonomyTermController::resetCache(array $ids = NULL). Then I cleared the cache. The depreciation warning appears but no errors.
strtok() is deprecated. I had just noticed it is used in a few places. https://wiki.php.net/rfc/deprecations_php_8_4. Though oddly, it's not mentioned here https://www.php.net/strtok.
Update: Turns out that page was just the proposals and strtok deprecation did not pass.
We should check with the contrib Redis module to see if we can switch it to objects.
The Redis module doesn't have capability for storing sessions. https://github.com/backdrop-contrib/redis/commit/ac11b29dc935a2f0dd65c3c15024274bbf7f16ba and https://www.drupal.org/node/2752735
I tested it and I think we've been granted this grace.
That's good to know.
To summarize: we have to decide to either introduce breaking changes for at least two pretty central methods in core (EntityControllerInterface::resetCache and SelectQueryInterface::preExecute) or we raise PHP minimal requirements to 7.1 to make use of nullable type declarations.
Implicitly nullable parameter types are deprecated as of PHP 8.4. The successor, nullable types have been introduced in 7.1.
We have to rewrite core/includes/session.inc to implement SessionHandlerInterface - which would also be possible in a bc way and could provide support even for PHP 5.6. Something I noticed along the way when experimenting: the "internal" function _backdrop_session_write() is in use in two contrib projects (backup_migrate and d2b_migrate). We have to consider that when rewriting.
IF we decide to raise the min version to 7.1 (hence drop support for PHP 5.6 and 7.0), we'd additionally get the ability to get rid of the \ReturnTypeWillChange in core by using return type declarations, so we'd be prepared for PHP 9 (where return type declarations will probably be enforced). Note that a release date for PHP 9 isn't published yet.
Your link about nullable types in 7.1 is helpful:
Parameter Types
The nullable type cannot be removed in a sub-class; it can be added if not present in a super-class. This behavior is consistent with the Liskov substitution principle.
I'm guessing then that my test worked only because the subclass had implicit nullable. If the subclass removed = NULL then it would produce a fatal error.
It seems like we're leaning towards increasing to minimum PHP 7.1 rather than introduce breaking changes. I believe the PMC will make the final decision based on our recommendation.
This will need a PMC decision and announcement (we already have an internal placeholder issue for it in the private PMC issue queue - pending final decision/plan/recommendations here).
PS: Would be great if we had a way to tag issues with the need for PMC decision. Should I create one such label?
We have to rewrite core/includes/session.inc to implement SessionHandlerInterface - which would also be possible in a bc way and could provide support even for PHP 5.6.
I updated the PR with a class implementation of session handling. For now it simply calls the old functions and leaves everything else in place.
I also am just now realizing that these two syntaxes do not mean the same thing:
function test1(array $optional_array = NULL) {
function test2(?array $optional_array) {
The first function can be called with no parameter at all. But the second function requires NULL be passed in.
test1(NULL); // Works.
test1(array()); // Works.
test1(); // Works.
test2(NULL); // Works.
test2(array()); // Works.
test2(); // Does not work.
So just replacing all of our function/method signatures to include NULLable parameters also does not provide 100% compatibility for existing functions.
EDIT:
NULLable parameters can still have a default set to NULL. So if we do this:
function test3(?array $optional_array = NULL) {
That is still 100% compatible with newer PHP versions.
test3(NULL); // Works.
test3(array()); // Works.
test3(); // Works.
PR at https://github.com/backdrop/backdrop/pull/4930 is ready for review.
With @indigoxela's suggestion, it seems as though we only need to bump to PHP 7.1 (not 7.2), so I set up the PR to work at that minimal increase.
I'm feeling pretty good that this set of changes is safe for sites running 7.1+, though sites that are on PHP 5.6 or 7.0 will be guaranteed not to run, as they will encounter syntax errors: Backdrop won't even be able to return an error page.
Awesome, tests passing!
I'm not sure if the currently used doc block notation's actually correct, though.
And the sandbox throws a deprecated notice on the home page even before logging in:
Deprecated: Optional parameter $entity declared before required parameter $context is implicitly treated as a required parameter in /var/lib/tugboat/core/includes/actions.inc on line 85
^^ sandboxes still run on 8.3, so that problem has nothing to do with 8.4.
More testing soon. We still have time... 😉
This issue is now unblocked because the PMC officially voted and approved dropping PHP 5.6 and 7.1 support. See https://backdropcms.org/news/backdrop-cms-drops-support-for-php-versions-prior-to-71
Re mixing union types and nullable type notation... I belief, I found something (sort of) related here
Union types and the ?T nullable type notation cannot be mixed. Writing ?T1|T2, T1|?T2 or ?(T1|T2) is not supported and T1|T2|null needs to be used instead.
It's not about doc blocks, though, but function signatures (which we can't use, yet).
I've updated the PR to follow the coding standards discussed and addressed all review feedback. All tests are passing and this is ready for review again: https://github.com/backdrop/backdrop/pull/4930
I've updated the PR to follow the coding standards discussed and addressed all review feedback
That's great news. Really close! It almost works, besides the PHP Deprecated: actions_execute() thing, which you forgot to commit.
And I added a handful of questions.
It might be that our phpcs version in our GHA's a bit old. We set the version to 3.7 (fixed), but eventually it might make sense to not set it at all (use that latest). Simply drop the :3.7 in file .github/workflows/phpcs-lint.yml (locally I'm using v3.11.3 without any problems).
I think the PR at https://github.com/backdrop/backdrop/pull/4930 is now ready for review again. Sorry for the delay.