Page MenuHomePhorge

AphrontFormControl: fix regression for some specific Captions
ClosedPublic

Authored by valerio.bozzolan on May 19 2023, 13:10.
Tags
None
Referenced Files
F2229876: D25231.1718542803.diff
Sat, Jun 15, 13:00
Unknown Object (File)
Thu, Jun 13, 10:51
Unknown Object (File)
Wed, Jun 12, 15:35
Unknown Object (File)
Tue, Jun 11, 23:51
Unknown Object (File)
Tue, Jun 11, 12:35
Unknown Object (File)
Sat, Jun 8, 17:17
Unknown Object (File)
Sat, Jun 8, 06:21
Unknown Object (File)
Fri, Jun 7, 01:41

Details

Summary

Fix a specific problem when visiting some specific pages like this one:

/auth/config/edit/?provider=PhabricatorPhabricatorAuthProvider:

Regression introduced in:

562d36ef5f9e83dba5e3ca0244f9c4af9e1f7b2b

Stack trace:

[Fri May 19 14:23:35.506028 2023] [php7:notice] [pid 20439] [client 127.0.0.1:39692] [2023-05-19 14:23:35] EXCEPTION: (InvalidArgumentException) Call to phutil_nonempty_string() expected null or a string, got: PhutilSafeHTML. at [<arcanist>/src/utils/utils.php:2127]
[Fri May 19 14:23:35.506647 2023] [php7:notice] [pid 20439] [client 127.0.0.1:39692] arcanist(head=arcpatch-D25049, ref.master=c14785c3795c, ref.arcpatch-D25049=1b6412c24640), phorge(head=arcpatch-D25216_1, ref.master=2df7ea13a387, ref.arcpatch-D25216_1=02b40a9e25eb)
[Fri May 19 14:23:35.506661 2023] [php7:notice] [pid 20439] [client 127.0.0.1:39692]   #0 <#2> phutil_nonempty_string(PhutilSafeHTML) called at [<phorge>/src/view/form/control/AphrontFormControl.php:206]
[Fri May 19 14:23:35.506665 2023] [php7:notice] [pid 20439] [client 127.0.0.1:39692]   #1 <#2> phutil_tag(string, array, array) called at [<phorge>/src/view/form/PHUIFormLayoutView.php:54]
[Fri May 19 14:23:35.506667 2023] [php7:notice] [pid 20439] [client 127.0.0.1:39692]   #2 <#2> PHUIFormLayoutView::render() called at [<phorge>/src/view/form/AphrontFormView.php:160]

We keep 100% backward compatibility, reproducing the implicit cast happening automatically before PHP 8.1

It has also been simplified the non-empty check since that is possible after casting to string.

Closes T15404

Test Plan
  • Visit the page mentioned in the commit summary
  • no longer explodes
  • you still don't see empty Captions
  • you still see populated Captions
  • you still see HTML rendered for populated Captions

Diff Detail

Repository
rP Phorge
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 416
Build 416: arc lint + arc unit

Event Timeline

valerio.bozzolan added inline comments.
src/view/form/control/AphrontFormControl.php
228

Reason for that comment:

If phutil_tag receives a string, it escapes that. But we don't want to see <tt>Stuff</tt> at video.

valerio.bozzolan added inline comments.
src/view/form/control/AphrontFormControl.php
206

↑ This explodes in cases of a caption consisting in PhutilSafeHTML

I discovered that during heavy testing of other random pages outside the reasonable test plan

avivey added inline comments.
src/view/form/control/AphrontFormControl.php
218–224

this whole section might be clearer like this:

$caption = $this->getCaption();
$has_caption = $caption || (phutil_string_cast($caption) !== '');

or some other variation on $has_caption rather then content that isn't used as content.

following suggestions from my friend avivey

src/view/form/control/AphrontFormControl.php
218–224

OK I get your point

Premising that probably the suggested code will not work for objects since an object short-circuit as truly even if it returns an empty string. So it would be necessary a cast call in any case. Premising that doing a cast seems very good to me since that was what PHP was doing under the hood with strlen stuff.

Thanks for the suggestion, I like this really much more

Just as a proof of concept, now it works:

Phorge Auth fixed.png (927×1 px, 140 KB)

NOTE: These visible backticks are unrelated and fixed in D25232
This revision is now accepted and ready to land.May 19 2023, 19:49