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

Add warningCount to QueryResult #82

Merged

Conversation

legionth
Copy link

A count of warnings is currently added via Io\Parser::onSuccess(). This value is dynamically added as attribute to the instance of QueryCommand.

This PR will add this value like affected_rows to the QueryCommand and QueryResult class and adds its actual value (if given) to the instance of these classes.

@legionth legionth changed the title Add dynamicly declared 'warnCount' to QueryCommand and QueryResult Add dynamically declared 'warnCount' to QueryCommand and QueryResult Oct 10, 2018
Copy link
Contributor

@clue clue left a comment

Choose a reason for hiding this comment

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

@legionth Thank you for filing this PR! I agree that it makes sense to add this to the QueryResult.

I'm a bit unsure about the naming, what do you think about warningCount instead? This way, it would mimic both PHP's ext-mysqli as well as node-mysql.

@legionth
Copy link
Author

Makes perfectly sense 👍 Just wanted to be align with the naming of Io\Parser. I changed the variable names in the latest commit. Check it out 👍

Copy link
Contributor

@clue clue left a comment

Choose a reason for hiding this comment

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

Thank you for the quick update, LGTM! 👍

@clue clue added this to the v0.4.1 milestone Oct 11, 2018
@clue clue changed the title Add dynamically declared 'warnCount' to QueryCommand and QueryResult Add warningCount to QueryResult Oct 11, 2018
@WyriHaximus WyriHaximus merged commit a9aa011 into friends-of-reactphp:master Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants