Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UpToDateThemes validator to setup:db:status #39383

Open
wants to merge 2 commits into
base: 2.4-develop
Choose a base branch
from

Conversation

peterjaap
Copy link
Contributor

@peterjaap peterjaap commented Nov 19, 2024

Description (*)

Running bin/magento setup:db:status right now does not check whether there is a theme update. If the only change in the deploy is a theme being added, bin/magento setup:db:status will now show;

$ bin/magento setup:db:status
All modules are up to date.

However if you were to run bin/magento setup:upgrade, you'd get this message;

$ bin/magento setup:upgrade
Processing configurations data from configuration file...
The following themes will be registered: frontend/Hyva/Elgentos
Do you want to continue [yes/no]?

With the change in this MR, we'll get this output from setup:db:status;

$ bin/magento setup:db:status
Themes are not up to date
Run 'setup:upgrade' to update your DB schema and data.

Since we rely on the output of setup:db:status to decide whether or not to run setup:upgrade in our deployment pipeline (to make near-zero downtime deployments possible), this is tripping us up from time to time.

This PR aims to add a validator to check whether there are theme changes, forcing setup:upgrade to run.

Thanks to @johnorourke for pointing this out.

Resolved issues:

  1. resolves [Issue] Add UpToDateThemes validator to setup:db:status #39385: Add UpToDateThemes validator to setup:db:status

@m2-github-services m2-github-services added Partner: Elgentos partners-contribution Pull Request is created by Magento Partner labels Nov 19, 2024
Copy link

m2-assistant bot commented Nov 19, 2024

Hi @peterjaap. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@engcom-Charlie
Copy link
Contributor

@magento create issue

@engcom-Charlie engcom-Charlie added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Nov 19, 2024
@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 19, 2024

@peterjaap, am I right that the bin/magento app:config:import command could also fix it? If so, I think it would be more logical to add it to bin/magento app:config:status command

@peterjaap
Copy link
Contributor Author

@ihor-sviziev afaik the app:config:import doesn't create/remove themes

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 19, 2024

@peterjaap, could you then provide more details of what does not check whether there is a theme update means? Are you adding a theme to env.php or the database? And what does the bin/magento setup:upgrade command does in this case?

The bin/magento setup:upgrade actually does a lot of things:

$installer = $this->installerFactory->create(new ConsoleLogger($output));
$installer->updateModulesSequence($keepGenerated);
$searchConfig = $this->searchConfigFactory->create();
$this->cache->clean();
$searchConfig->validateSearchEngine();
$installer->installSchema($request);
$installer->removeUnusedTriggers();
$installer->installDataFixtures($request, true);
if ($this->deploymentConfig->isAvailable()) {
$importConfigCommand = $this->getApplication()->find(ConfigImportCommand::COMMAND_NAME);
$arrayInput = new ArrayInput([]);
$arrayInput->setInteractive($input->isInteractive());
$result = $importConfigCommand->run($arrayInput, $output);
if ($result === Cli::RETURN_FAILURE) {
throw new RuntimeException(
__('%1 failed. See previous output.', ConfigImportCommand::COMMAND_NAME)
);
}
}

And I don't see anything related to theme (maybe it's hidden somewhere)

@peterjaap
Copy link
Contributor Author

@ihor-sviziev it's definitely in there somewhere, given this output;

$ bin/magento setup:upgrade
Processing configurations data from configuration file...
The following themes will be registered: frontend/Hyva/Elgentos
Do you want to continue [yes/no]?

In this case, the new theme is added by adding it to app/design. It could also be added by requiring a composer package, like hyva-themes/magento2-default-theme.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 26, 2024

@peterjaap, from what I see, the message is located in \Magento\Theme\Model\Config\Importer::getWarningMessages, and is used only a single place - \Magento\Deploy\Console\Command\App\ConfigImport\Processor::execute, which relates to php bin/magento app:config:import command. Then, the setup:upgrade command just runs app:config:import command internally.

Could you please double-check that?

Considering that, I think it has to be checked in the php bin/magento app:config:status command (if it's not checked already).

BTW, I guess you're using app:db:status for Zero downtime deployments. If yes - you should check both app:db:status and app:config:status, otherwise you'll see such issues like this

@peterjaap
Copy link
Contributor Author

Yes you are correct. So this might be a better check to add to app:config:status, and I need to add app:config:status to my pre-deploy check, right?

@ihor-sviziev
Copy link
Contributor

@peterjaap, yes, please update the pull request and update your pre-deploy check :)

@peterjaap
Copy link
Contributor Author

Ok thankfully that check is already in there;

https://github.com/deployphp/deployer/blob/725730d7ccf74e2fbbd8b95d33e16cb432ea046d/recipe/magento2.php#L283

So I only have to update this PR then. Coolcoolcool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Partner: Elgentos partners-contribution Pull Request is created by Magento Partner Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: pending review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Issue] Add UpToDateThemes validator to setup:db:status
4 participants