ENH Add MySQL 8.4 to matrix #112
Conversation
emteknetnz
force-pushed
the
pulls/1/mysql84
branch
from
d7aaa3f to
862c085
Compare
emteknetnz
force-pushed
the
pulls/1/mysql84
branch
from
862c085 to
1836950
Compare
There was a problem hiding this comment.
Can you please explain the implementation to me? Seems like we should just be either:
- always outputting all DB versions we need from
generatePhpToDBMapand relying on deduping to avoid duplicates - Passing the prefer-lowest db into
generatePhpToDBMapso it knows not to duplicate it, and then still outputting all the ones we do still need
And then we don't need to split code into the new getDBs() nor do we need a foreach ($dbNotAdded as $db) {?
There's probably some context I'm missing though, I think.
There was a problem hiding this comment.
generatePHPToDBMap() makes an assumption that there are greater or equal number of PHP versions to DB versions (minus 1, which is manually added for the --prefer-lowest build). However we now have a situation where there are more DB versions that PHP versions
Previously for CMS 6, which only supports PHP 8.3, we had mariadb manually added for --prefer-lowest, then a single PHP version (8.3), and a single DB (mysql80). However now we mysql84 as well, hence the $dbNotAdded code
There was a problem hiding this comment.
Should this just wait until after PHP 8.4 support is added for CMS 6 then? It feels like a fair amount of complexity is being added to an already pretty messy file.
As an aside, we should probably refactor this to be a few separate classes at some stage so it's easier to see where things are happening for maintenance purposes. But that's clearly not in scope for this PR.
There was a problem hiding this comment.
I don't think it needs to wait. I think it's worth while code to add as we'll end up in the situation again where there's more DB's than PHP's whenever we release a new major.
Issue #95