I just realized that Drupal.theme is never used as a constructor to make an object but is always used as a typical object.

Drupal.theme will explicitly look for a default implementation in the prototype object, for all intent and purpose we could be using mickey instead of prototype.

And better, just remove the extra object altogether and just implement Drupal.theme.myThemeFunction. I'm not aware of a module needing to override a JS theme function and still access the default implementation.

// Before
Drupal.theme.prototype.myThemeFunction = function () {};

// After
Drupal.theme.myThemeFunction = function () {};

Comments

nod_’s picture

Title: Drupal.theme.prototype doesn't make sense » Drupal.theme is not a constructor, don't use Drupal.theme.prototype
nod_’s picture

Category: bug » task
Status: Active » Needs review
StatusFileSize
new5.25 KB

not really a bug, just something dirty.

This patch remove prototype altogether and doesn't replace it with default either. The code is as straightforward as it's supposed to be.

nod_’s picture

Issue summary: View changes

syntax

lomo’s picture

Status: Needs review » Reviewed & tested by the community

Everything seems to work as expected after applying the patch and there are no references to theme.prototype anymore, anywhere in core, now, so I would say this can be considered RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Awesome, thanks for the functional review! I think this could still use code review though.

webchick’s picture

Status: Needs review » Fixed

Ok, nod_ sat down and spent the better part of a half hour on teaching me JS so I could understand this patch. ;) Basically we don't need Drupal.theme.prototype, because Drupal.theme is already an object. Or something like that. ;)

Committed and pushed to 8.x. Thanks!

nod_’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

nod_’s picture

Status: Closed (fixed) » Fixed

As WimLeers pointed out, this needs a change notice: http://drupal.org/node/1816980. Marking back as fixed in case someone missed it.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

correct example