[BUGFIX beta] `Ember.get` with first part of path chain resolving to `null` returns `null` rather than `undefined` #13449

pittst3r · 2016-05-04T21:19:05Z

Addresses issue #13444.

As is:

Ember.get({ foo: null }, 'foo.bar') === null

Expected:

Ember.get({ foo: null }, 'foo.bar') === undefined

This bug results in:

let o = Ember.Object.extend({ foo: null }).create();
o.get('foo.bar') || 'x' === 'x' 
o.getWithDefault('foo.bar', 'x') === null // wat

krisselden · 2016-05-05T06:45:56Z

Good catch

pittst3r · 2016-05-05T11:13:08Z

packages/ember-runtime/tests/legacy_1x/mixins/observable/observable_test.js

@@ -129,8 +129,8 @@ QUnit.test('should return null when property value is null on Ember.Observable',
 });

 QUnit.test('should call unknownProperty when value is undefined on Ember.Observable', function() {
-  equal(get(object, 'unknown'), 'unknown');


This would fail if only the Ember.get() module was run and relied on the object.get() module being run first to pass—for obvious reasons.

Ugh. Good catch!

pittst3r · 2016-05-05T16:38:40Z

K, ready for review!

… `null` rather than `undefined`

mmun · 2016-05-05T19:11:00Z

Looks good!

@rwjblue @krisselden Does this need a canary cycle or can we put it into beta?

krisselden · 2016-05-05T21:16:04Z

packages/ember-metal/lib/property_get.js

@@ -76,7 +76,7 @@ export function _getPath(root, path) {

  for (let i = 0; i < parts.length; i++) {
    if (obj == null) {


we only want to call get on object like things, this should do if (!isObjectLike(obj)) { return undefined; }

To be specific, I would check falsy, and that the typeof was 'string', 'object', 'function' but at a minimum this can simply check falsy. I'm sure the obj == null check was a bad fix to this sometimes returning false vs undefined.

Falsey alone won't work because of empty strings, e.g.

Ember.get({ foo: "" }, 'foo.length') should be 0 not undefined. (Can you add a test for this one @robbiepitts? if there isn't one already)

Confirm, using get for string.length seems super lame.

krisselden · 2016-05-05T21:39:28Z

This change is low risk. I think it is bad that we let random truthy values get to get() that aren't gettable too, not just that we return null sometimes.

pittst3r · 2016-05-05T21:43:32Z

@krisselden Agreed. Would you like that addressed in this PR? Alternatively I can create an issue and work on it as a separate thing. Glad to do either.

krisselden · 2016-05-05T21:47:52Z

@robbiepitts at least in this PR reduce it to just a falsy check.

rwjblue · 2016-05-05T21:59:55Z

Seems like [BUGFIX beta] to me.

mixonic · 2016-05-06T14:47:37Z

👍

…`null` returns `null` rather than `undefined` (#13449) (cherry picked from commit 82be117)

…`null` returns `null` rather than `undefined` (emberjs#13449)

pittst3r changed the title ~~[WIP BUGFIX] Ember.get with first part of path chain resolving to null results in null rather than undefined~~ [WIP] [BUGFIX] Ember.get with first part of path chain resolving to null returns null rather than undefined May 4, 2016

locks mentioned this pull request May 4, 2016

getWithDefault broken with chained properties #13444

Closed

pittst3r force-pushed the get-bug branch from f6e6bfb to 31f5f4f Compare May 5, 2016 11:06

pittst3r reviewed May 5, 2016
View reviewed changes

pittst3r force-pushed the get-bug branch from 31f5f4f to 5e8656c Compare May 5, 2016 16:12

pittst3r changed the title ~~[WIP] [BUGFIX] Ember.get with first part of path chain resolving to null returns null rather than undefined~~ [BUGFIX] Ember.get with first part of path chain resolving to null returns null rather than undefined May 5, 2016

pittst3r changed the title ~~[BUGFIX] Ember.get with first part of path chain resolving to null returns null rather than undefined~~ [WIP] [BUGFIX] Ember.get with first part of path chain resolving to null returns null rather than undefined May 5, 2016

Ember.get with first part of path chain resolving to null returns…

bf8bedf

… `null` rather than `undefined`

pittst3r force-pushed the get-bug branch from 5e8656c to bf8bedf Compare May 5, 2016 18:30

pittst3r changed the title ~~[WIP] [BUGFIX] Ember.get with first part of path chain resolving to null returns null rather than undefined~~ [BUGFIX] Ember.get with first part of path chain resolving to null returns null rather than undefined May 5, 2016

krisselden reviewed May 5, 2016
View reviewed changes

krisselden changed the title ~~[BUGFIX] Ember.get with first part of path chain resolving to null returns null rather than undefined~~ [BUGFIX beta] Ember.get with first part of path chain resolving to null returns null rather than undefined May 6, 2016

krisselden merged commit 82be117 into emberjs:master May 6, 2016

pittst3r mentioned this pull request May 6, 2016

Prevent ungettable objects from getting gotten during path lookup #13461

Merged

rwjblue pushed a commit that referenced this pull request May 9, 2016

[BUGFIX beta] Ember.get with first part of path chain resolving to …

9c45b02

…`null` returns `null` rather than `undefined` (#13449) (cherry picked from commit 82be117)

toddjordan pushed a commit to toddjordan/ember.js that referenced this pull request Sep 9, 2016

[BUGFIX beta] Ember.get with first part of path chain resolving to …

8f78e93

…`null` returns `null` rather than `undefined` (emberjs#13449)

rwjblue mentioned this pull request Oct 5, 2017

cleanup _getPath #15692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX beta] `Ember.get` with first part of path chain resolving to `null` returns `null` rather than `undefined` #13449

[BUGFIX beta] `Ember.get` with first part of path chain resolving to `null` returns `null` rather than `undefined` #13449

pittst3r commented May 4, 2016

krisselden commented May 5, 2016

pittst3r May 5, 2016

rwjblue May 5, 2016

pittst3r commented May 5, 2016

mmun commented May 5, 2016

krisselden May 5, 2016

krisselden May 5, 2016

mmun May 5, 2016 •

edited

Loading

krisselden May 6, 2016

krisselden commented May 5, 2016

pittst3r commented May 5, 2016 •

edited

Loading

krisselden commented May 5, 2016

rwjblue commented May 5, 2016

mixonic commented May 6, 2016

		@@ -76,7 +76,7 @@ export function _getPath(root, path) {

		for (let i = 0; i < parts.length; i++) {
		if (obj == null) {

[BUGFIX beta] Ember.get with first part of path chain resolving to null returns null rather than undefined #13449

[BUGFIX beta] Ember.get with first part of path chain resolving to null returns null rather than undefined #13449

Conversation

pittst3r commented May 4, 2016

krisselden commented May 5, 2016

pittst3r May 5, 2016

Choose a reason for hiding this comment

rwjblue May 5, 2016

Choose a reason for hiding this comment

pittst3r commented May 5, 2016

mmun commented May 5, 2016

krisselden May 5, 2016

Choose a reason for hiding this comment

krisselden May 5, 2016

Choose a reason for hiding this comment

mmun May 5, 2016 • edited Loading

Choose a reason for hiding this comment

krisselden May 6, 2016

Choose a reason for hiding this comment

krisselden commented May 5, 2016

pittst3r commented May 5, 2016 • edited Loading

krisselden commented May 5, 2016

rwjblue commented May 5, 2016

mixonic commented May 6, 2016

[BUGFIX beta] `Ember.get` with first part of path chain resolving to `null` returns `null` rather than `undefined` #13449

[BUGFIX beta] `Ember.get` with first part of path chain resolving to `null` returns `null` rather than `undefined` #13449

mmun May 5, 2016 •

edited

Loading

pittst3r commented May 5, 2016 •

edited

Loading