Details

Reviewers

Cigaryno
avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15190: PHP 8.1: strlen() and other scalar-only functions do not accept NULL anymore - understand fix strategies

Commits

rP0a82ccb8a4b0: AphrontFileResponse: avoid alien usages of setDownload()

Summary

I noticed that - historically - setDownload() could also be used with
false, true, or other weird values to activate a download filename.
So, this change continues to give full compatibility to PHP 8.1 but
with extra validations.

This also adds a bit of inline documentation to put this more explicit.

For more context see the previous version:

rP5952b0a31b6a: Stop mangling filenames when downloading them

rP96ae4ba13acb: PHP 8.1: fixes for strlen() not accepting NULL anymore, part 2

Ref T15190

Test Plan

Drop a file in a comment, click, download
See that it still works

Diff Detail

Repository

rP Phorge

Branch

arcpatch-D25113

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 240
Build 240: arc lint + arc unit

Event Timeline

valerio.bozzolan created this revision.Apr 7 2023, 08:16

Owners added a reviewer: O1: Blessed Committers.Apr 7 2023, 08:16

Herald added subscribers: Cigaryno, Matthew, tobiaswiese, speck. · View Herald TranscriptApr 7 2023, 08:16

valerio.bozzolan requested review of this revision.Apr 7 2023, 08:16

Harbormaster completed remote builds in B227: Diff 461.Apr 7 2023, 08:16

valerio.bozzolan added inline comments.Apr 7 2023, 08:18

src/aphront/response/AphrontFileResponse.php
22	In short, this was too rigid here ↑ I don't know why Evan wanted to explode when receiving a boolean, but that's the current potential situation, and that's too risky, since I've found some `setDownload(true)` around.

valerio.bozzolan mentioned this in Z1: Phorge.Apr 7 2023, 08:21

valerio.bozzolan added inline comments.Apr 7 2023, 08:24

src/aphront/response/AphrontFileResponse.php
47	In short, this ↑ was the previous situation, and now it's somehow documented and supported again

harden

Harbormaster completed remote builds in B228: Diff 462.Apr 7 2023, 08:30

valerio.bozzolan added inline comments.Apr 7 2023, 08:31

src/aphront/response/AphrontFileResponse.php
116	This was probably problematic in PHP 8.1 in the default NULL case

As long as there are no issue.

restore backward compatibility

Harbormaster completed remote builds in B229: Diff 463.Apr 7 2023, 10:04

valerio.bozzolan retitled this revision from AphrontFileResponse: allow setDownload() to receive alien values (again) to AphrontFileResponse: restore setDownload() ability to handle alien values.Apr 7 2023, 10:10

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

valerio.bozzolan added inline comments.Apr 7 2023, 10:13

src/aphront/response/AphrontFileResponse.php
47	For clarifications see the original version: rP5952b0a31b6a: Stop mangling filenames when downloading them
141	↑ In short, if `getDownload()` is falsy, that is ignored. If `getDownload()` is truly, it must be a string, and should be non-empty, so let's require a string and report alien values.

avivey requested changes to this revision.Apr 7 2023, 11:04

avivey subscribed.

avivey added inline comments.

src/aphront/response/AphrontFileResponse.php
31	From the code, this doesn't "activate" the download, only provides a name.
54	Is there any case where this method is called and the result is not assumed to be a string? Maybe we can just add a check here and throw if it's not.

This revision now requires changes to proceed.Apr 7 2023, 11:04

OK I agree that we should do more than this.

It seems to me that the only usages come from these files:

$ grep -R 'AphrontFileResponse' --include="*.php" .
./src/applications/files/controller/PhabricatorFileDataController.php:    $response = new AphrontFileResponse();
./src/applications/celerity/controller/CelerityResourceController.php:    $response = id(new AphrontFileResponse())
./src/applications/calendar/controller/PhabricatorCalendarController.php:    return id(new AphrontFileResponse())
./src/applications/phame/controller/blog/PhameBlogFeedController.php:    return id(new AphrontFileResponse())

So,

Direct instantiations

About the first one, is receives the setDownload($file->getName()) so, its domain is PhabricatorFile#getName() and I'm 99% sure that the name always is a STRING, or null when undefined.

The second (CelerityResourceController) does not call setDownload() as far as I can see.

