Page MenuHomePhabricator

Make it possible to instantiate all Widgets/Layouts/Elements without positional arguments, using just config options array/object, in both PHP and JS
Closed, ResolvedPublic

Description

We should make it possible to instantiate all Widgets/Layouts/Elements without positional arguments, using just config options array/object, in both PHP and JS. The fact that, say, FieldLayout takes a fieldWidget parameter and a config object with all other parameters inside was a terrible design mistake that we should (backwards-compatibly) fix before it is too late.

@cscott has already done some of this in https://gerrit.wikimedia.org/r/#/c/190368/ as part of his work on T74716: Enhancement of OOUI PHP widgets with JS (which would be a lot more unpleasant to implement if we had to pass positional parameters). We should go all the way with it in both PHP and JS.

Event Timeline

matmarex raised the priority of this task from to Needs Triage.
matmarex updated the task description. (Show Details)
matmarex added a project: OOUI.
matmarex added subscribers: matmarex, cscott.

Yes, sounds good to me!

Another slight inconsistency regards how child content is added to widgets. For most widgets, the PHP appendContent / JS $element.append() / 'content' config option is treated as an 'internal' method -- that is, the widget itself manages its content, and the enduser is not expected to directly add or mutate the contents of the $element / use appendContent.

But in other places (FormLayout for instance), the widget is entirely useless if you don't use $element.append / appendContent. And in the PHP demo app, a raw instance of Widget is created, and content is added to it.

It would probably be best if directly manipulating content were deprecated, and new initializers/methods added where previously the end user was expected to manipulate content. For example, FormLayout should probably take an items parameter. For the usage in the PHP demo app, a special ContentWidget subclass of Widget might be appropriate -- really what's wanted in the demo app is a Widget, but the FieldLayout insists that fieldWidget is a Widget, not a Layout. So maybe a LayoutWidget is needed to bridge the gap?

I think FormLayout is the only case where that happens, and indeed it should not happen anywhere. The only reason it doesn't take 'items' (containing FieldsetLayouts) is because I haven't done that yet.

@matmarex: grepping for serializeContent in my patch shows that FormLayout, PanelLayout, and DropdownInputWidget all require serializeContent. Plus the bare Widget in the demo.

Let's split that to T89750. I like having many small bugs, makes me look efficient when I resolve them all. ;)

(removed as obsolete)

gerritbot subscribed.

Change 191214 had a related patch set uploaded (by Bartosz Dziewoński):
[WIP] Don't use positional arguments to constructors that take 'config' objects

https://gerrit.wikimedia.org/r/191214

Patch-For-Review

I think for TextInputMenuSelectWidget you should change the parameter name from input to inputWidget -- that wouldn't be a breaking change, since parameter names are meh. Then you can make TextInputMenuSelectWidget take a config option named inputWidget to match, and it won't conflict with the parent, and everything will be wonderful. I think.

I scaled back the patch, and implemented the config-only approach as simply an alternative to positional parameters (and not the recommended, documented way). Too much deprecation for no gain, and the current convention that all config parameters are optional is nice.

Change 192722 had a related patch set uploaded (by Cscott):
Allow passing positional parameters inside the config object

https://gerrit.wikimedia.org/r/192722

Change 191214 abandoned by Bartosz Dziewoński:
Allow passing positional parameters inside the config object

Reason:
Superseded by https://gerrit.wikimedia.org/r/#/c/192722/

https://gerrit.wikimedia.org/r/191214

Change 192722 merged by jenkins-bot:
Allow passing positional parameters inside the config object

https://gerrit.wikimedia.org/r/192722