Opened 7 years ago
Closed 5 years ago
#2831 closed enhancement (wontfix)
Gzip support in class-snoopy.php
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Priority: | normal | Milestone: | |
| Component: | General | Version: | 2.0.4 |
| Severity: | normal | Keywords: | RSS gzip snoopy |
| Cc: |
Description
class-snoopy.php laks gzip support, sometimes you need gzip support if you want to fetch a gzipped rss feed over http. So I made a patch thats ads gzip support to class-snoopy.php.
Attachments (1)
Change History (10)
comment:4
masquerade — 7 years ago
-1 for the patch in its current state. Can we not rely on sketchy functionality, like undefined constants defaulting to that value as a string? Also, I don't like the error triggered here, because we shouldn't trigger errors on something that isn't user customizable without altering a core file, unless it is a truly necessary feature.
masquerade, I don't understand the comment about undefined constants. The only one I see in the patch is E_USER_NOTICE, which is PHP core.
Disagree about triggering the error, since reporting of E_NOTICE level errors is disabled by default (and not intended for production environments anyway). IIRC WordPress explicitly disables E_NOTICE, so the user will have already had to modify wp-settings.php in order to turn it on.
+1 technical merits, -1 disapproval that it looks like the modifications have been taken without credit (comments and all) from Kellan Elliot-McCrea's MagpieRSS copy. Not good form.
comment:6
masquerade — 7 years ago
if ( function_exists(gzinflate) ) {
Undefined constant.
As for triggering errors, at this point, I believe it better that we should be using WP's built in error handling, which is there for this reason. I really don't understand why an error is being triggered at all, especially since changing the option to use gzip requires modifying a core file, which is a Bad Thing. If anything, we should silently fail and disable gzip, disable the checks to see if its enabled, and those with gzip will silently use it, those without it will silently not use it, no harm done either way.
Ah yes, didn't see that. Sorry.
The built in error handling isn't suited to reporting notices. The whole point of a notice-level error is that execution can continue despite it, so a WP-style error object can't be returned. You're totally right about silently using/not using it.
What about something like this?
if ( function_exists('gzinflate') ) {
$headers .= "Accept-encoding: gzip\r\n";
$this->gzip_enabled = true;
}
// later ...
if ( $is_gzipped ) {
if ( !$this->gzip_enabled ) {
// Handle the error properly, or just...
return false;
}
// unzip
}
The latter condition should never be true, but IMO it's important to check for it, since it actually happened to me a couple of months ago. (A site was sending everything gzipped even if the accept header wasn't sent.)
- Milestone 2.5 deleted
- Resolution set to wontfix
- Status changed from new to closed
Ticket has seen no real activity over a year so closing as wontfix. I'm not sure why the has-patch tag was been removed - if this patch is still valid I apologise, please reopen this ticket.
I suggest that any changes to class-snoopy should go through http://sourceforge.net/projects/snoopy/

patch file to add gzip support in class-snoopy.php