craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.22k stars 626 forks source link

Add methods or guidelines for plugin developers to avoid conflicts/loading order issues #3028

Closed khalwat closed 6 years ago

khalwat commented 6 years ago

There are a number of parts of Craft that fire events to allow plugins and other things to register things like AdminCP routes or site routes.

The problem is that if any plugin calls a service method that causes it to initialize itself, that means that no other plugins will be able to register additional things. For example, in the FallbackSite plugin does this:

    public function init()
    {
        // Determine if there is an element at the given URL, and attempt to find one using fallback sites
        // if one is not available. This may interfere with custom routes or direct templates, if one
        // happens to conflict with the potential path.

        // Only run for regular web frontend requests.
        if (!Craft::$app->getRequest()->getIsConsoleRequest()
            && Craft::$app->getRequest()->getIsSiteRequest()
            && !Craft::$app->getRequest()->getIsLivePreview()) {
            $element = Craft::$app->getUrlManager()->getMatchedElement(); // Find the element that the current URL is going to.
            if ($element == null) { // No element found, this request may 404 normally.
                $this->renderFallbackEntry();
            }
        }
        parent::init();
    }

https://github.com/charliedevelopment/craft3-fallback-site/blob/master/src/FallbackSite.php#L39

Since it's calling Craft::$app->getUrlManager() that will cause the UrlManager to init itself, which means any plugins that load after it will be unable to register site or AdminCP routes.

I don't view this as a problem with the FallbackSite plugin, but rather with how Craft is handling things.

Similarly, if any plugin uses Twig to render something in its init() method, any plugins that load after it will be unable to register a new Twig extension, because Twig has already been initialized.

There are numerous other services/parts of Craft that work like this; you can register new things until someone calls a function that uses that service/part, at which time it initialize itself, and no further things can be registered.

Proposal

I think this behavior should be documented, otherwise we're going to be seeing no end of "plugin conflicts" or have something like WordPress where plugins have a loading "priority" (which never works out) or the bad old days of OS X extension conflicts/loading order.

But I think just documenting it isn't enough. I think what makes sense is the base plugin class should implement a new method called, perhaps, install() which Craft will call after it has loaded all plugins and done any other setup work.

It then should be documented that in your init() method you shouldn't be calling any Craft services that could cause them to initialize themselves.

You should register anything you want there, such as site/AdminCP routes, Twig extensions, etc., but not call any Craft services. Maybe there should even be an additional register() method to explicitly be where you do this?

Rather all of that code that you need to actually call Craft's various services should be in the install() method (or whatever it ends up being called).

Thoughts?

khalwat commented 6 years ago

c.f.: https://github.com/nystudio107/craft-seomatic/issues/154

sbossarte commented 6 years ago

As @khalwat says, teaching new plugin developers some best practices for plugin setup might reduce errors like the above. That could be done through standardized functions that isolate each part of this process, additional documentation, or more detailed examples. I also agree that something like a priority system would only be kicking the problem further down the line. These kinds of side effects should be avoided early on.

I'll admit that I did not initially realize this was a problem, and will resolve this for Fallback Site.

To add to that, in an ideal world, I believe all of a plugin's event handlers would be registered within the init() method, unless there is a very special reason not to. It's always those exceptions that cause trouble, but having a better understanding of the parts involved will help in developing better plugins.

Finally, if there was a way to raise some kind of notice for event handlers being attached after their events were already fired, it'd make tracking down any issues as a result of this behavior much easier.

brandonkelly commented 6 years ago

As of the next release, craft\web\UrlManager::createUrl(), createAbsoluteUrl(), and getMatchedElement() will log warnings if they’re called before Craft has been fully initialized, and getMatchedElement() will have a code example in the class reference that demonstrates how it should be called. I’ve also increased the visibility of the warnings for EVENT_REGISTER_CP_ROUTES and EVENT_REGISTER_SITE_ROUTES, noting that they should be calle via class-level events.

If similar issues crop up in other methos, we can do the same for them.

davist11 commented 6 years ago

I'm still having an issue using getMatchedElement() in a plugin (even after updating to 3.0.13.1). In my plugin's init(), I wrapped the service that is calling getMatchedElement() like the docs recommended:

Craft::$app->on(Application::EVENT_INIT, function() {
    // service that calls $element = Craft::$app->urlManager->getMatchedElement();
});

But I'm still getting a 404 when I hit a front-end entry page. If I comment out the code hitting getMatchedElement(), the 404 goes away.

Let me know if there are any other details I can provide or if you'd rather have me open a new issue.

brandonkelly commented 6 years ago

@davist11 Where is the NotFoundHttpException getting thrown?

davist11 commented 6 years ago

Here's the full stack trace

yii\web\NotFoundHttpException: Template not found: applications/test-application-1 in /Users/tdavis/sites/hamilton/vendor/craftcms/cms/src/controllers/TemplatesController.php:70
Stack trace:
#0 [internal function]: craft\controllers\TemplatesController->actionRender('applications/te...', Array)
#1 /Users/tdavis/sites/hamilton/vendor/yiisoft/yii2/base/InlineAction.php(57): call_user_func_array(Array, Array)
#2 /Users/tdavis/sites/hamilton/vendor/yiisoft/yii2/base/Controller.php(157): yii\base\InlineAction->runWithParams(Array)
#3 /Users/tdavis/sites/hamilton/vendor/craftcms/cms/src/web/Controller.php(103): yii\base\Controller->runAction('render', Array)
#4 /Users/tdavis/sites/hamilton/vendor/yiisoft/yii2/base/Module.php(528): craft\web\Controller->runAction('render', Array)
#5 /Users/tdavis/sites/hamilton/vendor/craftcms/cms/src/web/Application.php(282): yii\base\Module->runAction('templates/rende...', Array)
#6 /Users/tdavis/sites/hamilton/vendor/yiisoft/yii2/web/Application.php(103): craft\web\Application->runAction('templates/rende...', Array)
#7 /Users/tdavis/sites/hamilton/vendor/craftcms/cms/src/web/Application.php(271): yii\web\Application->handleRequest(Object(craft\web\Request))
#8 /Users/tdavis/sites/hamilton/vendor/yiisoft/yii2/base/Application.php(386): craft\web\Application->handleRequest(Object(craft\web\Request))
#9 /Users/tdavis/sites/hamilton/public/index.php(46): yii\base\Application->run()
#10 {main}
brandonkelly commented 6 years ago

Can you give us access to the site? (Git repo or just your composer.lock + composer.json files, plus any custom module/plugin code, plus a database backup.) If so pleaese send to support@craftcms.com

davist11 commented 6 years ago

Email sent, thanks!

khalwat commented 2 years ago

Related: https://github.com/craftcms/cms/issues/10441