Paths

Table of Contentst

Differential D25224

Fix PHP 8.1 "trim(null)" exception which blocks rendering Reports' Burnup Rate page
ClosedPublic
Actions

Authored by aklapper on May 12 2023, 22:08.

Tags

None

Referenced Files

	F2156824: D25224.diff
	Wed, Apr 24, 00:37

	Unknown Object (File)
	Mon, Apr 22, 07:19

	Unknown Object (File)
	Sun, Apr 21, 23:26

	Unknown Object (File)
	Sun, Apr 21, 22:15

	Unknown Object (File)
	Sun, Apr 21, 20:13

	Unknown Object (File)
	Wed, Apr 17, 06:16

	Unknown Object (File)
	Thu, Apr 11, 03:03

	Unknown Object (File)
	Sun, Apr 7, 05:44

View All 79 Files

Subscribers

valerio.bozzolan

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15392: PHP 8.1 "trim(null)" exception blocks rendering Reports' Burnup Rate page

Commits

rP2ffbef882038: Fix PHP 8.1 "trim(null)" exception which blocks rendering Reports' Burnup Rate…

Summary

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

Thus first check that $row['oldValue'] is not null before trimming it.

Closes T15392

Test Plan

Applied this change; afterwards "Burnup Rate" page at /maniphest/report/burn/ got correctly rendered in web browser.

Diff Detail

Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.May 12 2023, 22:08

Owners added a reviewer: O1: Blessed Committers.May 12 2023, 22:08

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 12 2023, 22:08

aklapper requested review of this revision.May 12 2023, 22:08

Harbormaster completed remote builds in B407: Diff 731.May 12 2023, 22:08

valerio.bozzolan mentioned this in Wikimedia Hackaton 2023 Athens Code Sprint.May 18 2023, 10:16

valerio.bozzolan added inline comments.May 19 2023, 22:14

src/applications/maniphest/controller/ManiphestReportController.php
192	Thanks but this is not so obvious for me to review since previously the `trim(null, '"')` was just returning an empty string and now it will return null. I will double-check.

Thanks

I tested this and it totally works. Also, since if this does not use any phutil_() stuff this will not cause any unexpected nuclear implosion.

Hoping to be useful I will just minimize this a bit even more

sgtm

This revision is now accepted and ready to land.May 21 2023, 08:15

minimize this even more, just because we can

Harbormaster completed remote builds in B432: Diff 774.May 21 2023, 08:16

valerio.bozzolan added inline comments.May 21 2023, 08:29

src/applications/maniphest/controller/ManiphestReportController.php
202–203	Unrealated: for performance reasons we may like in the future to also add a strict check here to avoid the unuseful call to `ManiphestTaskStatus::isOpenStatus(null)` that returns false after comparing that in a loop with each possible known status

valerio.bozzolan added inline comments.May 21 2023, 08:34

src/applications/maniphest/controller/ManiphestReportController.php
202–203	@aklapper If you want to relax proposing another easy-peasy patch, feel free to take inspiration from this small comment ↑ Easy +1

Closed by commit rP2ffbef882038: Fix PHP 8.1 "trim(null)" exception which blocks rendering Reports' Burnup Rate…. · Explain WhyMay 23 2023, 09:58

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rP2ffbef882038: Fix PHP 8.1 "trim(null)" exception which blocks rendering Reports' Burnup Rate….

Revision Contents
Changeset List

Path

Size

Packages

src/

applications/

maniphest/

controller/

ManiphestReportController.php

5 lines

O1: Blessed Committers

Diff 789

src/applications/maniphest/controller/ManiphestReportController.php

Loading...