Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved aliasing and plugin replacement #38

Merged
merged 19 commits into from
Jul 19, 2021
Merged

Conversation

bennothommo
Copy link
Member

@bennothommo bennothommo commented Jun 14, 2021

Combines the following PRs for better testing:

@mjauvin
Copy link
Member

mjauvin commented Jun 28, 2021

I've tested the following:

  • instanceof works for classes of replaced plugins.
    e.g. This works if Winter\Blog is installed and replaces RainLab\Blog:
   $model = new \Winter\Blog\Models\Post;
   $model instanceof RainLab\Blog\Models\Post; // returns true
  • public $implement = []; // works for classes of a replaced plugin.
    e.g. This works if Winter\Translate is installed and replaces RainLab\Translate:
use Model;

class myClass extends Model
{
    public $implement = ['@RainLab.Translate.Behaviors.TranslatableModel'];
}

@LukeTowers
Copy link
Member

Were we able to do any performance testing on this @mjauvin or @jaxwilko? This is such a low level feature that I want to make sure it's rock solid

@bennothommo
Copy link
Member Author

@mjauvin one extra thing to test is what happens if both RainLab.Blog and Winter.Blog are installed - do the replacements work correctly, or is still a requirement to remove the original plugins first? In my testing, it seemed to be fine with both plugins installed, but just wanted to see if someone else can confirm the same.

@mjauvin
Copy link
Member

mjauvin commented Jun 29, 2021

@bennothommo I tested with both Winter/RainLab Blog plugin installed and it's working fine. @jaxwilko's code still detects and warns that both plugins are installed, but the replacement seems to not care for the replaced plugin.

@jaxwilko
Copy link
Member

@mjauvin @bennothommo Last I left it, the desired functionality is that Winter.Blog would replace Rainlab.Blog by disabling Rainlab.Blog and showing a warning suggesting it's removal. This way the Rainlab.Blog plugin will stay in place but wont be registered and no code within it will be executed.

@mjauvin
Copy link
Member

mjauvin commented Jun 29, 2021

So, yeah, this code is what makes this work now in Support/ClassLoader::load():

        // If the class is already aliased, skip loading.
        if (in_array($class, $this->loadedAliases) || in_array($class, $this->reversedClasses)) {
            return true;
        }

Copy link
Contributor

@RomainMazB RomainMazB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little comment.

src/Support/Testing/MocksClassLoader.php Show resolved Hide resolved
src/Support/Testing/MocksClassLoader.php Show resolved Hide resolved
@LukeTowers LukeTowers merged commit dcfe855 into develop Jul 19, 2021
@LukeTowers LukeTowers deleted the wip/improved-aliasing branch July 19, 2021 23:23
LukeTowers added a commit that referenced this pull request Aug 20, 2021
* develop:
  Cleanup user impersonation inline docs and ensure the impersonation is working on the current request as well as future requests.
  Fix bug where user impersonation would sometimes fail
  Code quality
  Reorganize helper functions and add new ones.
  Improvements to URL generation (#45)
  Add support for trusted proxies (#42)
  Add issue notice to README
  Add shields to README
  Tweak GitHub Actions
  Retrieve and store headers if Http::toFile() method is used. (#44)
  Improved aliasing and plugin replacement (#38)
  Reduce the calls to PathResolver::resolve for speed (#34)
  Add pagination for queries using havings (#39)
  Add missing import to FilesystemAdapter (#33)
  Improved the unique validation rule test cases (#29)
  Remove old build files
  Support multiple where clauses in Unique validation rules (#28)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants