Problem/Motivation

In PHP 8.1 the exception message must default to an empty string and not a NULL to prevent deprecation errors in PHP 8.1.

Steps to reproduce

https://3v4l.org/DSEnM and https://dispatcher.drupalci.org/job/drupal_patches/98392/

Proposed resolution

Change the default value to an empty string.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review

Yep this is necessary :)

larowlan’s picture

Is this a BC break for anything that extends these classes?

alexpott’s picture

@larowlan nope - this is not about types - it's about default values which can change... see https://3v4l.org/jLt2v

larowlan’s picture

What about if they were calling the parent constructor with null

longwave’s picture

We already made similar changes in #3209619: [Symfony 6] Passing null as $message to Symfony exception constructors is deprecated, pass an empty string instead, we agreed there that this is a very minor BC break but there is not a lot we can do.

+++ b/core/lib/Drupal/Core/Database/RowCountException.php
@@ -7,8 +7,8 @@
-    if (!isset($message)) {
...
+    if (!empty($message)) {

This is wrong I think - !isset is the same as empty, not !empty. Also we perhaps want if ($message === '') as empty() is TRUE for "0".

alexpott’s picture

@larowlan calling the parent constructor with NULL is exactly what occurs in https://3v4l.org/jLt2v

alexpott’s picture

Nice catch @longwave. Added a test because why not :)

alexpott’s picture

@longwave wasn't the BC break part of #3209619: [Symfony 6] Passing null as $message to Symfony exception constructors is deprecated, pass an empty string instead the getReason() changes. There's nothing like that here and the behaviour of getMessage() is not changed...

Interactive shell

php > $e = new RuntimeException(NULL);
php > var_dump($e->getMessage());
string(0) ""
php >

NULLs have been cast to strings for a while... see https://3v4l.org/RlgWt for all version of PHP which are supported.

larowlan’s picture

Probably helps if I read the 3v4l code.
Ignore me

longwave’s picture

+++ b/core/tests/Drupal/Tests/Core/Database/RowCountExceptionTest.php
@@ -0,0 +1,50 @@
+    $e = new RowCountException(NULL);

Isn't this going to trigger the deprecation in PHP 8.1?

edit: oh no, because the message is overridden deliberately in that case.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Ignore #12.

I think this is all OK and ready to go in.

There is the edge case that new RowCountException('0') will now trigger the replacement message due to the empty check, but not sure that really matters.

alexpott’s picture

I agree with #13. FWIW, we never actually pass a custom message into RowCountException()

mondrake’s picture

+++ b/core/lib/Drupal/Core/Database/RowCountException.php
@@ -7,8 +7,8 @@
+    if (empty($message)) {

PHP is getting stricter, so why shouldn't we and go for if ($message === '') as suggested in #7?

  • catch committed fb54589 on 9.3.x
    Issue #3238452 by alexpott: Exception messages must default to an empty...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed fb54589 and pushed to 9.3.x. Thanks!

Issue credit added post-commit...

catch’s picture

Issue tags: +Needs followup

Crossposted with #17, but that's a fair question - lets change that in a follow-up.

mondrake’s picture

alexpott’s picture

Atm the if (empty($message)) { is needed for BC - so that anything that extends this exception and has NULL as the default message value and calls the parent. I think we'll end up making this stricter when PHP adds the inevitable string type hint to the $message param in PHP 9. Not sure we need a follow-up for this right now as that will cause us plenty of work.

mondrake’s picture

Ah, yes, #20 is right.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.