Opened 5 years ago
Last modified 2 years ago
#50159 new defect (bug)
Simplepie 1.5.5 - code review and modifications - fix SimplePie cache bug
Reported by: | arena | Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | normal | Version: | 5.5 |
Component: | Feeds | Keywords: | has-patch |
Focuses: | Cc: |
Description
- 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"
Attachments (6)
Change History (37)
#3
follow-up:
↓ 5
@
5 years ago
- Type changed from defect (bug) to enhancement
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.
This ticket was mentioned in PR #268 on WordPress/wordpress-develop by arena94.
5 years ago
#4
#5
in reply to:
↑ 3
@
5 years ago
The same filter was previously used twice.
Now it is used only once and value is passed as a parameter using SimplePie facility
#6
follow-up:
↓ 9
@
5 years ago
- Type changed from enhancement to defect (bug)
I passed it back to bug because since a while feed cache was not working
#7
@
5 years ago
- Summary changed from Simplepie 1.5.5 - code review and modifications to Simplepie 1.5.5 - code review and modifications - fixing cache bug
#8
follow-up:
↓ 10
@
5 years ago
- Summary changed from Simplepie 1.5.5 - code review and modifications - fixing cache bug to Simplepie 1.5.5 - code review and modifications
In one of the removed filters the 2nd parameter is $filename. In your patch it is $url.
#9
in reply to:
↑ 6
;
follow-up:
↓ 13
@
5 years ago
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.
#10
in reply to:
↑ 8
;
follow-up:
↓ 11
@
5 years ago
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.
#11
in reply to:
↑ 10
@
5 years ago
Replying to arena:
Look at line 776.
776 $lifetime = apply_filters( 'wp_feed_cache_transient_lifetime', 12 * HOUR_IN_SECONDS, $url );
#12
follow-up:
↓ 14
@
5 years ago
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
#13
in reply to:
↑ 9
;
follow-up:
↓ 15
@
5 years ago
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 );
#14
in reply to:
↑ 12
;
follow-ups:
↓ 16
↓ 17
@
5 years ago
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
#15
in reply to:
↑ 13
@
5 years ago
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.
#16
in reply to:
↑ 14
@
5 years ago
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.
#17
in reply to:
↑ 14
@
5 years ago
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 ?
#19
in reply to:
↑ 18
@
5 years ago
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.
#21
follow-up:
↓ 23
@
5 years ago
- Summary changed from Simplepie 1.5.5 - code review and modifications to Simplepie 1.5.5 - code review and modifications - fix SimplePie cache bug
@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 !
#22
in reply to:
↑ description
@
5 years ago
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.
#23
in reply to:
↑ 21
;
follow-up:
↓ 24
@
5 years ago
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)!
- if you find a problem with my code on the Beta Tester plugin, open an issue on GitHub.
- 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.
#24
in reply to:
↑ 23
@
5 years ago
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
#25
@
5 years ago
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
#27
follow-up:
↓ 28
@
5 years ago
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
#28
in reply to:
↑ 27
@
5 years ago
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??
#29
@
5 years ago
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
#31
@
2 years ago
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.
Simplepie 1.5.5 - code review and modifications