WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 6 years ago

#2831 closed enhancement (wontfix)

Gzip support in class-snoopy.php

Reported by: Phantagom Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.0.4
Component: General Keywords: RSS gzip snoopy
Focuses: 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)

class-snoopy.php.patch (2.7 KB) - added by Phantagom 8 years ago.
patch file to add gzip support in class-snoopy.php

Download all attachments as: .zip

Change History (10)

Phantagom8 years ago

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

comment:1 Phantagom8 years ago

  • Version changed from 1.2 to 2.0.4

comment:2 Phantagom8 years ago

  • Severity changed from normal to enhancement

comment:3 Phantagom8 years ago

  • Keywords has-patch added

comment:4 masquerade8 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.

comment:5 random8 years ago

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

comment:7 random8 years ago

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

comment:8 Nazgul7 years ago

  • Keywords has-patch removed
  • Milestone set to 2.4 (future)

comment:9 pishmishy6 years ago

  • 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/

Note: See TracTickets for help on using tickets.