Fix PHP 8.1 "addcslashes(null)" exception exporting task list to tab-separated text
ClosedPublic
Actions

Authored by aklapper on Apr 2 2024, 20:49.

Details

Reviewers

20after4
speck

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15771: PHP 8.1 "addcslashes(null)" exception exporting task list to tab-separated text

Commits

rP79464882da14: Fix PHP 8.1 "addcslashes(null)" exception exporting task list to tab-separated…

Summary

When a column value to export to Tab-Separated Text is empty, null is passed to addcslashes() which is deprecated behavior since PHP 8.1.
Thus only call addclashes() when the value is set.

ERROR 8192: addcslashes(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/phorge/src/infrastructure/export/format/PhabricatorTextExportFormat.php:45]

Closes T15771

Test Plan

Export a Maniphest task list of query results to Tab-Separated Text.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.Apr 2 2024, 20:49

Owners added a reviewer: O1: Blessed Committers.Apr 2 2024, 20:49

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptApr 2 2024, 20:49

aklapper requested review of this revision.Apr 2 2024, 20:49

Harbormaster completed remote builds in B1143: Diff 1812.Apr 2 2024, 20:49

20after4 accepted this revision.Apr 3 2024, 00:48

This revision is now accepted and ready to land.Apr 3 2024, 00:48

speck subscribed.Apr 3 2024, 02:31

speck added inline comments.

src/infrastructure/export/format/PhabricatorTextExportFormat.php
45–47	I’m guessing the answer is no, but asking anyways just for assurances - is there any possibility value is not a string but an object with a toString override that addcslashes would coerce?

In this way, do we skip that column creating a column shift?

Maybe we need to promote that NULL to empty string

In D25568#16354, @valerio.bozzolan wrote:

In this way, do we skip that column creating a column shift?

Err, yeah, that would be bad - better test it at least.
Looks like it would skip the value but not the \t separator, so maybe that's ok? Depends on what's reading it I guess?

In D25568#16354, @valerio.bozzolan wrote:

In this way, do we skip that column creating a column shift?

Maybe we need to promote that NULL to empty string

In the end it should _not_ make a difference if you do not add something to $rows[] between two tabs, or if you insert a $row which is in an empty string between two tabs.

The code beforehand for a row with content foo created \tfoo\t and for a row with no content created \t\t and that has not been changed by this patch. In my testing at least.

(PS: Edited reply as I had forgotten the word "not")

aklapper added inline comments.Apr 3 2024, 08:22

src/infrastructure/export/format/PhabricatorTextExportFormat.php
45–47	I have not seen problems testing here with some custom fields enabled and exporting in both Maniphest and Differential search query results, but I cannot say for absolutely 100% sure.

Did my previous comment sufficiently elaborate that this is not a change in behavior? If the comment did not, please speak up before I may merge this. :) Thanks!

Seems good

Closed by commit rP79464882da14: Fix PHP 8.1 "addcslashes(null)" exception exporting task list to tab-separated…. · Explain WhyApr 15 2024, 14:42

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rP79464882da14: Fix PHP 8.1 "addcslashes(null)" exception exporting task list to tab-separated….