Simplepie 1.5.5 - code review and modifications - fix SimplePie cache bug

[arena]

• deprecating files and classes
• renaming files and classes for code consistancy
• adopting extended wp class registering available since SimplePie 1.3.3 (SimplePie_Registry)

replace ticket #43357

tested with dashboard widget "WordPress Events and News"

[arena]

Simplepie 1.5.5 - code review and modifications

[arena]

#43357 was marked as a duplicate.

[arena]

@desrosj 

fyi

regards

[afragen]

There seems to be at least one filter that has been removed without replacing in its previous state.

Specifically the parameters are different. Any reason why?

I currently use this in the WordPress Beta Tester Plugin.

https://core.trac.wordpress.org/ticket/50159

[arena]

The same filter was previously used twice.
Now it is used only once and value is passed as a parameter using SimplePie facility

[arena]

I passed it back to bug because since a while feed cache was not working

[afragen]

In one of the removed filters the 2nd parameter is $filename. In your patch it is $url.

[afragen]

Replying to arena:

I passed it back to bug because since a while feed cache was not working

While adding this filter for the Beta Tester I can tell you the feed cache was working.

[arena]

Yes, filename is used because this is the name of the variable used in SimplePie native class but this is the same !

Replying to afragen:

In one of the removed filters the 2nd parameter is $filename. In your patch it is $url.

[afragen]

Replying to arena:

Look at line 776.

776	       $lifetime = apply_filters( 'wp_feed_cache_transient_lifetime', 12 * HOUR_IN_SECONDS, $url );

[afragen]

You have also changed the transient names.

54	 	                $this->name     = 'feed_' . $filename;
55	 	                $this->mod_name = 'feed_mod_' . $filename;

to

54	                $this->name     = 'feed_' . md5( "$filename:$extension" );
55	                $this->mod_name = 'feed_mod_' . md5( "$filename:$extension" );

This particular item will cause an issue for me.
​https://github.com/afragen/wordpress-beta-tester/blob/develop/src/WPBT/WPBT_Beta_RC.php#L520-L522

[arena]

If you refer to function wp_dashboard_cached_rss_widget() ).
This function is using its own cache through transient not using SimplePie cache ...

        $cache_key = 'dash_v2_' . md5( $widget_id . '_' . $locale );
        $output    = get_transient( $cache_key );

Replying to afragen:

Replying to arena:

I passed it back to bug because since a while feed cache was not working

While adding this filter for the Beta Tester I can tell you the feed cache was working.

[arena]

Yes, i have noticed that too. This is a replication of the samples provided by SimplePie.

This transient has a 12 hours lifetime by default and if deprecated will be erased by appropriate wp-cron job 'delete_expired_transients'.

I also changed some class names and added 2 new files

Replying to afragen:

You have also changed the transient names.

54	 	                $this->name     = 'feed_' . $filename;
55	 	                $this->mod_name = 'feed_mod_' . $filename;

to

54	                $this->name     = 'feed_' . md5( "$filename:$extension" );
55	                $this->mod_name = 'feed_mod_' . md5( "$filename:$extension" );

This particular item will cause an issue for me.
​https://github.com/afragen/wordpress-beta-tester/blob/develop/src/WPBT/WPBT_Beta_RC.php#L520-L522

[afragen]

Replying to arena:

If you refer to function wp_dashboard_cached_rss_widget() ).
This function is using its own cache through transient not using SimplePie cache ...

        $cache_key = 'dash_v2_' . md5( $widget_id . '_' . $locale );
        $output    = get_transient( $cache_key );

Replying to afragen:

Replying to arena:

I passed it back to bug because since a while feed cache was not working

While adding this filter for the Beta Tester I can tell you the feed cache was working.

I was not referring to this. If you have the Beta Tester Plugin active, there is a dashboard widget with data.

[afragen]

Replying to arena:

Yes, i have noticed that too. This is a replication of the samples provided by SimplePie.

This transient has a 12 hours lifetime by default and if deprecated will be erased by appropriate wp-cron job 'delete_expired_transients'.

