{"action":"created","issue":{"url":"https://api.github.com/repos/meteor/meteor/issues/8031","repository_url":"https://api.github.com/repos/meteor/meteor","labels_url":"https://api.github.com/repos/meteor/meteor/issues/8031/labels{/name}","comments_url":"https://api.github.com/repos/meteor/meteor/issues/8031/comments","events_url":"https://api.github.com/repos/meteor/meteor/issues/8031/events","html_url":"https://github.com/meteor/meteor/pull/8031","id":188154227,"number":8031,"title":"Make sure DDP._CurrentInvocation is never set inside publish functions","user":{"login":"mitar","id":585279,"avatar_url":"https://avatars.githubusercontent.com/u/585279?v=3","gravatar_id":"","url":"https://api.github.com/users/mitar","html_url":"https://github.com/mitar","followers_url":"https://api.github.com/users/mitar/followers","following_url":"https://api.github.com/users/mitar/following{/other_user}","gists_url":"https://api.github.com/users/mitar/gists{/gist_id}","starred_url":"https://api.github.com/users/mitar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mitar/subscriptions","organizations_url":"https://api.github.com/users/mitar/orgs","repos_url":"https://api.github.com/users/mitar/repos","events_url":"https://api.github.com/users/mitar/events{/privacy}","received_events_url":"https://api.github.com/users/mitar/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":16,"created_at":"2016-11-09T02:24:44Z","updated_at":"2016-12-02T22:50:23Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/meteor/meteor/pulls/8031","html_url":"https://github.com/meteor/meteor/pull/8031","diff_url":"https://github.com/meteor/meteor/pull/8031.diff","patch_url":"https://github.com/meteor/meteor/pull/8031.patch"},"body":"`_setUserId` is normally called from a Meteor method with `DDP._CurrentInvocation` set. But `DDP._CurrentInvocation` is not expected to be set inside a publish function, so we have to temporary unset it.\r\n\r\nOtherwise, this can lead to a problem that if a publish function is calling server-side Meteor methods, their `this.connection` property is set when publish function is restarted here, but the property should not be set."},"comment":{"url":"https://api.github.com/repos/meteor/meteor/issues/comments/264583789","html_url":"https://github.com/meteor/meteor/pull/8031#issuecomment-264583789","issue_url":"https://api.github.com/repos/meteor/meteor/issues/8031","id":264583789,"user":{"login":"mitar","id":585279,"avatar_url":"https://avatars.githubusercontent.com/u/585279?v=3","gravatar_id":"","url":"https://api.github.com/users/mitar","html_url":"https://github.com/mitar","followers_url":"https://api.github.com/users/mitar/followers","following_url":"https://api.github.com/users/mitar/following{/other_user}","gists_url":"https://api.github.com/users/mitar/gists{/gist_id}","starred_url":"https://api.github.com/users/mitar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mitar/subscriptions","organizations_url":"https://api.github.com/users/mitar/orgs","repos_url":"https://api.github.com/users/mitar/repos","events_url":"https://api.github.com/users/mitar/events{/privacy}","received_events_url":"https://api.github.com/users/mitar/received_events","type":"User","site_admin":false},"created_at":"2016-12-02T22:50:23Z","updated_at":"2016-12-02T22:50:23Z","body":"> By making this statement aren't you making the assumption that publication restarts are rare? I don't know if that's the case. Publications might get restarted frequently or as many times as they are started in many applications. I would need data from users to make this conclusion.\r\n\r\nPublications restarts because of the login while you are subscribed is definitely rarer than \"as many times as they are started in many applications\". In any case, I am not claiming or assuming anything about how often restart happens. I am saying that it seems that nobody until now cared to investigate or report why server-side method which uses information from `this.connection` and is called from inside the publish function, has `this.connection` set when logging in. You do agree it is rare that somebody is calling a server side method from inside a publish function? And I am saying that it is even rarer that that method happens to inspect this.connection, and even rarer that it would care for a behavior that this.connection is set during a login. I mean, only if somebody discovered this bug and made a workaround. But even then they should probably also report it. I have not seen one.\r\n\r\n> You're proposing that client => publish => method should not have this.connection set.\r\n\r\nI am proposing that we should not change behavior, because this is how it is currently.\r\n\r\n> I'm proposing that maybe client => publish => method should have this.connection set. \r\n\r\nYou are proposing to make a backwards incompatible change, so I am asking why?\r\n\r\nWhat I am reporting is a bug in current behavior and a fix for it. What you are proposing is a backwards incompatible change and a feature change.\r\n\r\nWhat I am saying is that we should merge a bug fix. And then if somebody wants to tackle a feature change they can open a ticket, pull request, or whatever. I do not need that feature, but I need the bug fixed. If you do not need that feature either, then let's maybe just agree on fixing the bug?"}}
{"actor":{"display_login":"mitar"}}