Paths

Table of Contentst

Differential D25231

AphrontFormControl: fix regression for some specific Captions
ClosedPublic
Actions

Authored by valerio.bozzolan on May 19 2023, 13:10.

Details

Reviewers

avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15404: PhabricatorController: fix Caption (can be a string but also PhutilSafeHTML) causing error in PhabricatorPhabricatorAuthProvider

Commits

rP976a21f6584b: AphrontFormControl: fix regression for some specific Captions

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 417
Build 417: arc lint + arc unit

Event Timeline

valerio.bozzolan created this revision.May 19 2023, 13:10

Owners added a reviewer: O1: Blessed Committers.May 19 2023, 13:10

Herald added subscribers: Cigaryno, Matthew, tobiaswiese, speck. · View Herald TranscriptMay 19 2023, 13:10

valerio.bozzolan requested review of this revision.May 19 2023, 13:10

Harbormaster completed remote builds in B416: Diff 742.May 19 2023, 13:10

valerio.bozzolan mentioned this in rP562d36ef5f9e: Fix PHP 8.1 "strlen(null)" exceptions to render the Account Creation page.May 19 2023, 13:11

valerio.bozzolan edited the summary of this revision. (Show Details)

fix native HTML stuff

Harbormaster completed remote builds in B417: Diff 743.May 19 2023, 13:14

valerio.bozzolan edited the test plan for this revision. (Show Details)May 19 2023, 13:15

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 mentioned this in Z1: Phorge.May 19 2023, 14:03

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

valerio.bozzolan mentioned this in Wikimedia Hackaton 2023 Athens Code Sprint.May 19 2023, 14:06

valerio.bozzolan edited the summary of this revision. (Show Details)May 19 2023, 15:04

valerio.bozzolan mentioned this in D25216: Fix PHP 8.1 "strlen(null)" exceptions setting up Username/Password Authentication Provider.May 19 2023, 15:27

avivey subscribed.May 19 2023, 17:22

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.

aklapper mentioned this in rP678c7a78be43: Fix PHP 8.1 "strlen(null)" exceptions setting up Username/Password….May 19 2023, 18:16

following suggestions from my friend avivey

Harbormaster completed remote builds in B420: Diff 751.May 19 2023, 18:34

valerio.bozzolan added inline comments.May 19 2023, 18:35

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.