YahnisElsts / plugin-update-checker

A custom update checker for WordPress plugins. Useful if you don't want to host your project in the official WP repository, but would still like it to support automatic updates. Despite the name, it also works with themes.
MIT License
2.27k stars 411 forks source link

Error PUC 5.1 does not support GitHub in ... #534

Closed KevinBatdorf closed 1 year ago

KevinBatdorf commented 1 year ago

I have v5.0 installed in a plugin, and a user shared this error:

Fatal error: PUC 5.1 does not support GitHub in /plugin-update-checker-5.0/Puc/v5p0/PucFactory.php on line 132

Turns out another plugin is loading version the library and they use 5.1. I logged out in my plugin's code

// PucFactory.php
protected static function getCompatibleClassVersion($class) {
  // log self::$latestCompatibleVersion comes to 5.1
  if ( isset(self::$classVersions[$class][self::$latestCompatibleVersion]) ) {
    return self::$classVersions[$class][self::$latestCompatibleVersion];
  }
  return null;
}

Is there a step I need to take to avoid this conflict?

My implementation looks like:

// plugin-file.php
require(
    plugin_dir_path(__FILE__) . 'plugin-update-checker-5.0/plugin-update-checker.php'
);
use YahnisElsts\PluginUpdateChecker\v5\PucFactory;

add_action('plugins_loaded', function () {
  PucFactory::buildUpdateChecker(
        'https://github.com/example',
        __FILE__,
        'slug'
    )
        ->setBranch('main')
        ->setAuthentication('secret-key');
});

The other plugin is loading it like:

<?php

namespace YahnisElsts\PluginUpdateChecker\v5p1;

use YahnisElsts\PluginUpdateChecker\v5\PucFactory as MajorFactory;
use YahnisElsts\PluginUpdateChecker\v5p1\PucFactory as MinorFactory;

require __DIR__ . '/update/v5p1/Autoloader.php';
new Autoloader();

require __DIR__ . '/update/v5p1/PucFactory.php';
require __DIR__ . '/update/v5/PucFactory.php';

//Register classes defined in this version with the factory.
foreach (
    array(
        'Plugin\\UpdateChecker' => Plugin\UpdateChecker::class,
        'Vcs\\PluginUpdateChecker' => Vcs\PluginUpdateChecker::class,
    )
    as $pucGeneralClass => $pucVersionedClass
) {
    MajorFactory::addVersion($pucGeneralClass, $pucVersionedClass, '5.1');
    MinorFactory::addVersion($pucGeneralClass, $pucVersionedClass, '5.1');
}
YahnisElsts\PluginUpdateChecker\v5\PucFactory::buildUpdateChecker(
    'not-github-url',
    __FILE__,
    'hide-my-wp'
);

I did find this issue here, and checked that both versions do have the GitHub file there.

Related https://github.com/YahnisElsts/plugin-update-checker/issues/271

YahnisElsts commented 1 year ago

Normally, version 5.1 would be backwards-compatible with 5.0, but it looks like the other plugin is using a modified loader that does not register the class that's required to support GitHub. So you effectively get an incomplete 5.1 version of PUC.

The simplest way to work around that would be to modify your plugin to explicitly use the 5.0 version instead of the latest loaded 5.x version. For example:

$updateChecker = new \YahnisElsts\PluginUpdateChecker\v5p0\Vcs\PluginUpdateChecker(
    new \YahnisElsts\PluginUpdateChecker\v5p0\Vcs\GitHubApi(
        'https://github.com/user/repo'
    ),
    __FILE__,
    'slug'
);
$updateChecker
    ->setBranch('main')
    ->setAuthentication('secret-key');
KevinBatdorf commented 1 year ago

Thanks. It might be better that i do that regardless so that it doesn't cause issues with any other plugins that load it differently later on. I'll also attempt to contact the other plugin author too and share this issue.

ddur commented 1 year ago

@KevinBatdorf @YahnisElsts

Or move your PUC initialization to earlier hook, or higher priority on same hook/action than other plugin?

ddur commented 1 year ago

@KevinBatdorf @YahnisElsts

Or move your PUC initialization to earlier hook, or higher priority on same hook/action than other plugin?

I mean, construct PUC earlier than other plugin, so other plugin will be forced to use your version of PUC 5.1 :sunglasses:

Depends on how and when other plugin loads and constructs PUC. Earliest time would be at your plugin load/construction (before plugin_loaded and plugins_loaded).

If other plugin does the same, then it depends of plugins load sequence. AFAIK, that sequence of plugins loading is sorted by 'plugin-slug' (string).

KevinBatdorf commented 1 year ago

@ddur that doesn't sound very resilient because of the caveats you mention. Someone could load it in init or in no hook at all.

ddur commented 1 year ago

@ddur that because of the caveats you mention. Someone could load it in init or in no hook at all.

You will at least confirm problem cause and have quick fix using latest PUC version until you get better solution.

I guess, other plugin author just removed PUC parts he doesn't need. He probably won't notice anything.

If there is a problem for him, he will contact you. :sunglasses:

BTW, his changing shared library is not best coding practice.

BTW, Using obsolete PUC version doesn't sound very resilient too.

Good luck. :vulcan_salute:

KevinBatdorf commented 1 year ago

I'm not just solving it for my personal use. It's a plugin I distribute to users, so the strategy must account for unlimited plugin combinations. And someone somewhere will always have an outdated PUC until they update. This is just how WordPress works.

I wonder if PUC could/should help prevent custom loading like the above to prevent this scenario for happening? Maybe throw an error or something.

YahnisElsts commented 1 year ago

I have a few ideas about that, but I'm not sure if/when I'll get around to actually implementing them.

For example, since PUC now uses namespaces, I could switch it from registering individual classes to just registering a whole vXpY namespace and then trying to auto-load classes on demand. That would have helped in this case since the required class was still present on disk, it just wasn't registered with the factory.

Another potential improvement would be to give the factory the ability to "back off" if it can't find the necessary class in the latest available version and to try a lower minor version instead (e.g. a version 5 factory could use either 5.0 or 5.1 instead of always going for 5.1 when 5.1 is registered). It could be a bit complicated since it's probably a bad idea to mix classes from different versions (even if it works in theory), so the factory would have to find all dependencies before instantiating anything.

ddur commented 1 year ago

@KevinBatdorf

I'm too using PUC to distribute plugin to users. Warp iMagick.

Now I'm worried about how long using PUC will be practical for me.

@YahnisElsts

Because of changing PUC version to 5.0 will fix this problem, as far as I understand, problem here is not about versions clashing, but about modifications of original version ?

If someone modifies PUC version 5.0 this issue will reappear again ?

Maybe something like library files checksum (or signature) may prevent linking to/using modified versions ?

YahnisElsts commented 1 year ago

Because of changing PUC version to 5.0 will fix this problem, as far as I understand, problem here is not about versions clashing, but about modifications of original version ?

More or less. The plugin that's loading 5.1 is using a modified loader that doesn't register everything that's included in version 5.1.

If someone modifies PUC version 5.0 this issue will reappear again ?

As long as you load your own, unmodified copy of 5.0, this specific kind of modification probably wouldn't be a problem. Even if the other plugin doesn't register some 5.0 classes, the copy in your plugin should register the missing classes.

Maybe something like library files checksum (or signature) may prevent linking to/using modified versions ?

It's open source. If someone really wanted to modify it, they could just remove the checksum code.

This seems like an example of a more general, common problem. WordPress is a shared execution environment, so one plugin can always mess things up for other plugins. I don't think there is a reliable solution. You can make these problems less likely by structuring your code in certain ways, but you can never completely prevent them.

ddur commented 1 year ago

As long as you load your own, unmodified copy of 5.0, this specific kind of modification probably wouldn't be a problem. Even if the other plugin doesn't register some 5.0 classes, the copy in your plugin should register the missing classes.

That means that version 5.1 is different, will not prevent this issue, and you recommend implementing PUC version 5.0?

I'm just about upgrading from PUC 4.11, and that information is now very important to me.

Thank You.

YahnisElsts commented 1 year ago

I think there's some confusion happening here. Neither version 5.0 nor version 5.1 will "prevent this issue". In this case, the specific version number doesn't matter. I was talking about 5.0 and 5.1 because those were the versions involved in the original problem reported by @KevinBatdorf.

All I'm saying is that if you're using the version that's bundled with your plugin then you won't be affected if another plugin modifies its copy of PUC in the particular way that was discussed above.

ddur commented 1 year ago

I think there's some confusion happening here. Neither version 5.0 nor version 5.1 will "prevent this issue". In this case, the specific version number doesn't matter. I was talking about 5.0 and 5.1 because those were the versions involved in the original problem reported by @KevinBatdorf.

All I'm saying is that if you're using the version that's bundled without your plugin then you won't be affected if another plugin modifies its copy of PUC in the particular way that was discussed above.

Thank you very much.