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

Use readfile() for serving protected uploads #26

Merged
merged 6 commits into from
Jun 28, 2024
Merged

Conversation

leedxw
Copy link
Contributor

@leedxw leedxw commented May 22, 2024

I suspect that one performance problem we have stems from loading binary files into memory, and then using echo() to send them to the client.

This switches off output buffering and uses readfile().

  • Version number has been bumped

@leedxw leedxw requested a review from snim2 May 22, 2024 14:02
@leedxw leedxw force-pushed the feature/readfile branch 4 times, most recently from 6722aaa to a6da13d Compare May 22, 2024 14:20
Copy link
Contributor

@snim2 snim2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I've tested this locally, and it still serves assets, so I think the change is fine. However, we need to update the Changelog before merging this.

@leedxw
Copy link
Contributor Author

leedxw commented May 30, 2024

You need to update the changelog, version string, git tagged version.

@leedxw leedxw marked this pull request as ready for review June 13, 2024 11:31
@RobjS
Copy link
Contributor

RobjS commented Jun 17, 2024

Do we have a recommended local config for testing this plugin on files in uploads/? By default, requests for files in that directory aren't routed via WordPress auth, so this plugin code doesn't run.

@leedxw
Copy link
Contributor Author

leedxw commented Jun 17, 2024

Is this for saluki testing?

ENABLE_INTRANET_SETTINGS=true

will force uploads to be delivered via index.php - it's commented out in /.env.test

@RobjS
Copy link
Contributor

RobjS commented Jun 17, 2024

No, this is for local testing using our thedxw/wpc-wordpress image

@snim2
Copy link
Contributor

snim2 commented Jun 28, 2024

We've merged other things since this was raised so the version number needs to be checked

Copy link
Contributor

@RobjS RobjS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this locally with Saluki and it's working, just needs version number updating.

readme.txt Outdated
- Where to redirect visitors who are not logged in and try to view restricted content
- Whether to automatically restrict access to uploads by default
- A max age for the cache-control header that will be served to any users who try to access restricted content when not logged in

== Development ==

https://github.com/dxw/dxw-members-only

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RobjS there are two README files in this plugin and this one is not displayed by default in GitHub. I'm inclined to delete it. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this is an artifact of when we published the plugin on the WordPress directory, but that's not the case in anymore, so happy to delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a WordPress plugin it should be using the Wordpress plugin conventions regardless of if it's published imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, for example, plugins that display changelogs expect them to be in readme.txt

leedxw and others added 6 commits June 28, 2024 15:17
I suspect that one performance problem we have stems from loading
binary files into memory, and then using `echo()` to send them to
the client.

This switches off output buffering and uses `readfile()`.
This header, if recieved by nginx, applies the following:

```
gzip off;
fastcgi_buffering off;
```

(The header is not passed to the client.)
This repository has two readme files, this is the one
that is not displayed in GitHub. The information in
the files is duplicated so we can just remove this.
@leedxw leedxw merged commit b3160e3 into master Jun 28, 2024
3 checks passed
@snim2 snim2 deleted the feature/readfile branch June 28, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants