Page MenuHomePhorge
Differential D25157

Fix regression in PHUIObjectItemView.php:662: allow PhutilURI and other stringlike
ClosedPublic
Actions

Authored by valerio.bozzolan on Apr 30 2023, 12:12.

Details

Reviewers
avivey
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15316: PHUITagView and similar: it's unclear whenever 'href' should be just a string
T15306: Fix Call to phutil_nonempty_string() expected null or a string, got: PhutilURI from PHUIObjectItemView.php:662
Commits
rPe9e2c954132c: Fix regression in PHUIObjectItemView.php:662: allow PhutilURI and other…
Summary

This fixes this specific exception that can happen with whatever PHP version in some pages:

Call to phutil_nonempty_string() expected null or a string, got: PhutilURI from PHUIObjectItemView.php:662

The regression was introduced since:

b56d86e48d0f63028cb5739c179d61ece2a24bb1

The problem is caused by the fact that we are trying to introduce very strict checks, and sometime we are too much strict.

In this specific case we use phutil_nonempty_stringlike() since it also allows objects with a __toString() method.

In order not to leave these cases to chance, we have added a log line, which can be removed in the future.

If you see this log line, report it in the respective Task. Thank you!

Closes T15306
Ref T15316

Test Plan
  1. Dashboard → Add a Panel: no crash
  2. Diffusion repo → README: no crash
  3. Herald → Create: no crash

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25157_1
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/view/phui/PHUIObjectItemView.php:936XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 337
Build 337: arc lint + arc unit

Event Timeline

valerio.bozzolan created this revision.Apr 30 2023, 12:12
Owners added a reviewer: O1: Blessed Committers.Apr 30 2023, 12:12
Herald added subscribers: Cigaryno, Matthew, tobiaswiese, speck. · View Herald TranscriptApr 30 2023, 12:12
valerio.bozzolan requested review of this revision.Apr 30 2023, 12:12
Harbormaster completed remote builds in B305: Diff 571.Apr 30 2023, 12:12
valerio.bozzolan retitled this revision from Fix regression in PHUIObjectItemView.php to Fix regression in PHUIObjectItemView.php:662.Apr 30 2023, 12:14
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan mentioned this in Z1: Phorge.
valerio.bozzolan retitled this revision from Fix regression in PHUIObjectItemView.php:662 to Fix regression in PHUIObjectItemView.php:662: allow PhutilURI and other stringlike.
aklapper subscribed.Apr 30 2023, 12:51

Sorry for introducing that. I can confirm the above change also fixes rendering of http://phorge.localhost/herald/create/

Using phutil_nonempty_string instead of phutil_nonempty_stringlike triggers Call to phutil_nonempty_string() expected null or a string, got: PhutilURI.

valerio.bozzolan added a comment.Apr 30 2023, 14:29
In D25157#4810, @aklapper wrote:

Sorry for introducing that.

No problem my friend, thanks for your patches and please don't stop. The stricter general approach is very OK.

I can confirm the above change also fixes rendering of http://phorge.localhost/herald/create/

Nice

valerio.bozzolan edited the test plan for this revision. (Show Details)Apr 30 2023, 15:11
valerio.bozzolan added a subscriber: avivey.Apr 30 2023, 21:40

This is really the minimal fix.

@avivey feel free to -1, share ideas or just +1

OH NO THE WORLD IS IN OUR HANDS

shipit

valerio.bozzolan added a comment.May 1 2023, 10:07

Hi @Matthew ... unbreak now patch here :D

Sorry if you are on vacation.

To explain this minimal fix: it seems this->href is not always a string, but it can be an object that can be converted to string ("string like"). So the previous check was too much strict.

valerio.bozzolan mentioned this in Next Up (since Week 18).May 1 2023, 20:12
valerio.bozzolan added a comment.May 2 2023, 07:15

Again in short:

  • Before: classes with a __toString() not allowed
  • After: allowed again (just like before, with strlen())
avivey added a comment.May 2 2023, 07:26
  1. please add phlogs in setter (or whatever) about expected types
  2. please remember this issue in all similar changes.
