Skip to content

Conversation

@ivanko-dev
Copy link
Contributor

Description (*)

Added checking if db table exists before fetch config data from db table. This fix for issue in case db data defined in configuration but required tables not exists in db.

Fixed Issues (if relevant)

  1. Since 2.3.2, we can no longer switch maintenance modes with an empty database #23577: Since 2.3.2, we can no longer switch maintenance modes with an empty database

Manual testing scenarios (*)

  1. Clone clean 2.3-develop branch
  2. Install composer packages (composer install)
  3. Install Magento application (bin/magento setup:install {config})
  4. Remove all tables from DB to get empty DB (drop database and create new database with same name)
  5. Try to enable maintenance mode: bin/magento maintenance:enable

Questions or comments

This issue appear in case when env.php file exists and contain credentials for db connection but db has no required tables to fetch config data used on deployment steps. Current fix contain update for checking if table exists before fetch data in \Magento\Store\App\Config\Source\RuntimeConfigSource::getEntities (app/code/Magento/Store/App/Config/Source/RuntimeConfigSource.php: Line 84).
Code execution for enabling maintenance mode have validation if db available several times but this validation checks if db credentials available on config and has no checking if db tables available: \Magento\Framework\App\DeploymentConfig::isDbAvailable and \Magento\Store\App\Config\Source\RuntimeConfigSource::canUseDatabase. Maybe would be good to make checking if required to fetch config data db tables exist in \Magento\Framework\App\DeploymentConfig::isDbAvailable to get more global solution in this case.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@ivanko-dev ivanko-dev requested a review from akaplya as a code owner October 10, 2019 09:43
@m2-assistant
Copy link

m2-assistant bot commented Oct 10, 2019

Hi @ivan-koliadynskyy. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

Update to PHPUnit test scenrio (some methods launched by condition after update).
@ghost ghost assigned aleron75 Oct 11, 2019
@aleron75 aleron75 added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Oct 11, 2019
@magento-engcom-team
Copy link
Contributor

Hi @aleron75, thank you for the review.
ENGCOM-6059 has been created to process this Pull Request

@engcom-Delta
Copy link
Contributor

Hi @ivan-koliadynskyy I still can reproduce issue on PR branch according steps from Manual testing scenarios .
#24959PRissue

Could you take a look?

@ivanko-dev
Copy link
Contributor Author

Hi @ivan-koliadynskyy I still can reproduce issue on PR branch according steps from Manual testing scenarios .
#24959PRissue

Could you take a look?

Hi @engcom-Delta , thanks for information about testing. Checking it now.

@ivanko-dev
Copy link
Contributor Author

Updates to fix this issue done in \Magento\Store\Model\ResourceModel\Store and \Magento\Store\Model\ResourceModel\Website. I see that these files are not covered with unit test. Maybe would be good to prepare unit test for these files?

@ivanko-dev ivanko-dev requested a review from aleron75 October 11, 2019 21:46
@aleron75
Copy link
Contributor

Maybe would be good to prepare unit test for these files?

it would be great if you manage to do that

@ivanko-dev
Copy link
Contributor Author

Maybe would be good to prepare unit test for these files?

it would be great if you manage to do that

New tests for \Magento\Store\Model\ResourceModel\Store and \Magento\Store\Model\ResourceModel\Website created. Updates included in this pull request. Could you please review these updates and let me know if changes required.
Thanks

@magento-engcom-team
Copy link
Contributor

Hi @aleron75, thank you for the review.
ENGCOM-6059 has been created to process this Pull Request

@engcom-Delta
Copy link
Contributor

✔️ QA passed

@m2-assistant
Copy link

m2-assistant bot commented Oct 18, 2019

Hi @ivan-koliadynskyy, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@devchris79
Copy link

@ivan-koliadynskyy This commit seems to be getting the blame for slow page loads (3x slower) from a couple of users in this issue #26692 have you got any thoughts on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants