Opened 10 years ago

Last modified 6 years ago

#3318 review New Feature

There should be some guarantee at plugin loading order

Reported by: Martin Kou Owned by: Garry Yao
Priority: Normal Milestone:
Component: General Version: 3.0
Keywords: Oracle Cc: Senthil

Description

The iframedialog plugin is now having to use onLoad to load its code because the plugin system is always loading iframedialog first, running iframedialog's init() first, and even running iframedialog's onLoad() first.

So far we've been able to get iframedialog plugin to load correctly with the onLoad() hack. But if someone else's plugin needs the iframedialog plugin to be loaded before loading theirs, they'll have to find some even dirtier ways to do it. The requires array in plugin definition doesn't help here.

This situation is clearly not sustainable. We'll need to find some way of guaranteeing the loading order of plugins based on their dependencies. If that's not possible, we should at least guarantee the calling order of some of their initialization functions.

Attachments (3)

3318.patch (2.4 KB) - added by Martin Kou 10 years ago.
3318_ref.patch (5.6 KB) - added by Garry Yao 10 years ago.
Reference patch
3318_2.patch (5.9 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 10 years ago by Martin Kou

With [topological sorting http://en.wikipedia.org/wiki/Topological_sorting], it should be possible to guarantee the init() or onLoad() functions of plugins being required are called before the corresponding functions at the current plugin. i.e. if I have a plugin like this...

CKEDITOR.plugins.add( 'myPlugin', { requires : [ 'iframedialog' ], ... } );

and then the iframedialog subsequently requires the dialog plugin, then it should be guaranteed that the dialog plugin is initialized first, then iframedialog, and finally myPlugin - no matter how they're ordered in the config.plugins string.

Currently, plugins the appear in the config.plugins string are first loaded according to the order they appear; then, the first level of required plugins that didn't appear in the config.plugins list are loaded; after that, the second level of required plugins (i.e. those required by the first level) are loaded; the process repeats itself until there's no more required plugin that hasn't been loaded.

comment:2 Changed 10 years ago by Frederico Caldeira Knabben

Type: BugNew Feature

That's ok, and we could even consider implementing something that executes "init" in the dependency order. But, this is not a bug. The requires property is there to indicate that other plugins are to be loaded, not to be executed in a specific order.

Currently, to avoid execution order issues, we don't need hacks. It's possible to use the "beforeInit" and "afterInit" properties. When plugins are loaded, "beforeInit" is called for all plugins, then "init" and finally "afterInit". Plugins that are known to be used by others, like "iframedialog", should use "beforeInit" for its initialization stuff.

Changed 10 years ago by Martin Kou

Attachment: 3318.patch added

comment:3 Changed 10 years ago by Martin Kou

Keywords: Review? added

comment:4 Changed 10 years ago by Frederico Caldeira Knabben

I would avoid having adding code that we don't really need right now, and we're not talking about a couple of lines here.

Can the "iframedialog" issue be fixed by other means, as explained in my previous comment?

comment:5 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

There are also a few other problems here:

  • I'm not sure we can rely on object properties to guarantee the order. It looks like a buggy point.
  • A much simpler solution can definitely be found.

Changed 10 years ago by Garry Yao

Attachment: 3318_ref.patch added

Reference patch

comment:6 Changed 10 years ago by Garry Yao

Proposing a simplified way to guarantee plugins methods been executed in dependency order, it should have almost same result as topology sorted algorithm.

comment:7 Changed 10 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.0CKEditor 3.x

comment:8 Changed 10 years ago by Tobiasz Cudnik

Keywords: HasPatch added

comment:9 Changed 9 years ago by Garry Yao

Owner: Martin Kou deleted

comment:10 Changed 9 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.x

Milestone CKEditor 3.x deleted

comment:11 Changed 8 years ago by Garry Yao

Keywords: HasPatch removed
Owner: set to Garry Yao
Status: review_failedreview
Version: SVN (CKEditor) - OLD3.0

Changed 8 years ago by Garry Yao

Attachment: 3318_2.patch added

comment:12 Changed 6 years ago by xmo

Noting that I encountered this issue: I didn't see any note that dependencies had no impact on loading order (only on plugins being loaded at all) and thus expected them to be (I believe this to be a reasonable expectation when dependencies have to be specified).

My context was having a plugin B alter/extend a plugin A (actually extending two different plugins but that's not a big issue), plus said plugin already uses afterInit so that's no help.

(bypassed the problem by reimplementing the behavior I needed from the old "base" plugins in the new one after tracking down the disappearance of topological sorting somewhere deep in the bowels of ckeditor's resources loading)

comment:13 in reply to:  12 Changed 6 years ago by Frederico Caldeira Knabben

@xmo, for now, you may give a try to the pluginsLoaded event. Not a solution I know, but a good workaround.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy