Page MenuHomePhorge
Differential D25210

Fix PHP 8.1 "ltrim(null)" exception which blocks adding additional user email address
ClosedPublic
Actions

Authored by aklapper on Tue, May 9, 21:04.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jun 4, 04:06
Unknown Object (File)
Sat, Jun 3, 09:08
Unknown Object (File)
Fri, Jun 2, 20:50
Unknown Object (File)
Fri, Jun 2, 20:31
Unknown Object (File)
Fri, Jun 2, 11:58
Unknown Object (File)
Wed, May 31, 06:41
Unknown Object (File)
Mon, May 29, 18:32
Subscribers
Cigaryno
Matthew
speck
tobiaswiese
valerio.bozzolan

Details

Reviewers
valerio.bozzolan
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15376: PHP 8.1 "ltrim(null)" exception blocks adding additional user email address
Commits
rP94d45de2f7dd: Fix PHP 8.1 "ltrim(null)" exception which blocks adding additional user email…
Summary

Since PHP 8.1, passing a null string to ltrim(string $string) is deprecated.

Thus we make sure that $request->getStr('email') does not return null as default.

Closes T15376

Test Plan

Applied this change, afterwards repeated the steps to add a new email address on /settings/panel/email/. This time, it's possible to close the "Verification Email Sent" and the page /settings/panel/email/ renders and lists the new email address.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

aklapper created this revision.Tue, May 9, 21:04
Owners added a reviewer: O1: Blessed Committers.Tue, May 9, 21:04
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptTue, May 9, 21:04
aklapper requested review of this revision.Tue, May 9, 21:04
Harbormaster completed remote builds in B387: Diff 704.Tue, May 9, 21:04
valerio.bozzolan accepted this revision as: valerio.bozzolan.Wed, May 10, 05:49

Thanks

Please see my inline comment, probably just adding 3 characters fixes, and that would be probably the approach from O1

I accept as behalf of myself in the meanwhile, thanks

src/applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php
175

Probably the exact minimal fix is just that.

Kind of "don't care" logic.

valerio.bozzolan mentioned this in Wikimedia Hackaton 2023 Athens Code Sprint.Thu, May 18, 10:16
valerio.bozzolan added a comment.Fri, May 19, 23:03

Yep I confirm my opinion here, let's ping-pong this tomorrow

valerio.bozzolan requested changes to this revision.Fri, May 19, 23:03
This revision now requires changes to proceed.Fri, May 19, 23:03
valerio.bozzolan resigned from this revision.Fri, May 19, 23:11

(it works indeed, so no need from me to red block)

aklapper updated this revision to Diff 756.Sat, May 20, 09:53

Use minimal fix instead

Harbormaster completed remote builds in B421: Diff 756.Sat, May 20, 09:53
valerio.bozzolan updated this revision to Diff 757.Sat, May 20, 11:58
  • small indentation fix
Harbormaster completed remote builds in B422: Diff 757.Sat, May 20, 11:58
valerio.bozzolan accepted this revision.Sat, May 20, 12:02
valerio.bozzolan edited the summary of this revision. (Show Details)

Thanks again :)

I tested this locally following the test plan without any nuclear implosion. Green light from me

lgtm

This revision is now accepted and ready to land.Sat, May 20, 12:03
Closed by commit rP94d45de2f7dd: Fix PHP 8.1 "ltrim(null)" exception which blocks adding additional user email… (authored by valerio.bozzolan, committed by aklapper). · Explain WhySat, May 20, 12:27
This revision was automatically updated to reflect the committed changes.
aklapper added a commit: rP94d45de2f7dd: Fix PHP 8.1 "ltrim(null)" exception which blocks adding additional user email….

Revision Contents
Changeset List

PathSizePackages
src/
applications/
settings/
panel/
2 linesO1: Blessed Committers

Diff 759

View Options

src/applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php

Loading...