TPROD-182 Dynamic database platform detection #9739

dennisameling · 2021-02-28T20:40:53Z

Q	A
Branch?	features
Bug fix?	yes and no
New feature?	no
Deprecations?	no
BC breaks?	yes
Automated tests included?	no
Related user documentation PR URL	N/A
Related developer documentation PR URL	N/A
Issue(s) addressed	Fixes #...

Description

Mautic currently has a hardcoded server_version config parameter, set to '5.7'. However, this is problematic for a few reasons:

Version 5.7 assumes MySQL, while MariaDB has a different versioning scheme (10.1, 10.2, etc.). Officially, if the server_version is hardcoded and the database type is MariaDB, it needs to be prefixed by mariadb-{VERSION}, so that Doctrine knows it's MariaDB. Mautic currently doesn't do this, so Doctrine always thinks it's interacting with MySQL.
The hardcoded version is written to Mautic's config/local.php file, so without a migration it won't be updated automatically if Mautic's minimal database version requirement changes. This has led to problems before; see Bump minimum MySQL version to 5.7 #9437 as an example.

Since version 2.5, Doctrine has automatic platform version detection built-in. So it will recognize if e.g. the minimum MySQL version is 5.7, required for some functionality that was only added in version 5.7.

As far as I know, there's no need (anymore) for Mautic to hardcode the database server version, hence this PR.

This PR also adds a new tab "Database info" to the "System info" page for easier debugging/troubleshooting:

Why database auto-detection is tricky in the installer

When Mautic isn't installed yet, you'll get an exception in the installer when no explicit server_version is set:

This was already reported by @alanhartless to the Doctrine team in 2015, with no good fix for Mautic's use case to date.

The good news is that we can set a server_version manually in the installer, until the database credentials are set. The order of steps is as follows then:

Mautic sets temporary server_version to 5.7 in the installer
User enters DB config in Mautic's installer UI (db host, name, etc.)
Mautic recognizes that the DB config has been entered and switches over to Doctrine's automatic platform detection
Mautic installs the database schema (installSchema() below)

This order of steps is crucial. During my testing, before I had a fix for this in place, the detected database platform remained MySQL and it stayed like that until I removed the cache. In order to prevent issues in the future, we want step 3 (switch to automatic platform detection before installing the schema) to happen first.

I came up with a somewhat ugly fix for this in the AppKernel by setting a constant called MAUTIC_DB_NOT_CONFIGURED that the installer can recognize. That way it knows exactly when to switch over to automatic database platform detection.

Step 1.0 in the installer (user sets database credentials):

Step 1.1 in the installer (Mautic now switches to automatic database platform detection, prior to installing the schema):

During schema installation, the correct database platform (MariaDB in this case) is indeed detected:

Steps to test this PR:

Load up this PR locally
Ensure Mautic's installer works correctly (ideally try both with MariaDB and MySQL, and both UI + CLI installations)
Check if the "database info" tab works as expected under System info > Database info
Run php bin/console doctrine:schema:update --dump-sql, it should return less than 10 queries to be executed
Try to do a clean installation based on the features branch. Ensure your app/config/local.php has a db_server_version parameter (if it doesn't show up right away, go to Configuration in Mautic's UI and hit Save). Then checkout this PR and run php bin/console doctrine:migrations:execute --up 20210502162314. The db_server_version parameter should now be removed from your app/config/local.php file.

codecov · 2021-05-02T16:31:52Z

Codecov Report

Merging #9739 (49848f0) into features (650acdc) will increase coverage by 0.05%.
The diff coverage is 92.30%.

@@              Coverage Diff               @@
##             features    #9739      +/-   ##
==============================================
+ Coverage       41.31%   41.37%   +0.05%     
- Complexity      34560    34563       +3     
==============================================
  Files            2060     2061       +1     
  Lines          111520   111520              
==============================================
+ Hits            46079    46139      +60     
+ Misses          65441    65381      -60

