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

Address issue #3631 by adding support for a float datatype #3645

Closed
wants to merge 1 commit into from

Conversation

mmucklo
Copy link
Contributor

@mmucklo mmucklo commented Aug 4, 2019

Q A
Type bug
BC Break no
Fixed issues #3631

Summary

Create a new DOUBLE type to satisfy the 'd' type necessary for full Mysqli compatibility as referenced here: mysqli_stmt::bind_param.

(NOTE: new PR b/c rebase didn't work on PR #3639).

@mmucklo
Copy link
Contributor Author

mmucklo commented Aug 4, 2019

@morozov

Regarding your request for a unit test from this comment: #3639 (comment)

I have a full reproducible test setup contained within the docker file posted to #3631

I'm not yet familiar enough with DBAL to quickly come up with a reproducible test case. I would probably need to modify the travis build, and force a language switch in one test (also installation of the relevant files, if possible).

Dealing with travis, unless I setup a local travis server has typically been a royal pain in my experience and fairly finicky, but if that's what's needed to get this to pass, I suppose then I have to somehow figure it out if I'm in a hurry to get this in.

I think what I'd end up doing is just hitting this PR with checkin after checkin until I get a PR that builds the travis test, unless you have a better suggestion?

@mmucklo
Copy link
Contributor Author

mmucklo commented Aug 5, 2019

Still auth errors continuing even with the latest changes.

@morozov
Copy link
Member

morozov commented Aug 5, 2019

@mmucklo can you set the locale to something which uses the comma as the decimal separator in a test and reproduce your case? Please ignore the authentication failures for now. They are inconsistent and are most likely caused by some MySQL internal issue.

@mmucklo mmucklo force-pushed the master branch 25 times, most recently from 5bee0b8 to e2a4e80 Compare August 8, 2019 07:45
@mmucklo
Copy link
Contributor Author

mmucklo commented Aug 8, 2019

@morozov, an update:

After much hacking I got the unit tests to consistently pass, although not excepting travis-ci network issues.

There were a few Mysql connection errors that weren't being caught correctly, which I fixed (see the changes), which was the bulk of the flakey tests, but the IBM DB2 test also turned bad, so I put in a fix I found from this issue lando/hyberdrive#10.

Anyway, finally things got to the new de_DE.UTF-8 test I put together, and it turns out there are 3 other failing tests due to the same issue - see this build:

https://travis-ci.org/doctrine/dbal/jobs/569223911

Anyway, I'm going to have to work other fixes across the board it looks like, but at the end things should work right in non '.' locales.

@morozov
Copy link
Member

morozov commented Aug 9, 2019

Approach-wise, looks good. @mmucklo could you make it one test with a non-default locale instead of an entirely new build job? Also, if there's a connection issue due to the non-default locale, I'd rather like to get it tested explicitly and as a separate pull request to keep the changes under control.

@mmucklo mmucklo force-pushed the master branch 20 times, most recently from 8e70213 to 966acf9 Compare August 12, 2019 06:15
Also try to add a new functional test per request.
@mmucklo
Copy link
Contributor Author

mmucklo commented Aug 13, 2019

@morozov This is ready for review, but please merge #3650 first as there are duplicated changes in this one (in order to get things to build).

There is one error condition that had to be changed in this specific PR as the error message comes back in a locale specific manner, but DBAL was only looking at the english version of the message.

@morozov
Copy link
Member

morozov commented Aug 14, 2019

[…] could you make it one test with a non-default locale instead of an entirely new build job? Also, if there's a connection issue due to the non-default locale, I'd rather like to get it tested explicitly and as a separate pull request to keep the changes under control.

Nothing of the above seems to be addressed.

@mmucklo
Copy link
Contributor Author

mmucklo commented Aug 14, 2019

@morozov

  1. Separate pull request is in Fix travis build instability with MySQL and IBM DB2 #3650.

I can't consistently run the MySQL tests without those fixes (let alone appveyor and IBM DB2 outright fail), and I can't restart each manually as I have no permissions on the repo. If you merge #3650 first, it's a non issue - I just need to merge those changes in order to build.

If you still insist I can pull all of these merged changes out of this branch, but multiple builds will fail, so it will be harder to tell if this is a good patch or not.

Please let me know.

  1. I had a separate "Test Intl" section before, I thought you wanted those moved into "Tests"

Anyway, in order to reproduce the problem, it's required me to call PHPUnit like this:

php -dauto_prepend_file=$PWD/tests/travis/setlocale-de.php vendor/bin/phpunit ....

(in .travis.yml I made such changes).

I can see if I put the code from setlocale-de.php outside of the class at the top of the test files (e.g. DoubleTest.php) if it will work (it may not work, phpunit may need to know about the locale, not just the test) - it's just a royal pain to try to get this whole setup working.

My flow has been:

  1. Make a change
  2. Force push to the fork
  3. Wait 20-30 minutes
  4. Did everything build?
    sometimes the builds fail b/c travis or network issues somewhere, have to go back to step 1
  5. Did any tests fail?
  6. Rinse and repeat.

Doing this in my spare time only gives me so many rounds of this I can do in an evening.

Also - I have been able to loosely get a local docker container setup and hook it up to a mysql container to try and test a single setup, but I have to reset it up every time I want to test locally. It would be nicer if there was some kind of docker-compose command I could run and get an instant dev environment with all the databases hanging off to the side.

But per your wish I will see what I can do to change this behavior when I have time.

@mmucklo
Copy link
Contributor Author

mmucklo commented Aug 17, 2019

@morozov

Started by splitting up my other PR #3650.

Will get back to this one time permitting and see what I can do.

Base automatically changed from master to 4.0.x January 22, 2021 07:44
@morozov morozov closed this Oct 26, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants