Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#9 | 3238452-9.patch | 18.78 KB | alexpott |
#9 | 2-9-interdiff.txt | 2.58 KB | alexpott |
#2 | 3238452-2.patch | 17.1 KB | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottYep this is necessary :)
Comment #4
larowlanIs this a BC break for anything that extends these classes?
Comment #5
alexpott@larowlan nope - this is not about types - it's about default values which can change... see https://3v4l.org/jLt2v
Comment #6
larowlanWhat about if they were calling the parent constructor with null
Comment #7
longwaveWe 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.
This is wrong I think - !isset is the same as empty, not !empty. Also we perhaps want
if ($message === '')
asempty()
is TRUE for "0".Comment #8
alexpott@larowlan calling the parent constructor with NULL is exactly what occurs in https://3v4l.org/jLt2v
Comment #9
alexpottNice catch @longwave. Added a test because why not :)
Comment #10
alexpott@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...
NULLs have been cast to strings for a while... see https://3v4l.org/RlgWt for all version of PHP which are supported.
Comment #11
larowlanProbably helps if I read the 3v4l code.
Ignore me
Comment #12
longwaveIsn't this going to trigger the deprecation in PHP 8.1?
edit: oh no, because the message is overridden deliberately in that case.
Comment #13
longwaveIgnore #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.Comment #14
alexpottI agree with #13. FWIW, we never actually pass a custom message into RowCountException()
Comment #15
mondrakePHP is getting stricter, so why shouldn't we and go for
if ($message === '')
as suggested in #7?Comment #17
catchCommitted fb54589 and pushed to 9.3.x. Thanks!
Issue credit added post-commit...
Comment #18
catchCrossposted with #17, but that's a fair question - lets change that in a follow-up.
Comment #19
mondrakeI will update #3185269: Introduce Connection::lastInsertId and deprecate the 'return' query option and Database::RETURN_* constants to change that there.
Comment #20
alexpottAtm 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.Comment #21
mondrakeAh, yes, #20 is right.