valerio.bozzolan updated this revision to Diff 618.May 2 2023, 09:09

log alien types (thanks avivey)

Harbormaster completed remote builds in B331: Diff 618.May 2 2023, 09:09
valerio.bozzolan added a comment.May 2 2023, 09:10
In D25157#5345, @avivey wrote:
  1. please add phlogs in setter (or whatever) about expected types
  2. please remember this issue in all similar changes.

Oh yes, thanks!

Hoping to be useful I've also added some minor PHPDoc and also a little hint to reach people and collect their logs somewhere (here → T15316)

valerio.bozzolan edited the summary of this revision. (Show Details)May 2 2023, 09:13
valerio.bozzolan added a task: T15316: PHUITagView and similar: it's unclear whenever 'href' should be just a string.
valerio.bozzolan updated this revision to Diff 619.May 2 2023, 09:16

add missing PHPDoc also to imageHref stuff

Harbormaster completed remote builds in B332: Diff 619.May 2 2023, 09:16
valerio.bozzolan updated this revision to Diff 620.May 2 2023, 09:21

do not log obvious types

Harbormaster completed remote builds in B333: Diff 620.May 2 2023, 09:21
valerio.bozzolan added a comment.May 2 2023, 09:22

@avivey thanks again - probably now this fix is not so bad

avivey added inline comments.May 2 2023, 13:47
src/view/phui/PHUIObjectItemView.php
98

pht(), not sprintf.

169

pht

valerio.bozzolan mentioned this in D25164: Fix PHP 8.1 "rawurlencode(null)" exception which blocks rendering a project page.May 2 2023, 15:31
valerio.bozzolan updated this revision to Diff 623.May 2 2023, 16:07

adopt the nice pht() so that the error message can be translated to make translators happy :D ihih

Also introduce a private static function to reuse code and avoid internationalization issues

Harbormaster completed remote builds in B336: Diff 623.May 2 2023, 16:07
valerio.bozzolan updated this revision to Diff 624.May 2 2023, 16:22

share stack trace, fix pht() usage

Harbormaster completed remote builds in B337: Diff 624.May 2 2023, 16:22
valerio.bozzolan added a comment.May 2 2023, 16:25

OK @avivey thanks again

This shares the full stack trace and uses pht() in a way that generates just one i18n messages

valerio.bozzolan marked an inline comment as done.May 2 2023, 21:02
valerio.bozzolan marked an inline comment as done.May 2 2023, 21:07
valerio.bozzolan updated this revision to Diff 629.May 3 2023, 07:14

keep phlog() happy to share a stack trace, without any esoteric approach

Harbormaster completed remote builds in B342: Diff 629.May 3 2023, 07:14
valerio.bozzolan added a comment.EditedMay 3 2023, 07:18

OK now it's surely better than before, I hope.

Just to clarify, this is an example stack trace that is generated in my Apache's error.log:

[Wed May 03 09:10:21.276206 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478] [2023-05-03 09:10:21] EXCEPTION: (Exception) The variable href received an unexpected type: PhutilURI. Please share this stack trace as comment in Task https://we.phorge.it/T15316 at [<phorge>/src/view/phui/PHUIObjectItemView.php:957]
[Wed May 03 09:10:21.276320 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478] arcanist(head=master, ref.master=82d1abd4edd1), phorge(head=arcpatch-D25157_2, ref.master=6e8852837004, ref.arcpatch-D25157_2=8ed06dc62c93)
[Wed May 03 09:10:21.276330 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #0 <#2> PHUIObjectItemView::assertValidHref(PhutilURI, string) called at [<phorge>/src/view/phui/PHUIObjectItemView.php:98]
[Wed May 03 09:10:21.276333 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #1 phlog(Exception) called at [<phorge>/src/view/phui/PHUIObjectItemView.php:100]
[Wed May 03 09:10:21.276336 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #2 PHUIObjectItemView::setHref(PhutilURI) called at [<phorge>/src/applications/herald/controller/HeraldNewController.php:145]
[Wed May 03 09:10:21.276340 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #3 HeraldNewController::newAdapterMenu(string) called at [<phorge>/src/applications/herald/controller/HeraldNewController.php:13]
[Wed May 03 09:10:21.276343 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #4 HeraldNewController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
[Wed May 03 09:10:21.276346 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #5 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:204]
[Wed May 03 09:10:21.276350 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #6 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]
[Wed May 03 09:10:21.276402 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478] [2023-05-03 09:10:21] EXCEPTION: (Exception) The variable href received an unexpected type: PhutilURI. Please share this stack trace as comment in Task https://we.phorge.it/T15316 at [<phorge>/src/view/phui/PHUIObjectItemView.php:957]
[Wed May 03 09:10:21.276520 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478] arcanist(head=master, ref.master=82d1abd4edd1), phorge(head=arcpatch-D25157_2, ref.master=6e8852837004, ref.arcpatch-D25157_2=8ed06dc62c93)
[Wed May 03 09:10:21.276526 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #0 <#2> PHUIObjectItemView::assertValidHref(PhutilURI, string) called at [<phorge>/src/view/phui/PHUIObjectItemView.php:98]
[Wed May 03 09:10:21.276530 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #1 phlog(Exception) called at [<phorge>/src/view/phui/PHUIObjectItemView.php:100]
[Wed May 03 09:10:21.276533 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #2 PHUIObjectItemView::setHref(PhutilURI) called at [<phorge>/src/applications/herald/controller/HeraldNewController.php:145]
[Wed May 03 09:10:21.276536 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #3 HeraldNewController::newAdapterMenu(string) called at [<phorge>/src/applications/herald/controller/HeraldNewController.php:13]
[Wed May 03 09:10:21.276539 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #4 HeraldNewController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
[Wed May 03 09:10:21.276543 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #5 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:204]
[Wed May 03 09:10:21.276546 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #6 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]
[Wed May 03 09:10:21.276598 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478] [2023-05-03 09:10:21] EXCEPTION: (Exception) The variable href received an unexpected type: PhutilURI. Please share this stack trace as comment in Task https://we.phorge.it/T15316 at [<phorge>/src/view/phui/PHUIObjectItemView.php:957]
[Wed May 03 09:10:21.276719 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478] arcanist(head=master, ref.master=82d1abd4edd1), phorge(head=arcpatch-D25157_2, ref.master=6e8852837004, ref.arcpatch-D25157_2=8ed06dc62c93)
[Wed May 03 09:10:21.276726 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #0 <#2> PHUIObjectItemView::assertValidHref(PhutilURI, string) called at [<phorge>/src/view/phui/PHUIObjectItemView.php:98]
[Wed May 03 09:10:21.276729 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #1 phlog(Exception) called at [<phorge>/src/view/phui/PHUIObjectItemView.php:100]
[Wed May 03 09:10:21.276732 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #2 PHUIObjectItemView::setHref(PhutilURI) called at [<phorge>/src/applications/herald/controller/HeraldNewController.php:145]
[Wed May 03 09:10:21.276736 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #3 HeraldNewController::newAdapterMenu(string) called at [<phorge>/src/applications/herald/controller/HeraldNewController.php:13]
[Wed May 03 09:10:21.276739 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #4 HeraldNewController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
[Wed May 03 09:10:21.276742 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #5 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:204]
[Wed May 03 09:10:21.276745 2023] [php7:notice] [pid 567829] [client 127.0.0.1:34478]   #6 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]

Available for further improvements on this "unbreak now" patch.

src/view/phui/PHUIObjectItemView.php
99

✅ Lint: Partial Catch read, and ignored.

170

✅ Lint: Partial Catch read, and ignored.

936

✅ Lint: Read, and ignored.

valerio.bozzolan updated this revision to Diff 630.May 3 2023, 07:21

fix extra indentation not catched by linter

Harbormaster completed remote builds in B343: Diff 630.May 3 2023, 07:21
valerio.bozzolan mentioned this in D25176: Fix PHP 8.1 "strlen(null)" exception which blocks creating a project with an empty Description field.May 3 2023, 09:38
valerio.bozzolan added a comment.May 4 2023, 07:33

I hope readability is still optimal after applying all the suggestions. Available for clarifications :D

src/view/phui/PHUIObjectItemView.php
692–694
NOTE: This was really the only needed fix ↑ All the rest is not strictly necessary.

This allows to receive also objects with a __toString() instead of being very string and only accepting native strings.

aklapper mentioned this in T15348: PHP 8.1 "strlen(null)" exception blocks creating personal and global Herald rules.May 4 2023, 20:24
aklapper mentioned this in D25190: Fix PHP 8.1 "strlen(null)" exception which blocks creating personal and global Herald rules.May 4 2023, 20:25
valerio.bozzolan awarded a token.May 4 2023, 21:45
valerio.bozzolan mentioned this in Z5: Phorge (flame).May 4 2023, 21:57
aklapper mentioned this in rPb0044bad621b: Fix PHP 8.1 "strlen(null)" exception which blocks creating personal and global….May 5 2023, 10:53
valerio.bozzolan added a comment.May 5 2023, 13:18

I love this community even if I think @avivey is planning to kill me :D

avivey requested changes to this revision.May 6 2023, 06:47
avivey added inline comments.
src/view/phui/PHUIObjectItemView.php
949

Rather then having throw-try-catch-log-ignore, just phlog the exception here and don't throw anything.

You'll need to change the name of the method accordingly.

This revision now requires changes to proceed.May 6 2023, 06:47
valerio.bozzolan added inline comments.May 6 2023, 08:49
src/view/phui/PHUIObjectItemView.php
949

I understand (really: my previous diff version was exactly that) but I then examined Phorge and I was not able to find 1 usage of "phlog(new Exception". So I don't think we should introduce this approach today here. I think it was more a debugging trick than something that Evan would recommend.

There are lot of "assertSomething" instead with catch.

Even if the previous diff (whith your tip) is shorter, this is probably still "more OK".

Sharing this, last word to you

avivey added inline comments.May 6 2023, 11:25
src/view/phui/PHUIObjectItemView.php
949

Yes, it's not happening anywhere, but neither is "throw-catch-log", which is obviously worse (less obvious intention).

I'm also not clear why you'd want the exception anyway - phlog already adds the complete trace of where it happened.

valerio.bozzolan added inline comments.May 6 2023, 11:27
src/view/phui/PHUIObjectItemView.php
949

OK I will rewrite this method, thanks

Premising that phlog() does not share the stack trace in the log without an exception: it only shares the log line without context. The stack trace is shared only if the user knows how to enable DarkConsole, if it was enabled, and if the user knows how to reproduce that problem to trigger it again and catch the stack trace.

avivey added inline comments.May 6 2023, 11:29
src/view/phui/PHUIObjectItemView.php
949

yes, that's fine. I wouldn't expect anyone who isn't actively working on phorge to check the server logs for problems to report.

valerio.bozzolan added inline comments.May 6 2023, 11:31
src/view/phui/PHUIObjectItemView.php
949

Sorry for clarification:

Would you love to remove the shared stack trace, and just use phlog()?

Thank you

avivey added inline comments.May 6 2023, 11:36
src/view/phui/PHUIObjectItemView.php
949

either way is fine.

valerio.bozzolan updated this revision to Diff 672.May 6 2023, 11:45

make my friend avivey more happy

Harbormaster completed remote builds in B368: Diff 672.May 6 2023, 11:45
avivey accepted this revision.May 6 2023, 12:58
This revision is now accepted and ready to land.May 6 2023, 12:58
Closed by commit rPe9e2c954132c: Fix regression in PHUIObjectItemView.php:662: allow PhutilURI and other…. · Explain WhyMay 6 2023, 13:57
This revision was automatically updated to reflect the committed changes.
valerio.bozzolan added a commit: rPe9e2c954132c: Fix regression in PHUIObjectItemView.php:662: allow PhutilURI and other….

Revision Contents
Changeset List

PathSizePackages
src/
view/
phui/
58 linesO1: Blessed Committers

Diff 624

View Options

src/view/phui/PHUIObjectItemView.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