I also changed some class names and added 2 new files

Replying to afragen:

You have also changed the transient names.

54	 	                $this->name     = 'feed_' . $filename;
55	 	                $this->mod_name = 'feed_mod_' . $filename;

to

54	                $this->name     = 'feed_' . md5( "$filename:$extension" );
55	                $this->mod_name = 'feed_mod_' . md5( "$filename:$extension" );

This particular item will cause an issue for me.
​https://github.com/afragen/wordpress-beta-tester/blob/develop/src/WPBT/WPBT_Beta_RC.php#L520-L522

I noticed the change, my point was why? There is potential incompatibility by changing and there is no need to change it.

[arena]

On

This particular item will cause an issue for me.
​https://github.com/afragen/wordpress-beta-tester/blob/develop/src/WPBT/WPBT_Beta_RC.php#L520-L522

Are you using SimplePie cache for what looks like a sensitive feed (​https://wordpress.org/news/category/development/feed/) ?

On

Why this change ?
then why all these wp classes extending simplepie if they are useless ?

[afragen]

I don't understand what you mean by a sensitive feed.

I am using wp_widget_rss_output(). This function caches the data and stores it in the SimplePie transient which is named as the first set of transient names. 'feed_' . $filename and 'feed_mod_' . $filename. The $extension isn't part of that transient name.

And yes this transient is cached correctly, expiring in 12 hours.

On

Why this change ?

then why all these wp classes extending simplepie if they are useless ?

I'm not saying this PR isn't worthwhile. I'm trying to understand the differences between the PR and the current code. I'm simply trying to point out the differences.

[arena]

@afragen

i am sorry but your code as shown in
​https://github.com/afragen/wordpress-beta-tester/blob/develop/src/WPBT/WPBT_Beta_RC.php#L520-L522
do not match with simplepie cache current code

your code (note that you "md5" the feed url) :

$transient = md5( 'https://wordpress.org/news/category/development/feed/' );
delete_transient( "feed_{$transient}" );
delete_transient( "feed_mod_{$transient}" );

current wordpress code in wordpress extended simplepie cache class in wp-includes/class-wp-feed-cache-transient.php is (filename being the url feed)

$this->name     = 'feed_' . $filename;
$this->mod_name = 'feed_mod_' . $filename;

so your code is not working (current simplepie cache code either)!

if you want to adapt your code to the patch, here are some hints :

• simplepie manage two kind of extensions : 'spc' and 'spi' for respectively 'Feed cache type' and 'Image cache type' (see wp-includes\SimplePie\Cache\Base.php)
• here is what your code should be when patch will apply

$transient = md5( 'https://wordpress.org/news/category/development/feed/:spc' );
delete_transient( "feed_{$transient}" );
delete_transient( "feed_mod_{$transient}" );

Regards.
Have a nice day.

btw (has-unit-tests removed ) : if any unit-tests within github (i am a github newbie), it looks like they are not relevant !

[SergeyBiryukov]

Replying to arena:

• deprecating files and classes
• renaming files and classes for code consistancy
• adopting extended wp class registering available since SimplePie 1.3.3 (SimplePie_Registry)

Thanks for the patch! I don't think any of the existing files should be deprecated or renamed here. Consistency is good in general, but doesn't seem like a strong enough reason. Renaming these files would make it harder to replace SimplePie with another library in the future should the need arise.

In any case, with the file renaming and actual changes in one patch, it's hard to see what's going on. For easier review, it would be helpful to only focus on code changes to the existing files in this ticket.

[arena]

new patch as required by @SergeyBiryukov

[afragen]

Replying to arena:

@afragen

i am sorry but your code as shown in
​https://github.com/afragen/wordpress-beta-tester/blob/develop/src/WPBT/WPBT_Beta_RC.php#L520-L522
do not match with simplepie cache current code

your code (note that you "md5" the feed url) :

$transient = md5( 'https://wordpress.org/news/category/development/feed/' );
delete_transient( "feed_{$transient}" );
delete_transient( "feed_mod_{$transient}" );

current wordpress code in wordpress extended simplepie cache class in wp-includes/class-wp-feed-cache-transient.php is (filename being the url feed)

$this->name     = 'feed_' . $filename;
$this->mod_name = 'feed_mod_' . $filename;

so your code is not working (current simplepie cache code either)!

1. if you find a problem with my code on the Beta Tester plugin, open an issue on GitHub.
2. My code really does work correctly. I have stepped through everything in xDebug and it has been checked by another developer as well.

Let me explain the flaw in your logic.

SimplePie is capable of using several different methods of creating the hash used in the transient. The default SimplePie cache_name_function = 'md5'. Your patch will have the effect of

$transient = 'file_' . md5( md5( $filename:$extension ) ) );

