[wp-trac] [WordPress Trac] #50159: Simplepie 1.5.5 - code review and modifications - fix SimplePie cache bug

[WordPress Trac]

#50159: Simplepie 1.5.5 - code review and modifications - fix SimplePie cache bug
--------------------------+------------------------------
 Reporter:  arena         |       Owner:  (none)
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  Awaiting Review
Component:  Feeds         |     Version:  trunk
 Severity:  normal        |  Resolution:
 Keywords:  has-patch     |     Focuses:
--------------------------+------------------------------

Comment (by afragen):

 Replying to [comment:21 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) :
 >
 > {{{#!php
 > $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)
 >
 > {{{#!php
 > $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
 {{{
 md5( md5( $filename ) );
 }}}

 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
 >
 > {{{#!php
 > $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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/50159#comment:23>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform