[FIX] Has plugin fix

[jaxwilko]

This PR resolves the issue raised in #575.

Simply it ensures that normalizeIdentifier() returns the value passed if the key is not found in the normalizedMap array.

[jaxwilko]

Probably helps if I commit the new plugin that the tests are targeting

[jaxwilko]

As a note, I've added some consts for plugin count in the PluginManagerTest class, this just makes it easier to maintain and reduces the amount of magic numbers in the test

[mjauvin]

@jaxwilko are you sure this fixes the issue, though? I don't see the link with replacementMap[]

[jaxwilko]

@mjauvin the replacement map correctly stores normalized identifiers 'Winter.Original' => 'Winter.Replacement'

The normalized map also correctly stores lower keys to normalized identifiers 'lower.case' => 'Camel.Case'

The issue is when you normalize an identifier via normalizeIdentifier() it should strtolower() the input, check the normalizedMap and return any value found, returning the original input if no key was found. However the returned value was the string that had been ran through strtolower()

[mjauvin]

But this line will never return true for a replaced plugin:
https://github.com/wintercms/winter/blob/develop/modules/system/classes/PluginManager.php#L184-L189

[jaxwilko]

Expanding on the above, when you call hasPlugin() it normalizes the identifier, then checks the replacement map, for the example of the test Winter.NotInstalled is not installed and therefore does not exist in the normalizedMap so returns the lower case version winter.notinstalled, which is not in either $this->plugins or $this->replacementMap. By adding the fix to return the passed value, Winter.NotInstalled is in $this->replacementMap and therefore hasPlugin() returns true

[mjauvin]

Oh, I get it now... but you still need to use the plugin name with the exact capitalization, otherwise it won't work... this is not true for the replacement plugin, though

[jaxwilko]

But this line will never return true for a replaced plugin: https://github.com/wintercms/winter/blob/develop/modules/system/classes/PluginManager.php#L184-L189

The reason for this is we don't know the true normalized identifier for the plugin being replaced, so we just take whatever the replacement plugin suggests, if a developer accidentally provides:

Then they we will use that as the normalized identifier of the plugin being replaced as we have no way of checking for that plugins real code (as it may not be installed on the system).

It is down to the plugin developer to correctly implement the replacement, they should notice when testing that the plugin wont be flagged as replaced if they mistype.

[mjauvin]

>>> $pm->hasPlugin('Rainlab.Blog');
=> false

>>> $pm->hasPlugin('RainLab.Blog');
=> true

>>> $pm->hasPlugin('rainlab.blog');
=> false

vs

>>> $pm->hasPlugin('winter.blog');
=> true

>>> $pm->hasPlugin('Winter.blog');
=> true

>>> $pm->hasPlugin('Winter.Blog');
=> true

[mjauvin]

But this line will never return true for a replaced plugin: https://github.com/wintercms/winter/blob/develop/modules/system/classes/PluginManager.php#L184-L189

The reason for this is we don't know the true normalized identifier for the plugin being replaced, so we just take whatever the replacement plugin suggests, if a developer accidentally provides: Then they we will use that as the normalized identifier of the plugin being replaced as we have no way of checking for that plugins real code (as it may not be installed on the system).

It is down to the plugin developer to correctly implement the replacement, they should notice when testing that the plugin wont be flagged as replaced if they mistype.

But why don't we put everything as lowercase internally, and always normalize the provided identifiers for any of the methods?

[jaxwilko]

@mjauvin honestly that's what I'd prefer to do, we have to transform everything everywhere and it doesn't make much sense doing that. Would be really nice to just strtolower() everything and ignore casing.

Re: mixed casing hasPlugin we could look at something like this:

But I really don't like it

[mjauvin]

What's wrong with:

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;
             }
         }  

[jaxwilko]

I think that would work with the pervious version of normalizeIdentifier() which would return strtolower($code). However if we fix normalizeIdentifier() it wont solve the hasPlugin when calling with mixed cases.

We could revert back to the old version of normalizeIdentifier(), but I'm not a massive fan of that side effect.

@LukeTowers any thoughts?

[jaxwilko]

I've marked this as a draft because while it resolves issues reported in #575 it adds in a lot of complexity with how we handle things internally.

This PR is more of a hack than a real fix at this time.

A proper solution would be to convert the internals of PluginManager to use lower cased identifiers, simplifying the logic for resolving plugins internally.

[LukeTowers]

Closing in favour of fba6446