Impacted Files	Coverage Δ	Complexity Δ
...les/InstallBundle/Controller/InstallController.php	`4.37% <ø> (+0.06%)`	`55.00 <0.00> (ø)`
app/bundles/InstallBundle/Helper/SchemaHelper.php	`0.00% <ø> (ø)`	`42.00 <0.00> (-1.00)`
app/bundles/CoreBundle/Loader/ParameterLoader.php	`94.20% <77.77%> (-1.26%)`	`19.00 <6.00> (+3.00)`	⬇️
...dles/ConfigBundle/Controller/SysinfoController.php	`94.11% <100.00%> (+94.11%)`	`3.00 <0.00> (ø)`
app/bundles/ConfigBundle/Model/SysinfoModel.php	`67.60% <100.00%> (+67.60%)`	`20.00 <2.00> (+2.00)`
...ncyInjection/EnvProcessor/MauticConstProcessor.php	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
...s/CoreBundle/Doctrine/Provider/VersionProvider.php	`100.00% <100.00%> (ø)`	`6.00 <1.00> (-4.00)`
...undle/EventListener/MigrationCommandSubscriber.php	`100.00% <100.00%> (ø)`	`9.00 <0.00> (ø)`
... and 1 more

dennisameling · 2021-05-02T22:43:35Z

This is now ready to test. Just updated the PR description with more details and background on the changes I made. Since this is a BC break, let's try to have it included in the Mautic 4 beta release!

dennisameling · 2021-05-03T08:46:44Z

app/bundles/ConfigBundle/Model/SysinfoModel.php

@@ -59,7 +67,7 @@ public function getPhpInfo()
            return $this->phpInfo;
        }