The following is from your latest patch. It is wrong and will break things. There is no reason for adding :$extension when it didn't exist in the first place.

54	 	                $this->name     = 'feed_' . $filename;
55	 	                $this->mod_name = 'feed_mod_' . $filename;
 	54	                $this->name     = 'feed_' . md5( "$filename:$extension" );
 	55	                $this->mod_name = 'feed_mod_' . md5( "$filename:$extension" );

if you want to adapt your code to the patch, here are some hints :

• simplepie manage two kind of extensions : 'spc' and 'spi' for respectively 'Feed cache type' and 'Image cache type' (see wp-includes\SimplePie\Cache\Base.php)
• here is what your code should be when patch will apply

$transient = md5( 'https://wordpress.org/news/category/development/feed/:spc' );
delete_transient( "feed_{$transient}" );
delete_transient( "feed_mod_{$transient}" );

The point is I shouldn't be required to make a change. I have followed what is in core and your patch causes a breaking change. A change that is not necessary and not relevant to the goals of your PR.

I'm still not clear on what you perceive the bug to be. Could you provide a better description.

There is no value in removing a filter that currently exists. Simply because it exists in more than one location is not justification for removal.

[arena]

Replying to afragen:

$transient = 'file_' . md5( md5( $filename:$extension ) ) );

i am sorry but you are wrong see screenshot #50159_wp_options.png which is an extract of wp_options caching with the patch.

The fact that i changed the code is because core code is wrong and is not working (that is the purpose of a patch i believe !)

Regards

[arena]

