Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: DownloadResponse cache headers #9237

Merged
merged 2 commits into from
Nov 17, 2024

Conversation

michalsn
Copy link
Member

Description
This PR fixes a bug when we cannot update the Cache-Control header for a DownloadResponse class.

By default, the cache is disabled by calling the noCache() method in the constructor of the parent class. But now we are able to override these settings simply by using the setHeader() or setCache() methods.

I don't know why previously it was decided that setCache() would throw an exception since setting the cache should be possible. In many cases, we probably don't want to do that, but still... it should be possible.

Last but not least, the header Expire-Disposition (this is the first time I have seen it) was removed since I don't think it's a valid one and has no effect.

Ref: #9234

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@michalsn michalsn added the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 25, 2024
@neznaika0
Copy link
Contributor

Good. There is a misunderstanding. DownloadResponse has the "html" format. Now the headers are being overwritten, could this ever break the system?
Do I need an additional "binary" format for Response?

  ["bodyFormat":protected]=>
  string(4) "html"
  ["filename":"CodeIgniter\HTTP\DownloadResponse":private]=>
  string(13) "unit-test.php"
  ["file":"CodeIgniter\HTTP\DownloadResponse":private]=>
  object(CodeIgniter\Files\File)#78353 (4) {
    ["size":protected]=>
    int(12919)
    ["originalMimeType":protected]=>
    NULL
    ["pathName":"SplFileInfo":private]=>
    string(86) "/home/aleksandr/www/codeigniter/development/tests/system/HTTP/DownloadResponseTest.php"
    ["fileName":"SplFileInfo":private]=>
    string(24) "DownloadResponseTest.php"
  }
  ["setMime":"CodeIgniter\HTTP\DownloadResponse":private]=>
  bool(false)
  ["binary":"CodeIgniter\HTTP\DownloadResponse":private]=>
  NULL
  ["charset":"CodeIgniter\HTTP\DownloadResponse":private]=>
  string(5) "UTF-8"

protected function formatBody($body, string $format)
{
$this->bodyFormat = ($format === 'json-unencoded' ? 'json' : $format);
$mime = "application/{$this->bodyFormat}";
$this->setContentType($mime);
// Nothing much to do for a string...
if (! is_string($body) || $format === 'json-unencoded') {
$body = service('format')->getFormatter($mime)->format($body);
}
return $body;
}

@github-actions github-actions bot added the stale Pull requests with conflicts label Nov 3, 2024
Copy link

github-actions bot commented Nov 3, 2024

👋 Hi, @michalsn!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@michalsn
Copy link
Member Author

michalsn commented Nov 3, 2024

DownloadResponse class doesn't use bodyFormat.

But if you're referring to the scenario like this:

return $this->response->download($filename, null)->setJSON(['foo' => 'bar']);

Which doesn't make much sense, then yes. The Content-Type will be changed to application/json, but this behavior has nothing to do with the changes made in this PR.

@michalsn michalsn removed the stale Pull requests with conflicts label Nov 3, 2024
@samsonasik
Copy link
Member

@michalsn rebase is needed

@michalsn
Copy link
Member Author

@samsonasik Thanks, done.

@michalsn michalsn merged commit 27ae8bb into codeigniter4:develop Nov 17, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants