search

silverstripe / silverstripe-sqlite3

SQLite3 DB Adapter for Silverstripe
BSD 3-Clause "New" or "Revised" License
8 stars 19 forks source link

BUG Properly deprecate old 'memory' config setting #4

Closed tractorcow closed 10 years ago

tractorcow commented 10 years ago

Recently the $databaseConfig['memory'] option was removed, but there was missing a proper deprecation, meaning anyone using this in their code had it silently revert to using a file based connection.

This fix restores the original option, but with a deprecation message.

I assume that the target for this module is version 1.3, since 1.2 is SS 2.4 targetted.

simonwelsh commented 10 years ago

I'm not a fan of this behaviour of deprecating something in the same version that its replacement is introduced. Especially since the path cannot be set from _ss_environment.php, but $databaseConfig['memory'] can be.

tractorcow commented 10 years ago

I see, so we should set the deprecation version to 1.4, and leave the current version to 1.3?

The commit message on https://github.com/silverstripe-labs/silverstripe-sqlite3/commit/69fee85469f984c25d2f71db8c5a0904d394ba41#commitcomment-5578370 said that you can set the path in environment with SS_SQLITE_DATABASE_PATH. Is that not correct?

Thanks for the review @simonwelsh

simonwelsh commented 10 years ago

I see, so we should set the deprecation version to 1.4, and leave the current version to 1.3?

Yeah, people need time to actually be able to migrate their code before it starts throwing errors at them.

The commit message on 69fee85#commitcomment-5578370 said that you can set the path in environment with SS_SQLITE_DATABASE_PATH. Is that not correct?

As I said on that comment, that's not guaranteed as SS_SQLITE_DATABASE_PATH is only used inside a _config.php, so can be overwritten by including conf/ConfigureFromEnv.php after the path key gets set.

tractorcow commented 10 years ago

I've updated the notice version to 1.4; Is this ok by you now @simonwelsh?

I did a bit of investigation and yeah, memory is set by SS_DATABASE_MEMORY in ConfigureFromEnv.php; I was looking at sqlite3/_config.php, where the path is assigned with SS_SQLITE_DATABASE_PATH. However, ConfigureFromEnv.php doesn't touch the 'path' $databaseConfig key, so it should be safe from being overwritten.

simonwelsh commented 10 years ago

There's an assignment directly to the variable, so it doesn't matter that it doesn't touch the key specifically: https://github.com/silverstripe/silverstripe-framework/blob/3.1/conf/ConfigureFromEnv.php#L91-L100

tractorcow commented 10 years ago

Well, I'm learning a lot today. Thanks for walking me through this... I appreciate your time, just sorry it took me a while to get my head around it.