Page MenuHomePhorge
Differential D25324

Fix PHP 8.1 PhabricatorEditorURIEngine::newForViewer() trim(NULL) error
ClosedPublic
Actions

Authored by Sten on Jul 3 2023, 16:43.

Details

Reviewers
valerio.bozzolan
avivey
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15518: PHP 8.1 trim(NULL) error in PhabricatorEditorURIEngine::newForViewer()
Commits
rP65e57fe23dd3: Fix PHP 8.1 PhabricatorEditorURIEngine::newForViewer() trim(NULL) error
Summary

Under PHP 8.1, PhabricatorEditorURIEngine::newForViewer() is throwing a trim(NULL) error when trying to view a diff.
This is because it tries to apply string operations to a user setting which will be null by default.

Fixes T15518

Test Plan

Unit test added -

arc unit

Or just view a diff. Eg:
https://my.phorge.site/D1234

Diff Detail

Repository
rP Phorge
Branch
PhabricatorEditorURIEngineTrim (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 623
Build 623: arc lint + arc unit

Event Timeline

Sten created this revision.Jul 3 2023, 16:43
Owners added a reviewer: O1: Blessed Committers.Jul 3 2023, 16:43
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptJul 3 2023, 16:43
Sten requested review of this revision.Jul 3 2023, 16:43
Harbormaster completed remote builds in B623: Diff 1053.Jul 3 2023, 16:43
valerio.bozzolan added inline comments.Jul 5 2023, 11:26
src/infrastructure/editor/PhabricatorEditorURIEngine.php
19

Since we are not interested in knowing the length of the string. Also note that trim() always return a string, and never NULL.

19

Premising that your addition of $pattern === null is perfectly OK ✅

valerio.bozzolan accepted this revision as: valerio.bozzolan.Jul 5 2023, 11:30

Thanks :) Soft +1 to wait for other thoughts. The test plan works for me.

Personally I will hard+1 if we get rid of that strlen() since we have that possibility here, and it's not such a frequent possibility.

Sten updated this revision to Diff 1080.Jul 5 2023, 13:44

Nice - complete removal of that strlen as suggested.

Harbormaster completed remote builds in B645: Diff 1080.Jul 5 2023, 13:44
Sten marked an inline comment as done.Jul 5 2023, 13:44
avivey accepted this revision.Jul 17 2023, 07:16
This revision is now accepted and ready to land.Jul 17 2023, 07:16
Closed by commit rP65e57fe23dd3: Fix PHP 8.1 PhabricatorEditorURIEngine::newForViewer() trim(NULL) error. · Explain WhyJul 17 2023, 10:04
This revision was automatically updated to reflect the committed changes.
Sten added a commit: rP65e57fe23dd3: Fix PHP 8.1 PhabricatorEditorURIEngine::newForViewer() trim(NULL) error.

Revision Contents
Changeset List

Diff 1053

View Options

src/infrastructure/editor/PhabricatorEditorURIEngine.php

Loading...
View Options

src/infrastructure/editor/__tests__/PhabricatorEditorURIEngineTestCase.php

Loading...
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0