X Tutup
Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(injector): make sure we call Function.prototype.toString not an override#14361

Closed
a510 wants to merge 3 commits intoangular:masterfrom
a510:master
Closed

fix(injector): make sure we call Function.prototype.toString not an override#14361
a510 wants to merge 3 commits intoangular:masterfrom
a510:master

Conversation

@a510
Copy link
Copy Markdown
Contributor

@a510 a510 commented Apr 1, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix

What is the current behavior? (You can also link to an open issue here)

the injector uses fn.toString to extract function arguments

What is the new behavior (if this is a feature change)?

the injector uses Function.prototype.toString.call to avoid calling an override

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

Other information:

use Function.prototype.toString.call instead of fn.toString, because the user might override the toString function of the "fn" instance, without knowing it's being used by the injector.

… an override

use Function.prototype.toString.call instead of fn.toString, because the user might override the toString function of the "fn" instance, without knowing it's being used by the injector.
@lgalfaso
Copy link
Copy Markdown
Contributor

lgalfaso commented Apr 2, 2016

Hi @a510, the change looks fine, but a test would be mostly welcome.

@lgalfaso
Copy link
Copy Markdown
Contributor

lgalfaso commented Apr 2, 2016

and this should be marked as fix not as refactor

@a510 a510 changed the title refactor(injector): make sure we call Function.prototype.toString not an override fix(injector): make sure we call Function.prototype.toString not an override Apr 3, 2016
@Narretz Narretz added this to the Backlog milestone Apr 7, 2016
@gkalpak gkalpak closed this in a0b5e1a Apr 11, 2016
gkalpak pushed a commit that referenced this pull request Apr 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

X Tutup