Add dependency injection to MySqlDatabase, and MySqlProperties #924
Conversation
…t for retrieving accounts via account id using a mock mysql object
…ields when retrieving account. initial attempt at getting travis to run unit tests
… integration tests with database. added travis config for launching test test mysql instance in order to run integration tests. no clue if it's gonna work yet, though
…s a test, and fails with a warning that there are no test functions in the file.
…hould get rid of any type hinting warnings from scrutinizer.
# Conflicts: # .travis.yml # composer.json # docker-compose.yml # lib/Default/MySqlDatabase.class.php # phpunit.xml # test/SmrTest/BaseIntegrationSpec.php
… using the container
# Conflicts: # .env.sample # .travis.yml # composer.json # docker-compose.yml # engine/Default/game_play.php # engine/Default/history_alliance_detail.php # engine/Default/history_games.php # engine/Default/history_games_detail.php # engine/Default/history_games_hof.php # engine/Default/history_games_news.php # lib/Default/MySqlDatabase.class.php # lib/Default/SmrMySqlDatabase.class.php # phpunit.xml # test/SmrTest/BaseIntegrationSpec.php
There was a problem hiding this comment.
This looks okay in general so far, though the benefits are not manifestly obvious to me yet. I see how the containers are being used in the tests, and that is nice, but I feel like I've done the same sort of mocking in python, for example, without needing IoC/DI (since these techniques are sometimes too heavyweight for simple programs).
Practically speaking, the MySqlDatabase class is now certainly more "pure", but it is also less clear to me how it interacts with the rest of the codebase. The static mysqli attribute in comparison is quite straightforward.
The design looks solid though. Happy to continue with this effort and see what we look like on the other side!
XaeroDegreaz
force-pushed
the
922-php-di-mysqldatabase
branch
from
c47cefd to
50c6b56
Compare
…o fail during coverage generation
XaeroDegreaz
force-pushed
the
922-php-di-mysqldatabase
branch
from
50c6b56 to
42b6535
Compare
Codecov Report
@@ Coverage Diff @@
## master #924 +/- ##
===========================================
+ Coverage 1.58% 2.33% +0.74%
- Complexity 4061 4074 +13
===========================================
Files 60 60
Lines 10157 10196 +39
===========================================
+ Hits 161 238 +77
+ Misses 9996 9958 -38
Continue to review full report at Codecov.
|
…etInstance/getNewInstance
…abase # Conflicts: # lib/Default/SmrShip.class.php # lib/Default/SmrWeapon.class.php
| * @throws \DI\NotFoundException | ||
| */ | ||
| private static function ensureConnected(MySqlDatabase $mysqlDatabase): MySqlDatabase { | ||
| if (!isset($mysqlDatabase->dbConn)) { |
There was a problem hiding this comment.
Now that I'm thinking about this, I can probably just get away with calling DiContainer::initialize() to reset the whole container. If not, then in the future after things are retrieving services through the container, they could end up with a defucnt transient MySqlDatabase instance if the connection is ever closed. That would be a lot of manual checks everywhere to make sure reconnect logic is triggered.
I think it's not a big deal since only those CLI apps are using the close method, but once the migration starts happening where they don't access db directly, but through domain services, this could be a problem. Maybe just reset the entire DI container for those CLI apps instead of having this reconnect logic.
There was a problem hiding this comment.
What you have set up now looks like it reproduces the existing behavior. But I agree that avoiding a defunct MySqlDatabase instance is probably more robust (especially if future abstraction layers would make misuse easier). However, a few questions I don't have answers for off the top of my head:
What would DiContainer::initialize() do to the existing connection? Would it try to close it (if so, might this throw if the connection has timed out)? Or would it leave it open (if so, might this result in a proliferation of open database connections)?
Also, it's important to keep in mind that not all CLI events even need a connection to the database, so I'm not exactly sure where a DiContainer::initialize() would go (if we're trying to be as efficient as possible). There are plenty of "reasonable" places to put it, though!
So I'm happy to leave this as-is for now, but if you want to look for a better solution, that's fine with me too.
Fixing typos.
The `SmrShip::checkPowerLevel` function was erroneously re-added during merge conflict resolution. It is now removed again.
|
This is merged now. Thanks for your contribution (and your patience)! |
Fixes a bug with overlapping queries clobbering previous results. The old SmrMySqlDatabase had a non-static `$dbResult`, so any call to `new SmrMySqlDatabase()` would have a new `$dbResult`. However, the original design of `MySqlDatabase::getInstance` from #924 shared a `$dbResult` (due to the shared MySqlDatabase from the DI container), which caused query results to be overwritten if another query was made while processing the results of the first query. The global variable `$db` is kind of what `MySqlDatabase::getInstance` was supposed to be. We now only use `getNewInstance` (which has been renamed `getInstance` to reduce the footprint of the diff). To handle the reconnection logic for CLI programs, we set the `mysqli` instance in the DI container to null, and then reconnect in the MySqlDatabase ctor if it is passed a null `$dbConn` (since there is no known way to "unset" an entry in the DI container so that it is regenerated on-demand the next time it is requested). Co-authored-by: Dan Hemberger <[email protected]>
This fixes an omission in #924.
In smrealms#924, the `$dbConn` property was changed so that it is unset when `close()` is called instead of being set to `false`. Therefore, when we call `close()` on an already closed MySqlDatabase instance, we need to perform an `isset` check on `$dbConn` instead of a boolean check. This fix allows us to make repeated calls to `close()` again, and we add a test to ensure this functionality. Note that this impacted the IRC bot, which will make repeated calls to close without reconnecting to the database if it processes an event that doesn't require the database.
In #924, the `$dbConn` property was changed so that it is unset when `close()` is called instead of being set to `false`. Therefore, when we call `close()` on an already closed MySqlDatabase instance, we need to perform an `isset` check on `$dbConn` instead of a boolean check. This fix allows us to make repeated calls to `close()` again, and we add a test to ensure this functionality. Note that this impacted the IRC bot, which will make repeated calls to close without reconnecting to the database if it processes an event that doesn't require the database.
closes #922