Fix Installer Exception Handling - TPROD-194 #9934

mabumusa1 · 2021-04-25T12:13:27Z

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

Description:

In 4.0alpha, going through the first step of the installer to configure and install the schema fails if using a db table prefix. It works fine without one but one throws an exception like An error occurred while attempting to add default data: Table db_leads does not exist!

To reproduce:

Go through the UI installer (delete or rename app/config/local.php)
Enter the database credentials with a db table prefix and proceed to the next step
Note the error
Repeat without a prefix and you'll be able to complete the installation
Update
@alanhartless wrote some patches that fix the issues on the UI.
The bug exists on the CLI part of the installer, To reproduce the issue
Make sure the cache folder is empty.
Make sure local.php does not exist.
Make sure the database is empty.
Run the command to install Mautic php bin/console mautic:install --db_port 3306 --db_host db --db_name db --db_user db --db_password db --db_table_prefix ma_ https://mautic.ddev.site/ --env=dev
Notice that the prefix is not honored

Run this script to reproduce the issue and for quick testing

#!/bin/bash         
mysql -udb -pdb -e "drop database db; create database db"
rm -rf /var/www/html/var/cache/*
rm /var/www/html/app/config/local.php
php bin/console mautic:install --db_port 3306 --db_host db --db_name db --db_user db --db_password db --db_table_prefix ma_ https://mautic.ddev.site/ --env=dev --admin_firstname=admin --admin_lastname=admin --admin_username=user --admin_email=email@domainc.com --admin_password=password

Steps to test this PR:

Load up this PR
Try to install with wrong DB settings, no exception should show
Add a prefix, database tables should have a prefix

codecov · 2021-04-25T12:23:27Z

Codecov Report

Merging #9934 (569a0be) into features (a30175e) will increase coverage by 0.02%.
The diff coverage is 45.05%.

@@              Coverage Diff               @@
##             features    #9934      +/-   ##
==============================================
+ Coverage       41.88%   41.91%   +0.02%     
+ Complexity      34518    34495      -23     
==============================================
  Files            2056     2059       +3     
  Lines          111361   111390      +29     
==============================================
+ Hits            46644    46685      +41     
+ Misses          64717    64705      -12

Impacted Files	Coverage Δ
...reBundle/Doctrine/Connection/ConnectionWrapper.php	`0.00% <0.00%> (ø)`
...les/InstallBundle/Controller/InstallController.php	`4.44% <4.16%> (+0.06%)`	⬆️
...ependencyInjection/Compiler/InstallCommandPass.php	`15.38% <15.38%> (ø)`
...p/bundles/InstallBundle/Install/InstallService.php	`32.87% <40.27%> (+5.56%)`	⬆️
...p/bundles/InstallBundle/Command/InstallCommand.php	`83.23% <95.55%> (+0.56%)`	⬆️
...nstallBundle/InstallFixtures/ORM/LeadFieldData.php	`76.66% <100.00%> (ø)`
app/bundles/InstallBundle/MauticInstallBundle.php	`100.00% <100.00%> (ø)`
... and 1 more

alanhartless

I'm still getting the same error when I try installing using a database table prefix with this PR.

An error occurred while attempting to add default data: Table mautic_leads does not exist!

alanhartless · 2021-04-25T15:15:09Z

app/bundles/InstallBundle/Install/InstallService.php

@@ -308,19 +308,20 @@ public function createDatabaseStep($step, $dbParams)
    {
        $translator = $this->translator;

-        $messages = $this->validateDatabaseParams($dbParams);
+        $status   = $this->validateDatabaseParams($dbParams);


validateDatabaseParams() is returning an array of validation errors as well which would be ignored with this change.

alanhartless · 2021-04-25T19:11:02Z

Hmmm, this is odd. It seems to work when I don't have xdebug on but does not when I have it off. Maybe a timing issue where the slightest delay in processing allows it to work in my Docker environment. Still doing some digging.

alanhartless · 2021-04-25T19:45:05Z

Ok, it's something to do with Symfony's cache and Doctrine's EntityManager (the reason xdebug affected it above is that I have two containers, one with xdebug and one without, so each had their own compiled cache and thus not related directly to the issue). Deleting app/config/local.php and nuking Symfony's cache will replicate the issue and I've been able to track it down to

mautic/app/bundles/InstallBundle/Helper/SchemaHelper.php

Line 162 in 0f6bbbb

$metadatas = $this->em->getMetadataFactory()->getAllMetadata();

does not include the table prefix in the table names for the metadata if going through a clean install. If app/config/local.php exists with the prefix defined, deleting the cache, and going through the installer works.

alanhartless · 2021-04-25T19:53:41Z

Doctrine is caching the table names in /prod/doctrine/orm/default_metadata.php and the file is only created when the cache is warmed up (DoctrineMetadataCacheWarmer) so we can't just delete the one file or lose it's benefits. I'd advocate that we remove support for naming a prefix through the UI installer but Mautic and the mautic:install command can continue to support one.

mabumusa1 · 2021-05-05T12:28:57Z

@alanhartless How can I support pushing this PR forward?

RCheesley · 2021-05-05T12:31:22Z

@mabumusa1 we had a discussion on this topic in Slack: https://mautic.slack.com/archives/C01KB7GQFCL/p1619382546118000

alanhartless · 2021-05-12T14:53:49Z

@mabumusa1 would you like help implementing as discussed in the slack channel?

RCheesley · 2021-08-10T13:18:19Z

@mabumusa1 @luguenth we seem to have a big load of tests failing on this PR now - can you please take a look?

dennisameling · 2021-08-20T15:56:50Z

OK I was able to reproduce the issue locally where tests fail if Mautic is not installed. It tries to redirect to /index.php/installer which is an indication that Mautic thinks it's not installed yet. After installing Mautic locally, the tests pass. The CI pipeline has an "Install Mautic" step so something must be going wrong there, because when running the tests, which comes right after that, Mautic still thinks that it's not installed yet. One possible cause I could think of is that this PR introduced a bug where it doesn't respect the db_name/db_user/etc. that we set in the CI pipeline, and installs into a different database instead (or something similar to this).

I'll have a closer look tomorrow by adding tmate to the CI pipeline, so I can log into the database and see in which database Mautic was actually installed

dennisameling · 2021-08-21T19:10:05Z

Created #10350 as a successor to this PR - it's up to date with the latest changes from the features branch and the tests are passing 🎉 therefore closing this one

…rs (mautic#10303) * button lable in all Translation language folders * de folder change translation for button * Update plugins/GrapesJsBuilderBundle/Translations/cs/javascript.ini Correct translation for form button label Co-authored-by: John Linhart <jan@linhart.email> Co-authored-by: John Linhart <jan@linhart.email>

* docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

…db_config_TPROD-194

There were some cases (e.g. when the user doesn't provide a table prefix when installing Mautic) where the installer would throw a warning and simply stop. This commit provides some default parameters for the database, enables PHP strict types and improves some parts of the logic.

dennisameling · 2021-08-23T08:39:17Z

For info - the tests in this PR were failing because of define('MAUTIC_INSTALLER', 1) - the thing here is that the constant is defined once during a PHPUnit test run, but then remains active in the rest of the tests as well. That's causing other, unrelated tests to fail as the logic thinks Mautic still has to be installed. ed82bfe prevents that by only defining MAUTIC_INSTALLER when we're not running tests (IS_PHPUNIT). CC @luguenth

Reopened this PR in favor of #10350

dennisameling · 2021-08-23T08:56:27Z

This PR can now be tested again. Please make sure to test this in the following 4 scenarios:

CLI installation with a DB prefix
CLI installation without a DB prefix
UI installation with a DB prefix
UI installation without a DB prefix

To clean things up after testing each scenario, you can run ddev delete --omit-snapshot && rm app/config/local.php && rm -rf var/cache && ddev start to start with a clean state.

For CLI installation, simply run the DDEV command below:

ddev exec bin/console mautic:install --mailer_from_name="DDEV" --mailer_from_email="mautic@ddev.local" --mailer_transport="smtp" --mailer_host="localhost" --mailer_port="1025" --db_driver="pdo_mysql" --db_host="db" --db_port="3306" --db_name="db" --db_user="db" --db_password="db" --db_table_prefix="mau_" --db_backup_tables="false" --admin_email="admin@mautic.local" --admin_password="mautic" https://mautic.ddev.site --env=dev

For CLI installation without a prefix, simply leave out --db_table_prefix, so that it looks as follows:

ddev exec bin/console mautic:install --mailer_from_name="DDEV" --mailer_from_email="mautic@ddev.local" --mailer_transport="smtp" --mailer_host="localhost" --mailer_port="1025" --db_driver="pdo_mysql" --db_host="db" --db_port="3306" --db_name="db" --db_user="db" --db_password="db" --db_backup_tables="false" --admin_email="admin@mautic.local" --admin_password="mautic" https://mautic.ddev.site --env=dev

mabumusa1 · 2021-08-23T09:34:59Z

I just tested the new fixes, it works as expected. Thanks @dennisameling

RCheesley

All four scenarios are now working flawlessly, a huuuuge thank you to everybody who has worked on getting this PR over the line!

Let's go! 🚀

escopecz · 2022-04-06T07:39:28Z

app/bundles/InstallBundle/DependencyInjection/Compiler/InstallCommandPass.php

+        $definition->addOption(
+            new InputOption(
+                '--env',
+                '-e',
+                InputOption::VALUE_REQUIRED,
+                'The Environment name.',
+                $container->getParameter('kernel.environment')
+            )
+        );


@mabumusa1 what was the reason the add the options here and not in the InstallCommand itself?

@escopecz I think we needed this to be injected before the cache warmup, otherwise, it will not be collected if inserted in the InstallCommand. I am not 100% sure. but I think this is the reason

Ok. That's one of the reasons why we need to cover the code with tests. A test would show us the need for it.

As the InstallCommand must have dependencies for Symfony 5 I had to simplify the code. I tested it by clearing the cache and then running the install command with a table prefix param and it is present in the compiler pass. See 3cabc82

I totally agree, this PR was not fully covered. Once I am done with the email PR I will work on increasing the coverage for this command

mabumusa1 added T1 Low difficulty to fix (issue) or test (PR) bug Issues or PR's relating to bugs regression A bug that broke something in the last release installation Anything related to the installation of Mautic labels Apr 25, 2021

mabumusa1 added this to the 4.0-beta milestone Apr 25, 2021

mabumusa1 requested review from alanhartless and RCheesley April 25, 2021 12:13

mabumusa1 self-assigned this Apr 25, 2021

cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Apr 25, 2021

alanhartless suggested changes Apr 25, 2021

View reviewed changes

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

mabumusa1 and others added 8 commits May 26, 2021 18:18

fix installer expection handling

d3b5d94

increase codcov

dd79e32

Cleaned up the code a bit to not return multiple types (bool or array)

b4eee35

Some more code cleanup

39092a5

Remove unused use statements

5a2323b

Fixed step typecasting and PHP math

25b2ca7

Removed a typecasting that's not required

6344cf0

Fixed cs

df8000e

mabumusa1 force-pushed the fix_installaer_db_config_TPROD-194 branch from e9196ca to df8000e Compare May 26, 2021 18:22

mabumusa1 added 2 commits May 26, 2021 22:06

rebase to features

e81e0ce

fix to PHPSTAN

83ce61e

Merge branch 'features' into fix_installaer_db_config_TPROD-194

34aa47e

RCheesley closed this Aug 12, 2021

RCheesley reopened this Aug 12, 2021

dennisameling dismissed n-azimy’s stale review via 48ba6d4 August 21, 2021 15:57

dennisameling mentioned this pull request Aug 21, 2021

Fix Installer Exception Handling v2 - TPROD-194 #10350

Closed

dennisameling closed this Aug 21, 2021

dennisameling reopened this Aug 23, 2021

disha1611 and others added 3 commits August 23, 2021 10:34

docs: add disha-pishavadia24 as a contributor for code (mautic#10324)

4ec5d3d

* docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

docs: add adiux as a contributor for review (mautic#10325)

fbb6647

* docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

dennisameling force-pushed the fix_installaer_db_config_TPROD-194 branch from 48ba6d4 to fbb6647 Compare August 23, 2021 08:35

dennisameling added 3 commits August 23, 2021 10:35

Merge remote-tracking branch 'upstream/features' into fix_installaer_…

ea11784

…db_config_TPROD-194

Don't define MAUTIC_INSTALLER when running in PHPUnit

ed82bfe

dennisameling added ready-to-test PR's that are ready to test and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Aug 23, 2021

RCheesley approved these changes Aug 23, 2021

View reviewed changes

Merge branch 'features' into fix_installaer_db_config_TPROD-194

569a0be

RCheesley merged commit c6fff63 into mautic:features Aug 23, 2021

This was referenced Sep 27, 2021

Exception handling in UI installer (database step) broken #10480

Closed

Fix Doctrine metadata cache handling during installation #10481

Merged

mabumusa1 deleted the fix_installaer_db_config_TPROD-194 branch February 13, 2022 12:50

escopecz reviewed Apr 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Installer Exception Handling - TPROD-194 #9934

Fix Installer Exception Handling - TPROD-194 #9934

mabumusa1 commented Apr 25, 2021 •

edited

codecov bot commented Apr 25, 2021 •

edited

alanhartless left a comment

alanhartless Apr 25, 2021

alanhartless commented Apr 25, 2021

alanhartless commented Apr 25, 2021 •

edited

alanhartless commented Apr 25, 2021 •

edited

mabumusa1 commented May 5, 2021 •

edited

RCheesley commented May 5, 2021

alanhartless commented May 12, 2021

RCheesley commented Aug 10, 2021

dennisameling commented Aug 20, 2021

dennisameling commented Aug 21, 2021

dennisameling commented Aug 23, 2021 •

edited

dennisameling commented Aug 23, 2021

mabumusa1 commented Aug 23, 2021

RCheesley left a comment

escopecz Apr 6, 2022

mabumusa1 Apr 6, 2022

escopecz Apr 6, 2022

mabumusa1 Apr 6, 2022

Fix Installer Exception Handling - TPROD-194 #9934

Fix Installer Exception Handling - TPROD-194 #9934

Conversation

mabumusa1 commented Apr 25, 2021 • edited

Description:

Steps to test this PR:

codecov bot commented Apr 25, 2021 • edited

Codecov Report

alanhartless left a comment

Choose a reason for hiding this comment

alanhartless Apr 25, 2021

Choose a reason for hiding this comment

alanhartless commented Apr 25, 2021

alanhartless commented Apr 25, 2021 • edited

alanhartless commented Apr 25, 2021 • edited

mabumusa1 commented May 5, 2021 • edited

RCheesley commented May 5, 2021

alanhartless commented May 12, 2021

RCheesley commented Aug 10, 2021

dennisameling commented Aug 20, 2021

dennisameling commented Aug 21, 2021

dennisameling commented Aug 23, 2021 • edited

dennisameling commented Aug 23, 2021

mabumusa1 commented Aug 23, 2021

RCheesley left a comment

Choose a reason for hiding this comment

escopecz Apr 6, 2022

Choose a reason for hiding this comment

mabumusa1 Apr 6, 2022

Choose a reason for hiding this comment

escopecz Apr 6, 2022

Choose a reason for hiding this comment

mabumusa1 Apr 6, 2022

Choose a reason for hiding this comment

mabumusa1 commented Apr 25, 2021 •

edited

codecov bot commented Apr 25, 2021 •

edited

alanhartless commented Apr 25, 2021 •

edited

alanhartless commented Apr 25, 2021 •

edited

mabumusa1 commented May 5, 2021 •

edited

dennisameling commented Aug 23, 2021 •

edited