-        if (function_exists('phpinfo')) {
+        if (function_exists('phpinfo') && 'cli' !== php_sapi_name()) {


This is needed because when PHP is running on the CLI (e.g. during automated tests), phpinfo() doesn't render HTML but regular text + line breaks instead. Then the logic within this if block breaks. This doesn't change any existing behavior for "normal" users, and the formatting is updated in the UI as it was before.

dennisameling · 2021-05-03T08:48:02Z

app/bundles/CoreBundle/Config/config.php

@@ -1177,7 +1177,7 @@
        'db_user'                         => '',
        'db_password'                     => '',
        'db_table_prefix'                 => '',
-        'db_server_version'               => '5.7',
+        'db_server_version'               => defined('MAUTIC_DB_NOT_CONFIGURED') ? '5.7' : null,


This is a crucial part of the logic. When no config/local.php has been created yet, MAUTIC_DB_NOT_CONFIGURED is defined. See PR description for more details.

@alanhartless I might need some help here. As correctly indicated by @RCheesley from her testing, the db_server_version parameter is still being written to config/local.php this way. Earlier on in my journey for this PR, I removed db_server_version from CoreBundle/Config/config.php and added the logic to app/config/config.php, here:

mautic/app/config/config.php

Line 117 in c6b7ae7

'server_version' => '%mautic.db_server_version%',

However that seems to write the value into Symfony's cache, which we also don't want. Because otherwise it remains at 5.7 even after installation, until folks remove their cache. Is there a way to dynamically get the value from db_server_version here (as it does now) while preventing it from being written to config/local.php? That's the final missing piece in this PR then!

Idea: we might be able to leverage MAUTIC_CONFIG_PARAMETERS for this (I couldn't find any documentation about that env variable by the way, but looking at the code we might be able to set/update it to {"db_server_version": "5.7"} in the AppKernel, instead of setting that constant). That way, we can remove db_server_version from CoreBundle/Config/config.php and it won't be stored in config/local.php anymore 🚀

I'll test this theory later today

mautic/app/bundles/CoreBundle/Loader/ParameterLoader.php

Lines 160 to 166 in 7dbb6ee

// Load from environment

$envParameters = getenv('MAUTIC_CONFIG_PARAMETERS');

if ($envParameters) {

$compiledParameters = array_merge($compiledParameters, json_decode($envParameters, true));

}

$this->localParameters = $compiledParameters;

FIXED IN 9f8bc8a 🎉 🚀 CC @RCheesley @alanhartless

dennisameling · 2021-05-03T09:03:21Z

app/bundles/CoreBundle/Loader/ParameterLoader.php

@@ -82,29 +82,35 @@ public function loadIntoEnvironment()
        $dotenv->populate($envVariables->all());
    }

-    public static function getLocalConfigFile(string $root): string
+    public static function getLocalConfigFile(string $root, $updateDefaultParameters = true): string


$updateDefaultParameters is needed because once one or more keys are set in $defaultParameters, the loadDefaultParameters() function doesn't work anymore because it thinks that all default parameters have already been set:

mautic/app/bundles/CoreBundle/Loader/ParameterLoader.php

Lines 110 to 116 in c806115

private function loadDefaultParameters(): void

{

if (self::$defaultParameters) {

// This is loaded within and outside the container so use static variable to prevent recompiling

// multiple times

return;

}

Since we only access getLocalConfigFile() in the AppKernel, and not any other config files or env variables, we want to prevent $defaultParameters from being set incompletely on application boot. Therefore we explicitly set $updateDefaultParameters to false when accessing this function from the AppKernel, since we simply need to get the path to our local config file from there.

I guess the alternative instead of changing this function is to simply look for $root/config/local.php in the AppKernel, but I'm not sure if that reliably covers all possible scenarios for service providers that might use custom paths to their config files?

RCheesley

Load up this PR locally ✅
Ensure Mautic's installer works correctly (ideally try both with MariaDB ✅ and MySQL ✅ , and both UI ✅ + CLI ✅ installations)
Check if the "database info" tab works as expected under System info > Database info ✅ MySQL ✅ MariaDB

Run php bin/console doctrine:schema:update --dump-sql, it should return less than 10 queries to be executed ✅ one query for MariaDB and one for MySQL
Try to do a clean installation based on the features branch. Ensure your app/config/local.php has a db_server_version parameter (if it doesn't show up right away, go to Configuration in Mautic's UI and hit Save) ❓ it has the param but it is 'db_server_version' => null on MySQL and was set to 'db_server_version' => '5.7' on MariaDB - which confused me somewhat!
Then checkout this PR and run php bin/console doctrine:migrations:execute --up 20210502162314. The db_server_version parameter should now be removed from your app/config/local.php file. ✅ MySQL and ✅ MariaDB

So other than the query on point 5 @dennisameling this is an approve - can you clarify?

dennisameling · 2021-05-05T09:54:38Z

@RCheesley the value of db_server_version won't be written to config/local.php anymore now. Could you please re-test point 5 from your review? 😊

mabumusa1

I tested this PR with MariaDB and Mysql, it worked as expected.

Only migration was showing
In the system info I was able to see the database information
local.php was adjusted as mentioned

RCheesley · 2021-05-05T15:12:29Z

@dennisameling just ran through it again - in both cases (MySQL and MariaDB) there is 'db_server_version' => '5.7' showing in the local.php before I apply the PR - is that correct? I think that is what the PR is fixing?

With both cases it is removed after I apply the migration now, the DB info is showing in the system info correctly.

dennisameling · 2021-05-05T15:15:02Z

@RCheesley Yes that's correct - this PR prevents the hardcoded version from being written to the config/local.php file moving forward. Also, when doing a clean installation based on this PR, db_server_version will also not show up anymore. So the PR works both for users that already have db_server_version in their config (which will then be removed) and for clean installations 👍🏼

RCheesley · 2021-05-05T15:22:54Z

Awesome! Just needs a code review then from @mautic/core-team and we are good to go!

dennisameling · 2021-05-06T19:58:09Z

app/bundles/CoreBundle/Config/config.php

@@ -1177,7 +1181,6 @@
        'db_user'                         => '',
        'db_password'                     => '',
        'db_table_prefix'                 => '',
-        'db_server_version'               => '5.7',


This prevents the db_server_version from being written to app/config/local.php moving forward 🚀

dennisameling · 2021-05-06T19:59:04Z

app/bundles/CoreBundle/DependencyInjection/EnvProcessor/MauticConstProcessor.php

+
+use Symfony\Component\DependencyInjection\EnvVarProcessorInterface;
+
+class MauticConstProcessor implements EnvVarProcessorInterface


Needed a custom env var processor because Symfony's built-in const processor didn't seem to work as expected https://symfony.com/doc/current/configuration/env_var_processors.html#built-in-environment-variable-processors

We use this in the code as %env(mauticconst:MAUTIC_DB_SERVER_VERSION)%

alanhartless

I'm not finding anything that jumps out to me as concerning with this. One small note of a newly added variable that I don't see used anywhere so not sure if that is needed. I think it's good to go with tests. I'd expect there is some risk as more environments are tested with the code but we'll have to deal with those as they come.

alanhartless · 2021-05-21T17:01:14Z

app/config/config.php

-$configParameterBag = (new \Mautic\CoreBundle\Loader\ParameterLoader())->getParameterBag();
+$parameterLoader         = new \Mautic\CoreBundle\Loader\ParameterLoader();
+$configParameterBag      = $parameterLoader->getParameterBag();
+$localConfigParameterBag = $parameterLoader->getLocalParameterBag();


Is this used anywhere?

@dennisameling can you follow up on this question at a later date? Thanks!

kuzmany

Works for me 👍 Nice PR
I tested new installation, I tested current installation + migrations.
Without issue.
Let's merge it and try tested in RC

mautibot · 2022-03-01T16:53:53Z

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/out-of-the-blue-dba-exception-mautic-down/22858/7

mautibot · 2023-02-15T19:08:21Z

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/out-of-the-blue-dba-exception-mautic-down/22858/21

galvani · 2023-05-14T17:03:34Z

Hi @RCheesley @dennisameling
sorry for opening this so late but I do not understand it well.
#9739

take
https://github.com/mautic/mautic/blob/4.x/app/AppKernel.php#L47

if (!defined('MAUTIC_DB_SERVER_VERSION')) {
    $localConfigFile = ParameterLoader::getLocalConfigFile($this->getRootDir(), false);
    define('MAUTIC_DB_SERVER_VERSION', file_exists($localConfigFile) ? null : '5.7');
}

evaluates to null which is worse than 5.7.
as this is AppKernel's constructor, the only way to declare this constant is to place it into index.php, which is definitely not a config file.

if you already have a local config file - where you may be willing to declare the constant - you can not.

the value of this constant is used in app/config/config.php

'server_version' => '%env(mauticconst:MAUTIC_DB_SERVER_VERSION)%',

while all other DB related configuration is declared this way:

'driver'                => '%mautic.db_driver%',
    'host'                  => '%mautic.db_host%',
    'port'                  => '%mautic.db_port%',
    'dbname'                => '%mautic.db_name%',
    'user'                  => '%mautic.db_user%',
    'password'              => '%mautic.db_password%',
    'charset'               => 'utf8mb4',

etc.

at my end this forces the platform autodetection which also keeps and extra open connection. that makes on very few requests very much open connections.

am I getting it wrong or will the server_version be overwritten elsewhere?
Any help welcome, thanks

kuzmany · 2023-05-15T06:39:00Z

@galvani @dennisameling not working anymore for Mautic.
From my view

If local.php not exists then MAUTIC_DB_SERVER_VERSION is set to 5.7 - used during the installation
If exists - MAUTIC_DB_SERVER_VERSION is set to null - then connection exist and Doctrine use automatic database detection

dennisameling added enhancement Any improvement to an existing feature or functionality mautic-4 Relates to Mautic 4.x labels Feb 28, 2021

dennisameling added this to the Mautic 4.0 milestone Feb 28, 2021

cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Feb 28, 2021

RCheesley modified the milestones: 4.0-alpha, 4.0-beta Mar 29, 2021

dennisameling force-pushed the dynamic-db-platform-detection branch from 90c8635 to b1fab2d Compare May 2, 2021 16:05

dennisameling added the bc-break A BC break PR for major release milestones only label May 2, 2021

dennisameling force-pushed the dynamic-db-platform-detection branch from b1fab2d to 7d5b456 Compare May 2, 2021 16:16

dennisameling changed the title ~~[WIP] Dynamic database platform detection~~ Dynamic database platform detection May 2, 2021

Dynamic database platform detection

b53317c

dennisameling force-pushed the dynamic-db-platform-detection branch from 4266ac3 to b53317c Compare May 2, 2021 22:19

dennisameling marked this pull request as ready for review May 2, 2021 22:20

dennisameling added the ready-to-test PR's that are ready to test label May 2, 2021

dennisameling requested review from alanhartless and RCheesley May 2, 2021 22:42

dennisameling commented May 3, 2021

View reviewed changes

dennisameling added the T2 Medium difficulty to fix (issue) or test (PR) label May 3, 2021

dennisameling changed the title ~~Dynamic database platform detection~~ TPROD-182 Dynamic database platform detection May 3, 2021

RCheesley reviewed May 4, 2021

View reviewed changes

Fix database detection in installer 🚀😍🤷🏼‍♂✨

9f8bc8a

mabumusa1 self-requested a review May 5, 2021 12:17

mabumusa1 previously approved these changes May 5, 2021

View reviewed changes

RCheesley removed pending-test-confirmation PR's that require one test before they can be merged pending-feedback PR's and issues that are awaiting feedback from the author labels May 5, 2021

RCheesley requested a review from a team May 6, 2021 10:23

dennisameling commented May 6, 2021

View reviewed changes

Merge branch 'features' into dynamic-db-platform-detection

d4fe508

npracht modified the milestones: 4.0-beta, 4.0-rc May 14, 2021

Merge branch 'features' into dynamic-db-platform-detection

3af8c6d

RCheesley dismissed mabumusa1’s stale review via 3af8c6d May 15, 2021 12:00

alanhartless approved these changes May 21, 2021

View reviewed changes

kuzmany approved these changes May 24, 2021

View reviewed changes

Merge branch 'features' into dynamic-db-platform-detection

49848f0

RCheesley merged commit de7e701 into mautic:features May 24, 2021

RCheesley modified the milestones: 4.0-rc, 4.0-beta May 25, 2021

RCheesley mentioned this pull request Jun 24, 2021

Fix Installer Exception Handling - TPROD-194 #9934

Merged

RCheesley mentioned this pull request Jul 5, 2021

Error thrown when pushing translations to Transifex #10230

Closed

kuzmany mentioned this pull request Feb 16, 2022

Unit tests: fix Unknown table 'COLUMN_STATISTICS' in information_schema #10869

Closed

escopecz mentioned this pull request Jan 10, 2024

Mautic 5.0.0 installation issue #13186

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TPROD-182 Dynamic database platform detection #9739

TPROD-182 Dynamic database platform detection #9739

dennisameling commented Feb 28, 2021 •

edited

Loading

codecov bot commented May 2, 2021 •

edited

Loading

dennisameling commented May 2, 2021

dennisameling May 3, 2021

dennisameling May 3, 2021

dennisameling May 4, 2021 •

edited

Loading

dennisameling May 5, 2021

dennisameling May 5, 2021

dennisameling May 3, 2021 •

edited

Loading

RCheesley left a comment

dennisameling commented May 5, 2021

mabumusa1 left a comment

RCheesley commented May 5, 2021

dennisameling commented May 5, 2021

RCheesley commented May 5, 2021

dennisameling May 6, 2021

dennisameling May 6, 2021 •

edited

Loading

alanhartless left a comment

alanhartless May 21, 2021

RCheesley May 24, 2021

kuzmany left a comment

mautibot commented Mar 1, 2022

mautibot commented Feb 15, 2023

galvani commented May 14, 2023

kuzmany commented May 15, 2023

	// Load from environment
	$envParameters = getenv('MAUTIC_CONFIG_PARAMETERS');
	if ($envParameters) {
	$compiledParameters = array_merge($compiledParameters, json_decode($envParameters, true));
	}

	$this->localParameters = $compiledParameters;

	private function loadDefaultParameters(): void
	{
	if (self::$defaultParameters) {
	// This is loaded within and outside the container so use static variable to prevent recompiling
	// multiple times
	return;
	}


		use Symfony\Component\DependencyInjection\EnvVarProcessorInterface;

		class MauticConstProcessor implements EnvVarProcessorInterface

TPROD-182 Dynamic database platform detection #9739

TPROD-182 Dynamic database platform detection #9739

Conversation

dennisameling commented Feb 28, 2021 • edited Loading

Description

Why database auto-detection is tricky in the installer

Steps to test this PR:

codecov bot commented May 2, 2021 • edited Loading

Codecov Report

dennisameling commented May 2, 2021

dennisameling May 3, 2021

Choose a reason for hiding this comment

dennisameling May 3, 2021

Choose a reason for hiding this comment

dennisameling May 4, 2021 • edited Loading

Choose a reason for hiding this comment

dennisameling May 5, 2021

Choose a reason for hiding this comment

dennisameling May 5, 2021

Choose a reason for hiding this comment

dennisameling May 3, 2021 • edited Loading

Choose a reason for hiding this comment

RCheesley left a comment

Choose a reason for hiding this comment

dennisameling commented May 5, 2021

mabumusa1 left a comment

Choose a reason for hiding this comment

RCheesley commented May 5, 2021

dennisameling commented May 5, 2021

RCheesley commented May 5, 2021

dennisameling May 6, 2021

Choose a reason for hiding this comment

dennisameling May 6, 2021 • edited Loading

Choose a reason for hiding this comment

alanhartless left a comment

Choose a reason for hiding this comment

alanhartless May 21, 2021

Choose a reason for hiding this comment

RCheesley May 24, 2021

Choose a reason for hiding this comment

kuzmany left a comment

Choose a reason for hiding this comment

mautibot commented Mar 1, 2022

mautibot commented Feb 15, 2023

galvani commented May 14, 2023

kuzmany commented May 15, 2023

dennisameling commented Feb 28, 2021 •

edited

Loading

codecov bot commented May 2, 2021 •

edited

Loading

dennisameling May 4, 2021 •

edited

Loading

dennisameling May 3, 2021 •

edited

Loading

dennisameling May 6, 2021 •

edited

Loading