YahnisElsts/plugin-update-checker

version_compare(): Passing null to parameter #1 ($version1) of type string is deprecated

JiveDig opened this issue · 25 comments

Using PUC 5.0 in my plugin, LocalWP, and PHP 8.1.9, I get the following error:

version_compare(): Passing null to parameter #1 ($version1) of type string is deprecated

on

/vendor/yahnis-elsts/plugin-update-checker/Puc/v5p0/UpdateChecker.php:318

which is inside the getUpdate() method.

This doesn't happen on PHP 8.0.22.

I'm not sure what other info you need, or how to fix/test anything, but let me know how I can help :) Thanks.

This error suggests that there is a stored update that has a null value saved as a version number. Usually, this shouldn't happen because PUC is supposed to perform basic validation and reject values like null or the empty string.

Try installing the Debug Bar plugin and take a look at the "PUC (your-plugin-slug)" panel. What does it show for "Cached update"? Also, what are you using to provide updates: a JSON file, a GitHub repository, or something else?

Interesting, Debug Bar shows 2 other plugins running PUC (1 via Puc_v4_Factory and the other v5), but it doesn't show the one with the error. It's not showing the plugin with the issue on either PHP 8.0.22 or 8.1.9.

This is a public GitHub repository and PucFactory::buildUpdateChecker is hooked into plugins_loaded.

external_updates-mai-analytics

O:8:"stdClass":5:{s:9:"lastCheck";i:1681397710;s:14:"checkedVersion";s:5:"0.2.0";s:6:"update";O:8:"stdClass":11:{s:4:"slug";s:13:"mai-analytics";s:7:"version";s:5:"0.2.0";s:12:"download_url";s:66:"https://api.github.com/repos/maithemewp/mai-analytics/zipball/main";s:12:"translations";a:0:{}s:2:"id";i:0;s:8:"homepage";s:23:"https://bizbudding.com/";s:6:"tested";N;s:12:"requires_php";N;s:14:"upgrade_notice";N;s:5:"icons";a:0:{}s:8:"filename";s:31:"mai-analytics/mai-analytics.php";}s:11:"updateClass";s:50:"YahnisElsts\PluginUpdateChecker\v5p0\Plugin\Update";s:15:"updateBaseClass";s:13:"Plugin\Update";}

Just to be clear, did you look at the full Debug Bar screen or just its dropdown menu? Due to some technical issues with how that plugin works, if there are multiple plugins using PUC, only some of them will appear in the dropdown. However, if you click one of those menu items, you should get a full-page screen that has more tabs on the left side, one of which should match your plugin.

That serialized update looks fine at a glance. The stored version number is not null but "0.2.0".

Could it be that the error is actually being triggered by one of the other plugins? If multiple plugins use the same version of PUC, only one copy of that version will actually be loaded, so you can have a situation where code in one plugin is calling code that's technically part of another plugin.

Oh geez, sorry. When I view the full page, I see it in the left sidebar, but the content is empty. All of the others work.

I have a lot of plugins using PUC, and after clicking on them all in Debug Bar sidebar, all of the ones using v5 are empty. The ones using v4 show correctly.

That sounds like an unusual problem. Maybe there are more PHP errors somewhere? Or one of the other plugins tries to hide its own panel and ends up hiding all of them?

In any case, I tried installing your plugin on a local site (from the main branch), and it did not trigger the error that you mentioned. Also, its Debug Bar panel showed up fine and was not empty. This was with Mai Analytics and Debug Bar only, with no other plugins. That seems to support the idea that the problem is actually caused by another plugin.

Thanks so much for checking. Can you also install one of the plugins that's running v4? This one is super simple: https://github.com/maithemewp/mai-display-taxonomy/.

When I have only those running I see the blank screen in Debug Bar.

All right, I tried that one as well, including the 1.2.1 update that you added just now. I'm not seeing any errors. When I click on the "PUC (mai-display-taxonomy)" tab in the left sidebar (not in the dropdown menu), it has the expected content. The "PUC (mai-analytics)" tab is also not empty. I've included a screenshot of one of them below.

firefox_2023-04-13_23-14-36

Okay, thanks for checking. I'll have to keep digging.

Just documenting that I updated a plugin to 5.2, then hit this on any page i attempt to visit, front & back end.

CleanShot 2023-08-19 at 16 31 40@2x

Forcing an update check, then updating translations in WP (cause I noticed they were available) somehow cleared the error.

Hmm, I'm still not sure why that's happening.

Another idea: since you looked at the external_updates-mai-analytics option earlier, could you check if there are any other options that start with external_updates- but have the version set to null? I think it would look like "version";N; in the serialized data.

Noting here my local test site has 31 plugins using various versions of PUC 😲

CleanShot 2023-08-19 at 17 56 09@2x

Most have a big array of data, but many have limited info, like this:

CleanShot 2023-08-19 at 17 55 51@2x

Of the ones that have a version listed, they are all "version";s:5:" or "version";s:6:", which makes sense from the way I version my plugins.

The full option value of one that doesn't show full data is this:

O:8:"stdClass":2:{s:9:"lastCheck";i:1686325817;s:14:"checkedVersion";s:5:"1.1.3";}

All right, so that didn't work out. Perhaps you could modify the getUpdate() method to check if $update->version is null, and, if so, dump the whole $update object to the PHP error log? Something like: error_log('NULL version found: ' . var_export($update, true));

Thanks. I'll try both of these things the next time I hit the issue. I wonder if the DB didn't show anything null because the error is gone now.

Okay, I hit it again via mai-ads-manager this morning. Here's a screenshot of the dump:

CleanShot 2023-08-23 at 10 35 04@2x

I logged $this->update inside public function getUpdate() { around line 71 of StateStore.php.

YahnisElsts\PluginUpdateChecker\v5p2\Plugin\Update {#7006 ▼
  #extraProperties: []
  +slug: "mai-ads-manager"
  +version: "0.10.0"
  +download_url: "https://api.github.com/repos/maithemewp/mai-ads-manager/zipball/master"
  +translations: []
  +id: 0
  +homepage: "https://bizbudding.com"
  +upgrade_notice: null
  +tested: null
  +requires_php: null
  +icons: []
  +filename: "mai-ads-manager/mai-ads-manager.php"
}

Some more info:

I deactivated Whoops debugger, then ran all of these via CLI, and nothing helped.

wp plugin update --all --skip-plugins
wp core check-update
wp core update

FWIW, none of my plugins had updates, it was only a few random plugins were updated.

Then I went to Dashboard > Updates and hit "Check again" which refreshes via /wp-admin/update-core.php?force-check=1, which cleared the error.

In the screenshot, the version number for mai-ads-manager is fine, but mai-analytics has a null version.

I've read through the relevant PUC code again, and I see a possibility that version could end up as null if you're using a branch and PUC fails to retrieve the main plugin file from the repository for some reason. There's no edge case check for that; the current implementation considers retrieving the file to be optional. However, when PUC can't get the version number from a tag or a release, it does actually need the file.

Could it be that your site is intermittently hitting GitHub API rate limits? That would explain how PUC has some update information but not all of it.

We could definitely be hitting rate limits. We hit it all the time since we use the updater for all of our plugins/themes, including our premium stuff that is on thousands of sites. Sidenote: That's still been a source of frustration for our users because it shows an error that looks like something is broken, instead of just saying, "rate limit reached, please try again later" or something.

Another note is that we don't always use tags/releases for all of our stuff. Should we be? It's an extra step for our internal plugins that we test/update often.

From the screenshot above, only mai-accordion uses tags/releases, the other 2 do not.

It's not required to use tags/releases. PUC is supposed to also support branches, so I would consider this a bug.

I'll need to add a sanity check to the VCS support code and reject updates that don't have a version number.

Okay, thanks. Let me know if I can help in any way.

A bit off-track, but is there a way I'm missing to limit how often update checks happen? If we can limit to every hour or every few hours instead of every 5-10min (or whatever the default is) we can probably prolong the time before we hit rate limits.

The default check period is 12 hours, but there are exceptions for specific admin pages. For example, it will check much more often if you're hitting the "Dashboard -> Updates" page. There are multiple ways to change how often PUC checks for updates.

  • You can change the default period by using the fourth argument to buildUpdateChecker(). Pass in a time period as a number of hours, e.g. 24.
  • You can entirely disable automatic checks by passing 0 as the fourth argument to buildUpdateChecker(). Then you could add your own cron job and call $updateChecker->checkForUpdates() as often as you want.
  • You can use the check_now filter to allow/block specific update checks. For example (untested):
    $updateChecker->addFilter(
    	'check_now',
    	function ($shouldCheck, $lastCheckTimestamp, $checkPeriod) {
    		if (!empty($lastCheckTimestamp) && ((time() - $lastCheckTimestamp) < 3600)) {
    			return false;
    		}
    		return $shouldCheck;
    	},
    	10,
    	3
    );
  • Finally, you could subclass YahnisElsts\PluginUpdateChecker\v5p2\Plugin\UpdateChecker, override the createScheduler() method, and return your own version of YahnisElsts\PluginUpdateChecker\v5p2\Scheduler.

Just a note that I hit this issue on 5.3 as well. I have to deactivate my debugger in order to hit "Check again" to clear the error.

The version_compare() error should not be happening with 5.3. Are you sure it's actually using 5.3 in practice? Did anything change in the error message, even something small like the line number?

Hmmm. I actually hit it while on 5.2, then updated to 5.3 and it was still there when I refreshed the page. I'll get more details if/when I hit it again. I have a bunch of plugins still on 5.2 but updating them as I get to them.