[patch][cla] lang::exists returning false positives

[mccormack]

For example dojo.exists("foo.bar", {foo:0}) === true

Please see attached patch and test cases.

[mccormack]

lang exists/setObject update for false positive bug

[mccormack]

Changes to getProps and setObject - added to existing tests

[bill]

Bumping this ticket since we are past the deadline for the 1.9RC. The fix can be put into 1.9.1 too, if desired.

[dylan]

I'll create a pull request from this asap.

[dylan]

Cleaned this up a bit and tried to reduce the size of the various comparison checks, pull request at ​https://github.com/dojo/dojo/pull/81

[bill]

As for fixing the bug with exists(), that's fine.

As for the patch, there's a philosophical question, because the patch affects not just exists() but modifies the behavior of getObject() and setObject(), and then it adds unit tests to confirm that getObject() and setObject() behave in IMO an unintuitive way.

Given an object like:

var test = {
	foo : 0
};

Then what should

lang.getObject("foo.bar", true, test);

or

lang.setObject("foo.bar", "hi", test);

do?

In order to create test.foo.bar, you have to overwrite test.foo from 0 to {}. IMO that's beyond the spec of getObject() which says:

Objects will be created at any point along the 'path' that is *undefined*.

As for setObject(), it's spec is vague, it just says:

Objects are created as needed along path.

Since it doesn't mention overwriting existing values, I figure it isn't supposed to, but the spec admittedly isn't explicit.

[bill]

After realizing this ticket is about a corner case (where the path reference primitive properties (numbers, booleans, etc.), it seems much less important.

But I made an alternate patch anyway, see ​https://github.com/dojo/dojo/pull/82.

I modified the tests from the original patch to only check the corner case for exists().

[bill]

I'm going to check in a highly modified version of the original patch. About your tests:

1. delete window.foo (and delete dojo.global.foo throw an exception on IE8, so I removed that testing code
2. I also removed the checks that getObject() and setObject() overwrite existing properties if they are primitive types; I'd rather just say that the behavior is undefined in that corner case.

[Bill Keese <[email protected]…>]

In 84cbaf9144b7c5ce1ef309f8297e8426066de0eb/dojo:

Error: Processor CommitTicketReference failed

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

[Bill Keese <[email protected]…>]

In 2957d1b090ae44f2e821e98ea754c99700672d5a/dojo:

Error: Processor CommitTicketReference failed

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

[dylan]

Per Claudio Procida on twitter:

Undocumented change: dojo.getObject("a.b", false, {a:null}) 1.9 null dojo.getObject("a.b", false, {a:null}) 1.10 undefined

Changed behavior is a side effect of the fix for this ticket

He suggests that

lang.getObject("a.b", false, {a:null}) should throw TypeError?: Cannot read property 'b' of null instead of returning undefined or null.

[bill]

Undocumented change: dojo.getObject("a.b", false, {a:null}) 1.9 null dojo.getObject("a.b", false, {a:null}) 1.10 undefined

That's a bug fix. Although the API doc is unspecific about the return value when the path doesn't exist, 1.9 returns undefined in general:

lang.getObject("a.b", false, {}) undefined

Therefore it should return undefined in this corner case too.

He suggests that lang.getObject("a.b", false, {a:null}) should throw ... instead of returning undefined or null

We can't make such a drastic change because it breaks backwards compatibility and relatedly will break existing applications.