WordPress.org

Make WordPress Core

Changes between Initial Version and Version 1 of Ticket #50159, comment 24


Ignore:
Timestamp:
05/14/2020 03:04:50 PM (16 months ago)
Author:
ocean90
Comment:

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #50159, comment 24

    initial v1  
    11Replying to [comment:23 afragen]:
    2 > Replying to [comment:21 arena]:
    3 > > @afragen
    4 > >
    5 > > i am sorry but your code as shown in
    6 > > https://github.com/afragen/wordpress-beta-tester/blob/develop/src/WPBT/WPBT_Beta_RC.php#L520-L522
    7 > > do not match with simplepie cache current code
    8 > >
    9 > > your code (note that you "md5" the feed url) :
    10 > >
    11 > > {{{#!php
    12 > > $transient = md5( 'https://wordpress.org/news/category/development/feed/' );
    13 > > delete_transient( "feed_{$transient}" );
    14 > > delete_transient( "feed_mod_{$transient}" );
    15 > > }}}
    16 > >
    17 > > current wordpress code in wordpress extended simplepie cache class in wp-includes/class-wp-feed-cache-transient.php is (filename being the url feed)
    18 > >
    19 > > {{{#!php
    20 > > $this->name     = 'feed_' . $filename;
    21 > > $this->mod_name = 'feed_mod_' . $filename;
    22 > > }}}
    23 > >
    24 > > so your code is not working  (current simplepie cache code either)!
    25 >
    26 > 1.  if you find a problem with my code on the Beta Tester plugin, open an issue on GitHub.
    27 > 2. My code really does work correctly. I have stepped through everything in xDebug and it has been checked by another developer as well.
    28 >
    29 > Let me explain the flaw in your logic.
    30 >
    31 > 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
    32 > {{{
    33 > $transient = 'file_' . md5( md5( $filename:$extension ) ) );
    34 > }}}
    35 >
    36 > 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.
    37 >
    38 > {{{
    39 > 54                            $this->name     = 'feed_' . $filename;
    40 > 55                            $this->mod_name = 'feed_mod_' . $filename;
    41 >       54                      $this->name     = 'feed_' . md5( "$filename:$extension" );
    42 >       55                      $this->mod_name = 'feed_mod_' . md5( "$filename:$extension" );
    43 > }}}
    44 >
    45 > >
    46 > > if you want to adapt your code to the patch, here are some hints :
    47 > > * 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)
    48 > > * here is what your code should be when patch will apply
    49 > >
    50 > > {{{#!php
    51 > > $transient = md5( 'https://wordpress.org/news/category/development/feed/:spc' );
    52 > > delete_transient( "feed_{$transient}" );
    53 > > delete_transient( "feed_mod_{$transient}" );
    54 > > }}}
    55 >
    56 > 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.
    57 >
    58 > I'm still not clear on what you perceive the bug to be. Could you provide a better description.
    59 >
    60 > 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.
    612
    623{{{#!php