simplepie caching with transients in wp_options table (row #78799 & #78793)

[arena]

Full extract of my db is in csv attached (wp_options.csv)

from sql query

select * 
from wp_options 
where option_name like '%t_feed%' or option_name like '%v2%' 
order by option_name

[arena]

wp_options from query

[afragen]

This is what I currently have.

How is this incorrect.

What you have is simply the double md5 hash of the transient. It will look similar but it's not the same.

[arena]

ok let's get back to the patch and i explain it step by step

WP_Feed_Cache_Transient implements SimplePie_Cache_Base
implementing the interface is a requirement of simplepie

• md5 or not md5 ... i do not know how simplepie can apply a second md5 on a part of a string ... still a mystery for me. Anyway, if you want to remove this md5, just do it !

• transient lifetime is passed as a parameter of simplepie cache location (to avoid to have the same filter called twice, meaning not executing twice the same add_filter code if any

WP_Feed_Cache

see simplepie doc

feed.php

• $feed->registry->register the new way to set class in SimplePie

[afragen]

Replying to arena:

ok let's get back to the patch and i explain it step by step

WP_Feed_Cache_Transient implements SimplePie_Cache_Base
implementing the interface is a requirement of simplepie

• md5 or not md5 ... i do not know how simplepie can apply a second md5 on a part of a string ... still a mystery for me. Anyway, if you want to remove this md5, just do it !

In https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-simplepie.php#L575 the default $cache_name_function = 'md5'. As you can see in the source there is a method set_cache_name_function() that allows the user to select an alternate hashing method. The point is that it's already being hashed. You don't need to do that in your patch.

The question is why did you change $this->name from $location to $location:$extension. This will only cause problems and serves no function other than change.

• transient lifetime is passed as a parameter of simplepie cache location (to avoid to have the same filter called twice, meaning not executing twice the same add_filter code if any

There are many filters that are called more than once in different locations. There is no point in removing an instance of it being called as there is no harm in being called. It's not like removing a call to add_filter will provide a performance benefit.

WP_Feed_Cache

see simplepie doc

feed.php

• $feed->registry->register the new way to set class in SimplePie

Other than creating a new way to set a class in SimplePie, what's the benefit?

You also state that you are fixing a bug. What is the bug??

[arena]

I am really sorry. i mean it !

i took my tests from the beginning ... and developped a little plugin to test different cases (one test at a time).

and here is the result of a test using the filter ... filter code is a bit complicated !

**************** TEST 04 ***********************
 * 04 *  ** using fetch_feed with filter (lifetime = 410363262) and two feeds as array : ["http:\/\/wordpress.org\/development\/feed\/","https:\/\/www.theverge.com\/rss\/index.xml"]
 * 04 *  rows in wp_options where option_name like "_transient%feed%" : 0
 * 04 * +++++ filter(1)
 * 04 * +++++ filter(1) beg >> lifetime >> 43200
 * 04 * +++++ filter(1) beg >> 2nd arg  >> ["http:\/\/wordpress.org\/development\/feed\/","https:\/\/www.theverge.com\/rss\/index.xml"]
 * 04 * +++++ filter(1) ++ filtered : should be 1st call (2nd func arg string or array)
 * 04 * +++++ filter(1) end >> lifetime >> 410363262
 * 04 * +++++ filter(2)
 * 04 * +++++ filter(2) beg >> lifetime >> 43200
 * 04 * +++++ filter(2) beg >> 2nd arg  >> 0ff4b43bd116a9d8720d689c80e7dfd4
 * 04 * +++++ filter(2) ++ filtered : should be 2nd call and 2nd arg is one of array of many
 * 04 * +++++ filter(2) end >> lifetime >> 410363262
 * 04 * +++++ filter(3)
 * 04 * +++++ filter(3) beg >> lifetime >> 43200
 * 04 * +++++ filter(3) beg >> 2nd arg  >> 196f73e8cf4330017ab92ef17541ebea
 * 04 * +++++ filter(3) ++ filtered : should be 2nd call and 2nd arg is one of array of many
 * 04 * +++++ filter(3) end >> lifetime >> 410363262
 * 04 *  rows in wp_options where option_name like "_transient%feed%" : 8
 * 04 * 
79541	_transient_timeout_feed_0ff4b43bd116a9d8720d689c80e7dfd4	2000000001
79542	_transient_feed_0ff4b43bd116a9d8720d689c80e7dfd4	a:4:{s:5:"child";a:1:
79543	_transient_timeout_feed_mod_0ff4b43bd116a9d8720d689c80e7dfd4	2000000001
79544	_transient_feed_mod_0ff4b43bd116a9d8720d689c80e7dfd4	1589636739
79545	_transient_timeout_feed_196f73e8cf4330017ab92ef17541ebea	2000000002
79546	_transient_feed_196f73e8cf4330017ab92ef17541ebea	a:4:{s:5:"child";a:1:
79547	_transient_timeout_feed_mod_196f73e8cf4330017ab92ef17541ebea	2000000002
79548	_transient_feed_mod_196f73e8cf4330017ab92ef17541ebea	1589636740

i attach the plugin to the ticket (SimplePieCacheTester.php). It removes all feed cache from options table. Once activated visit admin page wp-admin/tools.php

[arena]

tests cannot be chained due to mysql ... cache

[jrf]

Loosely related to #36669 (updating SimplePie to latest) and #51521 (update to latest again)

[ironprogrammer]

As pointed out in comment:30, SimplePie in core was updated to v1.5.6 in [49176], outdating the original intent of this ticket. Propose closing with wontfix.

Related: As of this writing, #55604 has already been planned to update core to v1.6.0, and the library has just released ​v1.7.0.

[SergeyBiryukov]

I believe [49565] / #29204 addressed some part of these changes.