FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

events: don't inherit from Object.prototype by mscdex · Pull Request #6092 · nodejs/node · GitHub

/ node Public

events: don't inherit from Object.prototype - #6092

Merged
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:events-empty-prototype
Apr 19, 2016
ConversationCommits (1)ChecksFiles changed
Merged

events: don't inherit from Object.prototype#6092
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:events-empty-prototype

Conversation

mscdex commented Apr 7, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)
  • events
Description of change

This commit safely allows event names that are named the same as properties that are ordinarily inherited from Object.prototype such as __proto__.

Fixes: #728

mscdex added events Issues and PRs related to the events subsystem / EventEmitter. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 7, 2016
mscdex force-pushed the events-empty-prototype branch from 8680b06 to 2cec8a1 Compare April 7, 2016 00:39

mscdex commented Apr 7, 2016

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/2186/

mscdex commented Apr 7, 2016

Copy link
Copy Markdown
Contributor Author

Also, there is no performance regression with this change 💃

mscdex added this to the 6.0.0 milestone Apr 7, 2016

thefourtheye commented Apr 7, 2016

Copy link
Copy Markdown
Contributor

Can we make use of the test changes in #2350?

mscdex commented Apr 7, 2016

Copy link
Copy Markdown
Contributor Author

@thefourtheye The included tests should cover it I think

evanlucas commented Apr 7, 2016

Copy link
Copy Markdown
Contributor

@mscdex will this have any affect on process since it sets _events to a regular object?

vkurchatkin commented Apr 7, 2016

Copy link
Copy Markdown
Contributor

@evanlucas it makes sense to simply call EventEmitter on process

mscdex force-pushed the events-empty-prototype branch from 2cec8a1 to 3711c52 Compare April 7, 2016 13:06

mscdex commented Apr 7, 2016

Copy link
Copy Markdown
Contributor Author

@evanlucas Fixed.

CI again: https://ci.nodejs.org/job/node-test-pull-request/2196/

evanlucas commented Apr 7, 2016

Copy link
Copy Markdown
Contributor

LGTM

Fishrock123 reviewed Apr 7, 2016
View reviewed changes
Comment thread src/node.cc Outdated

Fishrock123 Apr 7, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this process._events?

mscdex Apr 7, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes.

Fishrock123 commented Apr 7, 2016

Copy link
Copy Markdown
Contributor

Hmmm, can this be back-ported? Theoretically, this could be considered a security issue for unvalidated input.

Fishrock123 mentioned this pull request Apr 7, 2016
Merged
3 tasks

mscdex commented Apr 7, 2016

Copy link
Copy Markdown
Contributor Author

@Fishrock123 It's definitely a semver-major change, but I don't recall how that plays out with backporting/LTS/etc.

mscdex mentioned this pull request Apr 7, 2016
Closed
3 tasks

mscdex commented Apr 7, 2016

Copy link
Copy Markdown
Contributor Author

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/187/

jasnell commented Apr 7, 2016

Copy link
Copy Markdown
Member

I don't believe back porting would be possible to v4. It might be safe to get into v5 tho.

jasnell commented Apr 7, 2016

Copy link
Copy Markdown
Member

LGTM

mscdex commented Apr 7, 2016

Copy link
Copy Markdown
Contributor Author

CI is green except for a flaky test and citgm is green.

mscdex commented Apr 18, 2016

Copy link
Copy Markdown
Contributor Author

CI again one last time: https://ci.nodejs.org/job/node-test-pull-request/2305/

jasnell commented Apr 18, 2016

Copy link
Copy Markdown
Member

Still LGTM

events: don't inherit from Object.prototype
e38bade 
This commit safely allows event names that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__.

Fixes: nodejs#728
PR-URL: nodejs#6092
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
mscdex force-pushed the events-empty-prototype branch from 3711c52 to e38bade Compare April 19, 2016 01:47
mscdex merged commit e38bade into nodejs:master Apr 19, 2016
mscdex deleted the events-empty-prototype branch April 19, 2016 01:49

Fishrock123 commented Apr 19, 2016

Copy link
Copy Markdown
Contributor

iirc usually we don't care about warnings

jasnell commented Apr 19, 2016

Copy link
Copy Markdown
Member

Typically, but it's nice to avoid them when possible to do so, no?

Fishrock123 commented Apr 19, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

I guess, just consider though, that there are tons of warnings of the same and there isn't necessarily easy ways to avoid it.

thefourtheye mentioned this pull request Apr 19, 2016
Closed
2 tasks
thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Apr 19, 2016
src: fix -Wunused-result warning in e38bade
52a40be 
This patch fixes the warning introduced by the changes in e38bade.

Ref: nodejs#6092
thefourtheye added a commit that referenced this pull request Apr 19, 2016
src: fix -Wunused-result warning in e38bade
697790c 
This patch fixes the warning introduced by the changes in e38bade.

Ref: #6092
PR-URL: #6276
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
events: don't inherit from Object.prototype
0324c24 
This commit safely allows event names that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__.

Fixes: nodejs#728
PR-URL: nodejs#6092
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
src: fix -Wunused-result warning in e38bade
2a45a0b 
This patch fixes the warning introduced by the changes in e38bade.

Ref: nodejs#6092
PR-URL: nodejs#6276
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
events: don't inherit from Object.prototype
c3ad97d 
This commit safely allows event names that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__.

Fixes: #728
PR-URL: #6092
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
src: fix -Wunused-result warning in e38bade
f286e1e 
This patch fixes the warning introduced by the changes in e38bade.

Ref: #6092
PR-URL: #6276
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
emorikawa mentioned this pull request Oct 11, 2016
Closed
BridgeAR mentioned this pull request Apr 1, 2019
Closed
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

events Issues and PRs related to the events subsystem / EventEmitter. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL