django-cms / djangocms-moderation

Other
13 stars 21 forks source link

Overwriting the PageToolBar key with a Toolbar object featuring a non-matching module path breaks toolbar unregister function #12

Closed geoidesic closed 5 years ago

geoidesic commented 6 years ago

https://github.com/divio/djangocms-moderation/blob/9dddf2f18b9a9b3ec4dfc4d3b904fb467a1f254d/djangocms_moderation/cms_toolbars.py#L186

E.g. toolbar_pool.toolbars['cms.cms_toolbars.PageToolbar'] = ExtendedPageToolbar you will now have a false mapping which is unexpected by unregister:

def unregister(self, toolbar):
        name = '%s.%s' % (toolbar.__module__, toolbar.__name__)
        if name not in self.toolbars:
            raise ToolbarNotRegistered('The toolbar %s is not registered' % name)
        del self.toolbars[name]

... thus it becomes impossible to unregister this toolbar

How I would suggest we do this instead of directly overwriting the PageToolbar key in the toolbar_pool is using the API like so:

toolbar_pool.unregister(PageToolbar)
toolbar_pool.register(ExtendedPageToolbar)

I'm not sure whether this will have any undesirable side-effects that someone was trying to avoid but doing it this way doesn't seem to break anything obvious within moderation after some testing.

mmoravcik commented 5 years ago

Handled in release 1.0.x