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

CommentFileSizeAuthor
#9 3238452-9.patch18.78 KBalexpott
#9 2-9-interdiff.txt2.58 KBalexpott
#2 3238452-2.patch17.1 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Issue tags: +PHP 8.1
Parent issue: » #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves)
StatusFileSize
new 3238452-2.patch17.1 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review

Yep this is necessary :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

What about if they were calling the parent constructor with null

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2-9-interdiff.txt2.58 KB
new 3238452-9.patch18.78 KB

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@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

larowlan
Location 🇦🇺🏝.au GMT+10
commented

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
+++ 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

longwave
he/him
Primary language English
Location UK
commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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

mondrake’s picture

mondrake
Location 🇮🇹
commented 
+++ 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

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed fb54589 and pushed to 9.3.x. Thanks!

Issue credit added post-commit...

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +Needs followup

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

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: -Needs followup

I will update #3185269: Introduce Connection::lastInsertId and deprecate the 'return' query option and Database::RETURN_* constants to change that there.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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

mondrake
Location 🇮🇹
commented

Ah, yes, #20 is right.

Status: Fixed » Closed (fixed)

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