[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 arena):

 Replying to [comment:23 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
 > {{{
 > $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
 > >
 > > {{{#!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.

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

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