PluginManager doesn't find replaced plugins

[cstorus]

Winter CMS Build

1.2

PHP Version

8.1

Database engine

MySQL/MariaDB

Plugins installed

Skripteria.SiteSearch, Winter.Blog, Winter.Pages

Issue description

The Winter PluginManager->hasPlugin() method doesn't correctly identify replaced plugins. This appears to be caused by searching the replacementMap array using the normalized identifier, however the array is keyed using non-normalized identifiers.

    public function hasPlugin(PluginBase|string $plugin): bool
    {
        $normalized = $this->getNormalizedIdentifier($plugin);

        return isset($this->plugins[$normalized]) || isset($this->replacementMap[$normalized]);
    }

Steps to replicate

Create a replacement plugin with a replaced identifier
Search for the original plugin identifier using the PluginManager

Workaround

Amend the hasPlugin function in PluginManager:476 to use the non-normalized identifier when looking up in the replacementMap.

[mjauvin]

@jaxwilko looks like this is for you!

[jaxwilko]

@cstorus @mjauvin hey guys, I've had a look at it and I'm not sure I see the issue. I added this test (I'll push it later with anything else needed for this issue), and everything passed.

Basically the rational for this is:

• hasPlugin(): bool - Tells us if the plugin is on the system (when querying a replaced plugin, if the replacement is installed and the original is not then hasPlugin will return true).
• isDisabled(): bool - Tells us if the plugin is disabled (false = enabled).
• findByIdentifier(): Plugin|null - Fetches the plugin class, this will always return a replacement if queried for an original plugin if the second param is not true.

@cstorus could you explain how hasPlugin is affecting what you're trying to do?

[mjauvin]

@jaxwilko I think your test above succeeds because Winter.Original is still installed.

@cstorus is right, the test to see if the plugin is in the replacement map does so with the NORMALIZED identifier of the plugin... but the replacementMap does NOT use normalized keys.

[jaxwilko]

@mjauvin ahh I see, thanks for explaining. So basically the issue is the following:

hasPlugin() should return true as the plugin is being aliases however it doesn't.

[mjauvin]

Correct. The following patch fixes the issue:

diff --git a/modules/system/classes/PluginManager.php b/modules/system/classes/PluginManager.php
index f2ba9cdbb..73a88c651 100644
--- a/modules/system/classes/PluginManager.php
+++ b/modules/system/classes/PluginManager.php
@@ -180,7 +180,7 @@ class PluginManager
         $replaces = $pluginObj->getReplaces();
         if ($replaces) {
             foreach ($replaces as $replace) {
-                $this->replacementMap[$replace] = $classId;
+                $this->replacementMap[$this->getNormalizedIdentifier($replace)] = $classId;
             }
         }  

But might cause regressions elsewhere...

[mjauvin]

Note: the above patch also fixes the exists() method for replaced plugins

[mjauvin]

Any way to backport the fix to dev-develop @jaxwilko ?

[LukeTowers]

Probably not @mjauvin, the PluginManager enhancements were made to the 1.2 branch.

[jaxwilko]

@mjauvin from my perspective the issue is that normalizeIdentifier() should return the passed value if the identifier was not found in the replacement map, this is what the comment suggests should happen, but because we were overwriting the $code var, we were returning a modified version.

I've implemented this change and tests in #576

[mjauvin]

@LukeTowers Yes, but I submitted a proof of concept fix that could be applied to develop

[jaxwilko]

@mjauvin I just ported my test to develop and it passed without modification to the PluginManager, the issue may have only affected 1.2 due to the changes made to PluginManager during the refactoring.