search

friends-of-reactphp / mysql

Async MySQL database client for ReactPHP.
MIT License
334 stars 66 forks source link

Fix MariaDB support in `FactoryTest` #194

Closed bwaidelich closed 1 month ago

bwaidelich commented 7 months ago

Apparently MariaDB does not contain the

 (using password: YES)

substring in the authentication error message. By making that substring optional in the corresponding regex check, the tests are green for me (11.1.2-MariaDB @ Macbook Pro M1)

bwaidelich commented 7 months ago

how are we making sure this works as expected across different relational database management systems?

I was wondering the same. You could maybe add MariaDB to the CI matrix as it's a fairly common MySQL replacement

WyriHaximus commented 7 months ago

Interested in your and @WyriHaximus's and @clue's opinion on this.

TL;DR: here is my opinion on this: https://github.com/friends-of-reactphp/mysql/pull/196

The test suite is currently green and was green before, so we could argue that this doesn't break anything we're currently testing against and merge it as is. I personally can't really "approve" this PR as it's hard to proof what's currently getting fixed, so maybe postpone until we have tests in place?

We should increase our compatibility matrix. I've just put up https://github.com/friends-of-reactphp/mysql/pull/196 as to how we will achieve that. It's rough on the edges but it is the way forward IMHO. The question is should MariaDB be in that PR or a follow up. Funny enough with MariaDB it's still green. However, only for the previous major version of both, doing both previous and current mayor version and tests start failing: https://github.com/friends-of-reactphp/mysql/actions/runs/8381598989

bwaidelich commented 1 month ago

Closing to remove this one from my list of pending PRs