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

Don't send dispatcher parameters #13123

Merged
merged 2 commits into from
Oct 21, 2017
Merged

Don't send dispatcher parameters #13123

merged 2 commits into from
Oct 21, 2017

Conversation

dugwood
Copy link
Contributor

@dugwood dugwood commented Oct 18, 2017

  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

Small description of change:
These will erase previously set variables, and dispatcher's params were never accessible this way so far, so removing the argument in this function is fine.

These will erase previously set variables, and dispatcher's params were never accessible this way so far, so removing the argument in this function is fine.
@dugwood
Copy link
Contributor Author

dugwood commented Oct 18, 2017

@Jurigag thanks, that should do the trick! Hope the release will happen soon (o.o')

@Jurigag
Copy link
Contributor

Jurigag commented Oct 18, 2017

Update changelog also but whatever.

@dugwood
Copy link
Contributor Author

dugwood commented Oct 18, 2017

Sorry, forgot that too :-( Updating right now!

@dugwood
Copy link
Contributor Author

dugwood commented Oct 18, 2017

You mean this file?
https://github.com/dugwood/cphalcon/blob/patch-1/CHANGELOG-3.2.md

So I add a 3.2.4?

@Jurigag
Copy link
Contributor

Jurigag commented Oct 18, 2017

Yes

@sergeyklay sergeyklay self-requested a review October 18, 2017 14:11
@sergeyklay sergeyklay self-assigned this Oct 18, 2017
@dugwood
Copy link
Contributor Author

dugwood commented Oct 18, 2017

Updated, waiting for rebuild. Thanks @Jurigag for your help.

@sergeyklay
Copy link
Contributor

I will keep this open for a day or two to allow other contributors a chance to speak up, and to take a fresh look at it later. Anyway we can to release this on weekends. But at the moment it seems good to me. Thank you or contributing!

@dugwood
Copy link
Contributor Author

dugwood commented Oct 18, 2017

You're welcome. I've fixed my code where I've found exceptions (variable changed from an object to a scalar value, ouch), so for now it's «okay». But I'll revert my own code when this fix goes live :-)

@dugwood
Copy link
Contributor Author

dugwood commented Oct 18, 2017

The point is to release it soon before a lot of people upgrade to 3.2.3 and face this very issue. Hope there's not a lot of them :-) If it's in about a week or so, that's great!

@sergeyklay sergeyklay merged commit bf2f6d7 into phalcon:3.2.x Oct 21, 2017
@sergeyklay
Copy link
Contributor

Thank you

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.

3 participants