#17250 closed defect (fixed)
_TimePicker breaks if non-plain-Object is passed as `constraints`
Reported by: | Colin Snover | Owned by: | |
---|---|---|---|
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 8 years ago by
Milestone: | tbd → 1.9.2 |
---|---|
Priority: | undecided → blocker |
comment:2 Changed 8 years ago by
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 8 years ago by
PS: For the record, the workaround to this limitation is to use lang.mixin():
new _TimePicker({ constraints: lang.mixin({}, fancyObject) });
comment:4 Changed 8 years ago by
@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 7 years ago by
Owner: | set to Colin Snover <[email protected]…> |
---|---|
Resolution: | → fixed |
Status: | new → closed |
comment:9 Changed 7 years ago by
Milestone: | 1.9.2 → 1.7.6 |
---|---|
Version: | 1.9.0 → 1.7.5 |
Interesting how like 75% of the unit tests between 1.8 and 1.9 were deleted.
comment:10 Changed 7 years ago by
Yes, it's because the drop down was replaced with a completely different and much simpler UI.
Did a quick review of other places in the codebase with _setConstraintsAttr:
dijit/form/ValidationTextBox.js
Same issue:
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.