Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent ungettable objects from getting gotten during path lookup #13461

Merged
merged 1 commit into from May 9, 2016

Conversation

pittst3r
Copy link
Contributor

@pittst3r pittst3r commented May 6, 2016

This is a follow-up to #13449.

It was observed by @krisselden that doing something like the following…

Ember.get({ foo: 42 }, 'foo.bar');

… would eventually call get internally like…

get(42, 'bar');

… which is no bueno.

To generalize, we should only call get from _getPath with objects (except null), functions, and strings. I'm also wondering if we should add a deprecation to warn people when they do something like…

Ember.get(42, 'bar');

… so we can eventually add an assertion. Right now that will just return undefined.

return false;
}

let allowableTypes = ['object', 'function', 'string'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to const outside the function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also can just be an object literal with 1s

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like:

const ALLOWABLE_TYPES = {
  object: true,
  function: true,
  string: true
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. Thanks, guys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed the update. In the update I am casting undefined into false, so let me know if you believe that to be superfluous and I will remove.

@krisselden
Copy link
Contributor

krisselden commented May 6, 2016

I think @mmun wanted a test for obj = { str: '' }; strictEquals(get(obj, 'str.length'), 0);

@pittst3r
Copy link
Contributor Author

pittst3r commented May 6, 2016

There's already one here, but now that I'm looking at it again it's only equal and not strictEqual. I can update that.

@pittst3r
Copy link
Contributor Author

pittst3r commented May 6, 2016

K, strictEqual added. 👍

return false;
}

return !!ALLOWABLE_TYPES[typeof obj];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the explicit coercion is needed here. It is probably fine either way though...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed the update for this.

@pittst3r pittst3r changed the title [WIP] [BUGFIX] Prevent ungettable objects from getting gotten during path lookup [BUGFIX] Prevent ungettable objects from getting gotten during path lookup May 9, 2016
@krisselden
Copy link
Contributor

This looks good, though, this is cleanup, not really a BUGFIX.

@krisselden krisselden changed the title [BUGFIX] Prevent ungettable objects from getting gotten during path lookup Prevent ungettable objects from getting gotten during path lookup May 9, 2016
@krisselden krisselden merged commit e79d18b into emberjs:master May 9, 2016
rwjblue pushed a commit that referenced this pull request May 9, 2016
…3461)

Prevent ungettable objects from getting gotten during path lookup
(cherry picked from commit e79d18b)
toddjordan pushed a commit to toddjordan/ember.js that referenced this pull request Sep 9, 2016
…berjs#13461)

Prevent ungettable objects from getting gotten during path lookup
@rwjblue rwjblue mentioned this pull request Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants