r86716 MediaWiki - Code Review archive

A few random comments:

• bug 19410 covers the gadget name restriction; I'd really like to see the restriction killed before ga_name mirrors it;
• using a column for ga_legacy (whether the gadget supports ResourceLoader) seems to be an odd storage system for this info; ResourceLoader 2 is on the horizon; no idea if it'll introduce breaking changes with ResourceLoader 1, but if so, the binary nature of a bool column is going to be annoying; normalization of gadget properties might make more sense instead;
• "create [unique] index" syntax looks strange; you're not specifying index_col_name in any of them.

+	-- Gadget internal name. Length restricted by user_properties.up_property length (32) - length( 'gadget-' ) (7)
+	ga_name varchar(25) binary NOT NULL,

I'd make this longer in the assumption that bug 19408 will eventually be fixed.

+	-- If true, gadget's scripts don't support ResourceLoader
+	ga_legacy bool default FALSE,
+
+	-- Whether this gadget is enabled by default for everyone
+	ga_default bool default FALSE,
+
+	-- If true, gadget cannot be used by end-users directly, it just creates
+	-- a ResourceLoader module that can be reused by other modules
+	ga_resource_only bool default FALSE

These might be better denormalised as a gadget_options table? I see a Zero One Infinity scenario, which could easily end up going the same way as the blockip table with endless additions of new fields for new options, with ever-increasing pain in doing so.

+CREATE UNIQUE INDEX /*i*/ga_names ON /*_*/gadgets;

Is this a typo? It doesn't seem to specify any fields.

+	-- On-wiki path to resource
+	gr_path varchar(255) binary NOT NULL

Is this going to store the prefixed pagename? It shouldn't, should have the same syntax as page_title. Which means you either need a gr_namespace parameter, or you'll be forcing everything to NS_MEDIAWIKI. Any thoughts about global/interwiki gadgets?

+-- Tracks dependencies between gadgets and ResourceLoader modules
+CREATE TABLE /*_*/gadget_dependencies (
+	-- References gadgets.ga_id
+	gd_gadget int PRIMARY KEY NOT NULL,
+
+	-- Module name
+	gd_module varbinary(255) NOT NULL

Any reason not to use gadget ids for both fields? Would remove a potential source of subtle errors if you're going to allow ga_name to change. There's also a type mismatch: gd_module is VARBINARY while you've made ga_name VARCHAR.

+CREATE INDEX /*i*/gd_modules ON /*_*/gadget_dependencies;

Again, does this syntax actually work? I'm happy to be proved wrong...

+-- Tracks user permissions required by gadgets
+CREATE TABLE /*_*/gadget_rights (
+	-- References gadgets.ga_id
+	grt_gadget int PRIMARY KEY NOT NULL,
+
+	-- Permission
+	grt_right varchar(64) binary NOT NULL

Pick either "right" or "permission" and use it consistently. Is this going to be keyed to permissions or groups? If permissions (which is perfectly reasonable), this is defining the datatype for permissions in any bug 15252 fix (edit group permissions on-wiki), which might be worth some discussion. IIRC there's an extension which implements this functionality; it would be worth checking if that's the case, and what data type it uses. Is varchar(64) binary equivalent to varbinary(64)??

I'd make this longer in the assumption that bug 19408 will eventually be fixed

Alternatively, preferences could use gadget IDs instead of names for preference names. Less clear way, though.

Is this going to store the prefixed pagename? It shouldn't, should have the same syntax as page_title. Which means you either need a gr_namespace parameter, or you'll be forcing everything to NS_MEDIAWIKI. Any thoughts about global/interwiki gadgets?

ResourceLoaderWikiModule accepts only prefixed page names. I'll think about this more.

Any thoughts about global/interwiki gadgets?

My current idea is to load gadgets from local wiki, then load from the same tables on the central wiki, then combine the list. It will involve more trickery (handling of global gadget changes, localisation, etc).

Any reason not to use gadget ids for both fields? Would remove a potential source of subtle errors if you're going to allow ga_name to change. There's also a type mismatch: gd_module is VARBINARY while you've made ga_name VARCHAR.

It lists dependencies on RL module names, not gadgets. Type is chosen to match the one from core table definitions (msg_resource_links.mrl_resource etc).

Again, does this syntax actually work?

Did I say anywhere that this schema actually works? :P It's just a work in progress.