Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#17250 closed defect (fixed)

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

Reported by: Colin Snover Owned by: Colin Snover <github.com@…>
Priority: blocker Milestone: 1.7.6
Component: Dijit Version: 1.7.5
Keywords: Cc:
Blocked By: Blocking:

Description

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.

Change History (10)

comment:1 Changed 6 years ago by dylan

Milestone: tbd1.9.2
Priority: undecidedblocker

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.

comment:2 Changed 6 years ago by 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.

comment:3 Changed 6 years ago by bill

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

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

comment:4 Changed 6 years ago by 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.

comment:5 Changed 5 years ago by Colin Snover <github.com@…>

Owner: set to Colin Snover <github.com@…>
Resolution: fixed
Status: newclosed

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? 

comment:6 Changed 5 years ago by 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? 

comment:7 Changed 5 years ago by 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? 

comment:8 Changed 5 years ago by 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? 

comment:9 Changed 5 years ago by Colin Snover

Milestone: 1.9.21.7.6
Version: 1.9.01.7.5

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

comment:10 Changed 5 years ago by bill

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

Note: See TracTickets for help on using tickets.