Details

Reviewers

Matthew

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15517: Differential PHP 8.1 failure - DifferentialChangeset getOldStatePathVector() strlen

Commits

rPe8ea7a4a3697: Fix PHP 8.1 issue in DifferentialChangeset getOldStatePathVector()

Summary

The DifferentialChangeset getOldStatePathVector() method assumes oldFile and filename are set.
This worked under PHP <= 8.0, but fails for PHP >= 8.1 with error messsage

strlen(): Passing null to parameter #1 ($string) of type string is deprecated

Fixes T15517

Test Plan

Create a diff in which a new file is added.
This file will have oldFile NULL and filename a string.
View the diff https://my.phorge.site/D1234

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Sten created this revision.Jul 3 2023, 15:36

Owners added a reviewer: O1: Blessed Committers.Jul 3 2023, 15:36

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptJul 3 2023, 15:36

Sten requested review of this revision.Jul 3 2023, 15:36

Harbormaster completed remote builds in B622: Diff 1052.Jul 3 2023, 15:36

Sten edited the summary of this revision. (Show Details)Jul 3 2023, 15:38

Sten planned changes to this revision.Jul 4 2023, 09:57

valerio.bozzolan added inline comments.Jul 4 2023, 09:58

src/applications/differential/storage/DifferentialChangeset.php
332	I think this only works in JavaScript or Lua, but not in PHP

PHP != Perl

Harbormaster completed remote builds in B629: Diff 1061.Jul 4 2023, 10:35

Yeah - spotted & fixed!

valerio.bozzolan added inline comments.Jul 4 2023, 10:44

src/applications/differential/storage/DifferentialChangeset.php
328	Probably, just a NULL check is not enough. Also checking whenever it's an empty string or not could be useful to have backward compatibility.
328–330	Apparently `$path` can be an empty string, and also in that case (length zero) `$this->getFilename()` should be assumed.

Update as per peer review

Harbormaster completed remote builds in B631: Diff 1063.Jul 4 2023, 12:50

Sten marked an inline comment as done.Jul 4 2023, 12:52

Interestingly, it seems to me that, before, the result always has been an array since it passes through explode().

Interestingly, explode('/', '') returns array('').

So maybe we should always elevate $path to a string to at least always return array('').

But this is weird so I think we need more Phorge experts :D let's wait.

src/applications/differential/storage/DifferentialChangeset.php
327	AAAH ONE EXTRA NEWLINE `ヾ(｡ꏿ﹏ꏿ)ﾉﾞ`

Interesting point about the array return.

Looking deeper into this, getOldStatePathVector() is only called in two places.

src/infrastructure/diff/view/PHUIDiffTableOfContentsListView.php calls getOldStatePathVector, and makes loads of null checks on the results, so expects a null return.
src/applications/differential/query/DifferentialDiffInlineCommentQuery.php calls getOldStatePathVector and passes the results to last(), which will error if not passed an array. However, it does some previous checks suggesting that it only calls getOldStatePathVector if getOldStatePathVector is actually going to return an array.

So I've updated the code to make it explicit that we will return either a null or an array.

Harbormaster completed remote builds in B649: Diff 1086.Jul 5 2023, 16:10

Could we have a lint rule to catch forbidden newlines?

In D25323#9739, @Sten wrote:

Could we have a lint rule to catch forbidden newlines?

Yep but then we cannot reply anymore with AAAH ONE EXTRA NEWLINE ヾ(｡ꏿ﹏ꏿ)ﾉﾞ and that is 90% of my job here :(

Probably this could be approved more quickly by somebody else, suggesting a Test plan that is able to trigger both cases (NULL and not null)

Can't find a test case in which the function returns null :-(

Uhm. What about removing a file?

In D25323#9875, @valerio.bozzolan wrote:

Uhm. What about removing a file?

In this case oldFile exists, so we have a string value for it.

Gentle reminder that this has been waiting review for a while now...

LGTM

This revision is now accepted and ready to land.Aug 11 2023, 16:27

Closed by commit rPe8ea7a4a3697: Fix PHP 8.1 issue in DifferentialChangeset getOldStatePathVector(). · Explain WhyAug 12 2023, 07:39

This revision was automatically updated to reflect the committed changes.

Sten added a commit: rPe8ea7a4a3697: Fix PHP 8.1 issue in DifferentialChangeset getOldStatePathVector().

Fix PHP 8.1 issue in DifferentialChangeset getOldStatePathVector()
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1243

src/__phutil_library_map__.php

src/applications/differential/storage/DifferentialChangeset.php

src/applications/differential/storage/tests/DifferentialChangesetTestCase.php

Fix PHP 8.1 issue in DifferentialChangeset getOldStatePathVector()ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1243

src/__phutil_library_map__.php

src/applications/differential/storage/DifferentialChangeset.php

src/applications/differential/storage/__tests__/DifferentialChangesetTestCase.php

Fix PHP 8.1 issue in DifferentialChangeset getOldStatePathVector()
ClosedPublic
Actions

Revision Contents
Changeset List

src/applications/differential/storage/tests/DifferentialChangesetTestCase.php