_TimePicker breaks if non-plain-Object is passed as `constraints`

[Colin Snover]

In _TimePicker#_setConstraintsAttr:

_setConstraintsAttr: function(/* Object */ constraints){
        // brings in increments, etc.
        for(var key in constraints){
                this._set(key, constraints[key]);
        }
        // ...
}

This code makes indiscriminately overwrites properties of _TimePicker by properties of the constraints object *and* the constraints object’s prototype chain. This means if you pass ANY non-plain Object, like a Stateful, you’ve just nuked _TimePicker’s constructor property and broken everything in a really confusing way since declare relies on that property to function.

Constraints should not be mixed into the _TimePicker instance indiscriminately like this. If specific properties need to be pulled from the constraints object for some reason, pull just those ones. No matter what, definitely don’t ever copy constructor.

[dylan]

Did a quick review of other places in the codebase with _setConstraintsAttr:

dijit/form/ValidationTextBox.js

Same issue:

		_setConstraintsAttr: function(/*__Constraints*/ constraints){
			if(!constraints.locale && this.lang){
				constraints.locale = this.lang;
			}
			this._set("constraints", constraints);
			this._refreshState();
		}

Each of the following modules passes the constraints object up the inheritance chain. There's not an issue with code in this module, but there is likely an issue given the the code in ValidationTextBox?.

dijit/form/_DateTimeTextBox.js dijit/form/_Spinner.js dijit/form/CurrencyTextBox.js dijit/form/NumberTextBox.js

dojox/widget/_CalendarBase.js

dojox/widget/_CalendarBase.js does not do a for/in, but simply sets the constraints object on the instance to what is passed, and then looks for relevant values and applies them. That said, this widget may not work properly with stateful beyond get/set on its value, as it is setting min and max directly on the constraints object rather than using _set

I would nominate this as something that should be fixed in 1.9.x and backported.

[bill]

I'm sure there are lots of places that only work with a plain Object. It's worked that way for years. But anyway if you want to submit a patch with an automated test case then I'm not opposed to it.

As for accessing the values via constraints.get("foo") and set() or _set() instead of just directly accessing constraints.foo, that sounds like overkill, and certainly not something to put into a point release.

[bill]

PS: For the record, the workaround to this limitation is to use lang.mixin():

new _TimePicker({
   constraints: lang.mixin({}, fancyObject)
});

[Colin Snover]

@dylan I don’t see how that code in ValidationTextBox? is related to this issue. It sets the locale property of the constraints object and then sets the constraints property to that object. That is not the same as a blind bulk property copy which is what this ticket is about.

[Colin Snover <github.com@…>]

In 492fbe3cbfff2987c7b29fb7b5232f4c9ee7566a/dijit:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[Colin Snover <github.com@…>]

In e657e2aa25bde2a38d38c22ede1a7087e1113f21/dijit:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[Colin Snover <github.com@…>]

In fe0150061508c7594b425f9a99a9d205543dbffc/dijit:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[Colin Snover <github.com@…>]

In 76142f0364f5fba3463f1627f8b61175fdca8b23/dijit:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[Colin Snover]

Interesting how like 75% of the unit tests between 1.8 and 1.9 were deleted.

[bill]

Yes, it's because the drop down was replaced with a completely different and much simpler UI.