Page MenuHomePhorge

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

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

Details

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 requested review of this revision.Apr 2 2024, 20:49
This revision is now accepted and ready to land.Apr 3 2024, 00:48
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 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 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")

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!