Magento 2: Changed upgrading database

[dverkade]

• Bug fix Magento 2 Recipe: Use roles to execute DB tasks only on one server #3800
• New feature
• BC breaks?
• Tests added?
• Docs added?

This PR addresses two issues:

• Currently the Magento 2 recipe runs the following command to update the database setup:upgrade however this command does more then only upgrading the database. It can change the sort order of the modules listed in app/etc/config.php or if a module is not listed in that file it will enable that module. IMHO deployments should be consistent, and the app/etc/config.php shouldn't be changed during deployment. That's why I changed this into two commands that actually only update the DB schema and DB data. I've borrowed this from the Magento 2 Capistrano implementation, which already does it like this: https://github.com/davidalger/capistrano-magento2/blob/master/lib/capistrano/tasks/magento.rake#L261
• I've also fixed issue Magento 2 Recipe: Use roles to execute DB tasks only on one server #3800 by only running the DB commands once on the selected servers. This will fix the issue of multiple DB upgrades which are executed on every node instead of only one node.

[peterjaap]

Thanks for the PR!

Changing setup:upgrade to the two separate upgrade commands is a great idea.

However, I don't see this working;

run("{{bin/php}} {{bin/magento}} setup:db-data:upgrade --no-interaction")->once();

Afaik run() returns a string (the output of the command) and not an object to set once() on. This should be wrapped in a separate task(), which can take once(). Right, @antonmedv ?

As a sidenote, we ran into accidentally enabling non-listed modules error with setup:upgrade before. To avoid this, we created a small PHP tool to check this before the deployment task starts in our pipeline;

<?php

$output = [];
exec('php bin/magento module:status --disabled', $output, $exitCode);

// If No disabled modules, exit with exit code 1
if ($exitCode === 1) {
    exit(0);
}

$config = include 'app/etc/config.php';

$whitelistedDisabledModules = $config['whitelisted_disabled_modules'] ?? [];
$messages = [];

foreach ($output as $module) {
    if (!in_array($module, $whitelistedDisabledModules)) {
        $messages[] = $module . ' is disabled in modules but not whitelisted under whitelisted_disabled_modules in app/etc/config.php.';
    }
}

if (!empty($messages)) {
    foreach ($messages as $message) {
        echo $message . PHP_EOL;
    }
    exit(1);
} else {
    exit(0);
}

Maybe you'll find it useful too.

[antonmedv]

Yeap. Once should be applied to tasks

[peterjaap]

Perfect!

[peterjaap]

@dverkade I just had time to take a deeper look at this, and I have some concerns.

The installSchema is now ran from setup:db-schema-upgrade.

The installDataFixtures is now ran from setup:db-data:upgrade.

So far so good.

However, now $installer->removeUnusedTriggers();` seems not to be triggered.

Also, app:config:import is not ran at this point. Won't this cause issues when people put configuration settings in app/etc/config.php? The Capistrano task runs this separately, see here.

I feel like we should add app:config:import in a separate task. That does leave the removal of the unused triggers..

[dverkade]

@peterjaap I've checked and the app:config:import command is already triggered before the db:upgrade here. So no issue there.

Regarding the removeUnusedTriggers() function. That's interesting and IMHO is an issue of Magento itself. Shouldn't that be part of the setup:db-schema:upgrade command too as this will make sure the db-schema is up to date. I've never had issues with this setup (and that particular function not being run) while using Capistrano which we have used for years before moving to Deployer.

[peterjaap]

@dverkade I guess it won't immediately introduce issues when the unused triggers aren't removed, it's more a part of good database hygiene. Weirdly enough, that removeUnusedTriggers() call is the ONLY call in the whole of Magento where it's run.

[peterjaap]

It also looks like that the removeUnusedTriggers() call also CREATED triggers <2.4.6, which could indeed cause issues down the line; magento/magento2#33386 (comment)

[peterjaap]

The nicest solution would be that core Magento will contain a console command to delete the unused triggers, so we can call that too. Or that it is behind a flag in the setup:db-schema:upgrade command.

[rickvb97-io]

Hi guys, we recently upgraded our deployer package from v4.7.0 to v7.5.5 and noticed that this change breaks deployments (to production) if dev modules are committed in the app/etc/config.php file. Due to this change, we had to rollback the module update. Is this something that can be fixed?

[peterjaap]

@rickvb97-io why would there be dev modules in an config.php that goes to production?

[dverkade]

@rickvb97-io your issue is unrelated to this pull request. If you have modules in the dev section of your composer.json they should not be in your app/etc/config.php file as PHP Deployer will only install non dev modules when deploying (to production). So this will cause your module not to be available in Magento and cause issues that the enabled module in app/etc/config.php can't be found.

[rickvb97-io]

@peterjaap Because I don't think there is currently no option to exclude dev modules on a production environment in the config.php. Symlinking the config.php isn't an option, because new modules aren't added.

Not committing the dev modules in the config.php is inconvenient and only causes hassle among (our) developers.

If you have an idea, I would like to hear it.

[peterjaap]

@rickvb97-io we use a system where we symlink the config.php in the repository to config.production.php and use a symlink locally to config.development.php. Both are committed to the Git repo.

[dverkade]

@rickvb97-io @peterjaap As mentioned in my earlier comment, this is not an issue related to this PR. IMHO this should not be discussed here and if you want to continue the conversation you should open up a separate issue for it.

[peterjaap]

Agreed