Freemius active_plugins reorder may cause other plugins to be deactivated

[nobleclem]

Describe the bug In some cases the freemius vendor code attempts to reorder the "active_plugins" list in the options table. In cases where the list contains a missing numeric key (e.g. you have 9 active plugins but when you look at the array keys one of the numbers between 0 and 8 is missing) the code that reorders actually removes the wrong plugin from the list.

To Reproduce Prior to updating pods my active_plugins value was:

a:10:{i:0;s:27:"bu-versions/bu-versions.php";i:1;s:33:"cyclone-slider/cyclone-slider.php";i:3;s:33:"formidable-pro/formidable-pro.php";i:4;s:59:"hide-youtube-related-videos/hide-youtube-related-videos.php";i:5;s:31:"page-links-to/page-links-to.php";i:6;s:13:"pods/init.php";i:7;s:27:"redirection/redirection.php";i:8;s:49:"tw-recent-posts-widget/tw-recent-posts-widget.php";i:9;s:29:"umich-alerts/umich-alerts.php";i:10;s:29:"widget-logic/widget_logic.php";}

after update it became:

a:10:{i:0;s:13:"pods/init.php";i:1;s:27:"bu-versions/bu-versions.php";i:2;s:33:"cyclone-slider/cyclone-slider.php";i:3;s:33:"formidable-pro/formidable-pro.php";i:4;s:59:"hide-youtube-related-videos/hide-youtube-related-videos.php";i:5;s:31:"page-links-to/page-links-to.php";i:6;s:13:"pods/init.php";i:7;s:49:"tw-recent-posts-widget/tw-recent-posts-widget.php";i:8;s:29:"umich-alerts/umich-alerts.php";i:9;s:29:"widget-logic/widget_logic.php";}

As you can see it actually removed key #7 which is the 6th element in the array.

I run a multisite install and this happened on some of our sites. You can also see that pods/init.php is listed twice in the after update version.

Pods Version

2.7.17

Additional context The problem code is:

• file: vendor/freemius/wordpress-sdk/includes/fs-essential-functions.php

• function: fs_newest_sdk_plugin_first

  $newest_sdk_plugin_key = array_search( $newest_sdk_plugin_path, $active_plugins );
      if ( 0 === $newest_sdk_plugin_key ) {
          // if it's 0 it's the first plugin already, no need to continue
          return false;
      } else if ( is_numeric( $newest_sdk_plugin_key ) ) {
          // Remove plugin from its current position.
          array_splice( $active_plugins, $newest_sdk_plugin_key, 1 );
  
          // Set it to be included first.
          array_unshift( $active_plugins, $newest_sdk_plugin_path );

As you can see the array_search finds the element key in this case 6. Then it performs an array_splice which is where the problem of removing the incorrect element actually happens.

Test Code for replication of bug

$string = 'a:10:{i:0;s:27:"bu-versions/bu-versions.php";i:1;s:33:"cyclone-slider/cyclone-slider.php";i:3;s:33:"formidable-pro/formidable-pro.php";i:4;s:59:"hide-youtube-related-videos/hide-youtube-related-videos.php";i:5;s:31:"page-links-to/page-links-to.php";i:6;s:13:"pods/init.php";i:7;s:27:"redirection/redirection.php";i:8;s:49:"tw-recent-posts-widget/tw-recent-posts-widget.php";i:9;s:29:"umich-alerts/umich-alerts.php";i:10;s:29:"widget-logic/widget_logic.php";}';

$array = unserialize( $string );
print_r( $array );

$podsKey = array_search( 'pods/init.php', $array );
var_dump( $podsKey );

$ret = array_splice( $array, $podsKey, 1 );
var_dump( $ret );

array_unshift( $array, 'pods/init.php' );

print_r( $array );
print_r( serialize( $array ) );

[sc0ttkclark]

@nobleclem can you confirm that the patched code suggestion https://github.com/Freemius/wordpress-sdk/pull/397 fixes the issue?

[nobleclem]

Short answer: No

Long Answer: I think the issue is that numeric key arrays sequence order cannot be relied upon.

So in one case you have something like: (every array key exists between 0-9)

a:10:{i:0;s:27:"bu-versions/bu-versions.php";i:1;s:33:"cyclone-slider/cyclone-slider.php";i:2;s:33:"formidable-pro/formidable-pro.php";i:3;s:59:"hide-youtube-related-videos/hide-youtube-related-videos.php";i:4;s:31:"page-links-to/page-links-to.php";i:5;s:13:"pods/init.php";i:6;s:27:"redirection/redirection.php";i:7;s:49:"tw-recent-posts-widget/tw-recent-posts-widget.php";i:8;s:29:"umich-alerts/umich-alerts.php";i:9;s:29:"widget-logic/widget_logic.php";}

And in another case you have something like: (missing a key between 0 and where pods/init.php exists)

a:10:{i:0;s:27:"bu-versions/bu-versions.php";i:1;s:33:"cyclone-slider/cyclone-slider.php";i:3;s:33:"formidable-pro/formidable-pro.php";i:4;s:59:"hide-youtube-related-videos/hide-youtube-related-videos.php";i:5;s:31:"page-links-to/page-links-to.php";i:6;s:13:"pods/init.php";i:7;s:27:"redirection/redirection.php";i:8;s:49:"tw-recent-posts-widget/tw-recent-posts-widget.php";i:9;s:29:"umich-alerts/umich-alerts.php";i:10;s:29:"widget-logic/widget_logic.php";}

So array_splice is actually unreliable in this case. There are two ways that I can see resolving the problem.

1) just before array_splice add a sort( $active_plugins ); 2) replace array_splice with unset( $active_plugins[ $newest_sdk_plugin_key ] );

Now the second option to me is preferable as it doesn't mess with plugin ordering in case there are other instances where plugin order matters.

So I was just now curious if after this code reorders the active_plugins if when I activate a new plugin if the order would go back to alphabetical. And it does. So really this code as it gets executed is currently broken and non-persistent. I am not sure beyond executing it after plugins are activated/deactivate that its intention of loading the plugin with the newest freemius is actually going to work.

Also full disclosure I ended up finding the github project after I submitted this issue and created one for them just in case, however I thought it best that both projects knew of this bug. So I do have an issue with them: https://github.com/Freemius/wordpress-sdk/issues/396

[sc0ttkclark]

Aha! I didn't notice that the active_plugins list wasn't fully intact. If we apply a array_values() to the active plugins list before doing anything, that fixes everything else in the code there. I've updated https://github.com/Freemius/wordpress-sdk/pull/397 accordingly.

[sc0ttkclark]

Updating Pods to use the latest Freemius SDK should resolve some of the concerns here, but the logic decisions for Freemius ultimately rest with them. This needs to happen by me because there were some customizations we made to our committed version that I want to follow up and confirm it doesn't break anything on our end upon update.