Page MenuHomePhorge

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

Authored by valerio.bozzolan on Apr 30 2023, 12:12.
Tags
None
Referenced Files
F1415802: D25157.id676.diff
Sun, Feb 25, 07:36
F1415801: D25157.id672.diff
Sun, Feb 25, 07:36
F1415800: D25157.id630.diff
Sun, Feb 25, 07:36
F1415799: D25157.id629.diff
Sun, Feb 25, 07:36
F1415798: D25157.id624.diff
Sun, Feb 25, 07:36
F1415797: D25157.id623.diff
Sun, Feb 25, 07:36
F1415796: D25157.id620.diff
Sun, Feb 25, 07:36
F1415795: D25157.id618.diff
Sun, Feb 25, 07:36
Tokens
"Cup of Joe" token, awarded by valerio.bozzolan.

Details

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. DashboardAdd a Panel: no crash
  2. Diffusion repoREADME: no crash
  3. HeraldCreate: no crash

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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.

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

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

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.

Again in short:

  • Before: classes with a __toString() not allowed
  • After: allowed again (just like before, with strlen())
  1. please add phlogs in setter (or whatever) about expected types
  2. please remember this issue in all similar changes.

log alien types (thanks avivey)

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)

add missing PHPDoc also to imageHref stuff

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

src/view/phui/PHUIObjectItemView.php
98

pht(), not sprintf.

169

pht

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

share stack trace, fix pht() usage

OK @avivey thanks again

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

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

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.

fix extra indentation not catched by linter

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.

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
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

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.

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.

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.

src/view/phui/PHUIObjectItemView.php
949

Sorry for clarification:

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

Thank you

src/view/phui/PHUIObjectItemView.php
949

either way is fine.

make my friend avivey more happy

This revision is now accepted and ready to land.May 6 2023, 12:58