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

Fix for negative number handling (DECIMAL -51.1234 could not read). #80

Merged
merged 6 commits into from
Mar 9, 2021

Conversation

dominiquegerber
Copy link
Contributor

Thanks for considering my bug report. I have investigated a little deeper and found a solution that seems to work correctly (and read numbers correctly).

The PHPUnit test passes (although not totally, I am running a MariaDB not a MySQL server so the mysql-specific tests are skipped), however I could not seem to find anything that tests that "INTs" are decoded correctly. I'm not really a pro of PHPUnit so I may well have missed something.

Anyway, I'm trying to help on this cool project so here is a stone to the building. Glad if it can help !

Inspired by fengxiangyun/mysql-replication@a9da836

dominiquegerber and others added 2 commits March 2, 2021 11:06
Playing around to get composer use this fork with the negative number fix...
Copy link
Owner

@krowinski krowinski left a comment

Choose a reason for hiding this comment

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

Pls add to test your case with -57.1234 tx

@dominiquegerber
Copy link
Contributor Author

Should be all good this time. I added a the test and fixed the code.

@dominiquegerber
Copy link
Contributor Author

I was not able to test the BinaryDataReader::readInt40Be function (I only managed to go up to 32 bits ints), so I can't tell if it is missing a "fix" like the 32bit or lower version, or if it is ok.

@krowinski krowinski merged commit cc89cda into krowinski:master Mar 9, 2021
@krowinski
Copy link
Owner

GJ! tx for this

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.

2 participants