Jump to content

User:Salvatore Ingala/Review 1

From mediawiki.org

(in progress)

Use OOP, Luke!

[edit]

Static stuff like getGadgetPrefsDescription() is simply creepy, why not $gadget->getPrefsDescription()? Further points dduced for duplication: the same function uses Gadget::loadStructuredList(), then iterates over the array, then does its job. Let's see the callers... bah, ApiGetGadgetPrefs does the same!

Yes Done in r90127. Salvatore Ingala 18:06, 15 June 2011 (UTC)

Gadgets_body.php

[edit]
  • This thingie had grown to the point where it could make sense to split it into several files.
  • Profiling: add wfProfileIn()/wfProfileOut() wherever appropriate.
    • Yes Done in r90525. Added to the UserLoadOptions hook, it's probably the only relevant place in new code. Salvatore Ingala 14:13, 21 June 2011 (UTC)
  • SQL injection via gadget name in getUserPrefs()/setUserPrefs()
    • Why duplicate stuff from class User here at all?
    • Yes Done in r90215: now using UserLoadOptions and UserSaveOptions hooks instead of direct DB access. Salvatore Ingala 17:07, 16 June 2011 (UTC)
  • Verify that all existing gadgets will survive wrapping into yet another closure.
  • Once getGadgetPrefsDescription() has been turned into a member function, cache its result.
  • serialize() is much faster than JSON for prefs storage.

ApiGetGadgetPrefs.php

[edit]

Why not just merge it into ApiQueryGadgets?

jquery.formBuilder.js

[edit]

Other

[edit]