Page MenuHomePhorge

AphrontFileResponse: avoid alien usages of setDownload()
ClosedPublic

Authored by valerio.bozzolan on Apr 7 2023, 08:16.
Tags
None
Referenced Files
F2055710: D25113.id467.diff
Wed, Mar 27, 23:55
Unknown Object (File)
Wed, Mar 27, 18:04
Unknown Object (File)
Tue, Mar 26, 17:06
Unknown Object (File)
Sun, Mar 24, 06:10
Unknown Object (File)
Thu, Mar 21, 23:27
Unknown Object (File)
Thu, Mar 21, 16:11
Unknown Object (File)
Thu, Mar 21, 08:27
Unknown Object (File)
Thu, Mar 21, 06:04

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

src/aphront/response/AphrontFileResponse.php
47

In short, this ↑ was the previous situation, and now it's somehow documented and supported again

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

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

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

valerio.bozzolan marked 2 inline comments as done.
  • improve phpdoc

Now I like it. Ready for review again.

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.

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