The third (PhabricatorCalendarController) is probably the nicer since it always receives something like E123.ical and that is good.

The fourth (PhameBlogFeedController) does not directly call setDownload().

Now, indirect usages.

Probable usages

Note that it's not so clear to find direct since it's not the unique method calling setDownload(). Anyway:

$ grep -R 'setDownload(' --include="*.php" .
./src/applications/files/controller/PhabricatorFileDataController.php:      $response->setDownload($file->getName());
./src/applications/files/controller/PhabricatorFileViewController.php:          ->setDownload($can_download)
./src/applications/calendar/controller/PhabricatorCalendarController.php:      ->setDownload($file_name)
./src/view/layout/PhabricatorActionView.php:  public function setDownload($download) {
./src/aphront/response/AphrontFileResponse.php:  public function setDownload($download) {

One example is:

./src/applications/files/controller/PhabricatorFileViewController.php

$curtain->addAction(
  id(new PhabricatorActionView())
    ->setUser($viewer)
    ->setDownload($can_download)
    ->setName(pht('Download File'))
    ->setIcon('fa-download')
    ->setHref($file->getDownloadURI())
    ->setDisabled(!$can_download)
    ->setWorkflow(!$can_download));

Nnote that this is a different class (PhabricatorActionView) but I'm not 100% sure that AphrontFileResponse#getDownload() is never set from the value of AphrontFileResponse#setDownload() in some ways.

Anyway this value is a boolean.

In short

Very probably, the unique usages are: string, and NULL (default case when unset). The boolean usages are not related to this method, so it's safe to harden this a bit and avoid confusing usages.

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 7 2023, 12:48

valerio.bozzolan retitled this revision from AphrontFileResponse: restore setDownload() ability to handle alien values to AphrontFileResponse: avoid alien usages of setDownload().

follow suggestions from kind avivey to be more bold and harden this

Harbormaster completed remote builds in B232: Diff 466.Apr 7 2023, 12:49

valerio.bozzolan marked 2 inline comments as done.Apr 7 2023, 12:54

valerio.bozzolan added inline comments.

src/aphront/response/AphrontFileResponse.php
31	Thanks. Replaced with "Set a download filename".
39	Note that this now will raise useful warnings when setting to false or true, since both values have really no sense here. The only accepted value now are: a populated string (the value is preserved) an empty string or NULL (the default value is used)
54	Probably fixed: now the getter is as-is, but it's safe to assume that it always return NULL, or a string.
141	The method `phutil_nonempty_string()` is still useful since it literally checks whenever that string is non-empty and non-NULL, and reporting alien values. This is much more readable than before with `strlen()` and effectively fixes PHP 8.1 issues with the default value (NULL).

improve phpdoc

Harbormaster completed remote builds in B233: Diff 467.Apr 7 2023, 13:08

Now I like it. Ready for review again.

Approval required.

one lint issue that escaped, otherwise good.

src/aphront/response/AphrontFileResponse.php
40	2 spaces here. Not sure why `arc lint` doesn't catch this one.

fix spaces

Harbormaster completed remote builds in B240: Diff 475.Apr 8 2023, 17:49

valerio.bozzolan marked an inline comment as done.Apr 8 2023, 17:49

Cigaryno accepted this revision.Apr 9 2023, 17:29

avivey accepted this revision.Apr 9 2023, 18:08

This revision is now accepted and ready to land.Apr 9 2023, 18:08

Closed by commit rP0a82ccb8a4b0: AphrontFileResponse: avoid alien usages of setDownload(). · Explain WhyApr 9 2023, 20:46

This revision was automatically updated to reflect the committed changes.

valerio.bozzolan added a commit: rP0a82ccb8a4b0: AphrontFileResponse: avoid alien usages of setDownload().

AphrontFileResponse: avoid alien usages of setDownload()
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Direct instantiations

Probable usages

In short

Revision Contents
Changeset List

Diff 475

src/aphront/response/AphrontFileResponse.php

AphrontFileResponse: avoid alien usages of setDownload()ClosedPublicActions

Details

Diff Detail

Event Timeline

Direct instantiations

Probable usages

In short

Revision ContentsChangeset List

Diff 475

src/aphront/response/AphrontFileResponse.php

AphrontFileResponse: avoid alien usages of setDownload()
ClosedPublic
Actions

Revision Contents
Changeset List