Skip to content

Keep maintenance mode on if it was previously enabled #11052

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

Merged
merged 5 commits into from
Dec 1, 2017

Conversation

jokeputs
Copy link
Contributor

@jokeputs jokeputs commented Sep 26, 2017

Description

When enabling maintenance mode it will get disabled after using one of the following commands:

  • bin\magento module:uninstall
  • bin\magento setup:backup
  • bin\magento setup:rollback
  • bin\magento theme:uninstall
  • bin\magento deploy:mode:set production

The changes in this PR will first check or maintenance mode was already enabled. If so, it will skip disabling maintenance mode after executing the command.

Fixed Issues

  1. Magento 2 automatically disables maintenance mode after certain actions #9918: Magento 2 automatically disables maintenance mode after certain actions

Manual testing scenarios

  1. Enable maintenance mode: bin\magento maintenance:enable
  2. Create a backup: bin/magento setup:backup --media
  3. Confirm that maintenance mode is still enabled: bin\magento maintenance:status

The output will be for example:

Maintenance mode already enabled
Media backup is starting...
Media backup filename: 1506422834_filesystem_media.tgz (The archive can be uncompressed with 7-Zip on Windows systems)
Media backup path: /var/www/var/backups/1506422834_filesystem_media.tgz
[SUCCESS]: Media backup completed successfully.
Skipped disabling maintenance mode

* @param OutputInterface $output
* @return void
*/
private function enableMaintenanceMode(OutputInterface $output)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code duplication can be avoided by extracting this behavior to a separate object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you do this? Create something like a MaintenanceModeManagement? Or would you add this feature to Magento\Framework\App\MaintenanceMode? I considered that last one but I don't think MaintenanceMode should be responsible for knowing or it should disable maintenance mode.
Do you already have something in mind?

@jokeputs
Copy link
Contributor Author

jokeputs commented Sep 26, 2017

@antonkril, I've updated the pull request. All logic was moved to Magento\Framework\App\Console\MaintenanceModeEnabler.

@vrann vrann self-assigned this Sep 26, 2017
@vrann vrann added this to the September 2017 milestone Sep 26, 2017
@vrann vrann reopened this Sep 29, 2017
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Sep 29, 2017

CLA assistant check
All committers have signed the CLA.

@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
@magento-engcom-team magento-engcom-team added 2.2.x bugfix Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 7, 2017
Copy link

@vkublytskyi vkublytskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jokeputs Changing of constructor arguments is a backward incompatible change and should be avoided. See inline comment how it may be resolved.

Also, some little code duplication is still present as for each command we need to enable and disable mode.
In general, Magento does not allow introduce new public methods in patch releases. But in this case, I suppose this is ok so I propose to add new method to
Magento\Framework\App\MaintenanceMode:

public function executeInMaintainanceMode(callable $task)
{
    // check current maintenance mode
    // switch on maintenance mode if not yet enabled
    call_user_func($task);
    // restore initial mode if needed
}

Such approach will reduce code duplication to minimum an will not lead to a lot of code changes and constructor arguments (only wrapping some part of logic in anonymous function is required)

@@ -83,7 +83,7 @@ class Mode
* @param OutputInterface $output
* @param Writer $writer
* @param Reader $reader
* @param MaintenanceMode $maintenanceMode
* @param MaintenanceModeEnabler $maintenanceMode

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such change is backward incompatible and not allowed for a patch releases. You should deprecate old constructor parameter and add new one.

* @param OutputInterface $output
* @return void
*/
private function enableMaintenanceMode(OutputInterface $output)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this function private as it changes internal state. So class interfaces with enable/disable methods creates temporal coupling in a code.

Class MaintenanceModeEnabler is preserved in a code as it would be wrong to introduce dependency on OutputInterface in MaintenanceMode (initial recommendation).

@vkublytskyi vkublytskyi force-pushed the keep-maintenance-enabled branch from 6b05533 to f3f36c5 Compare November 30, 2017 16:04
@vkublytskyi vkublytskyi force-pushed the keep-maintenance-enabled branch from f3f36c5 to fa5c45d Compare November 30, 2017 16:11
@vkublytskyi vkublytskyi force-pushed the keep-maintenance-enabled branch from fa5c45d to f71efe0 Compare November 30, 2017 16:25
…agento#11052

 - revert backward incompatible changes
 - fix temporal coupling in \Magento\Framework\App\Console\MaintenanceModeEnabler
 - cover maintenance mode with unit test
 - revert changes in unit test to ensure code have same behavior after refactoring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Progress: accept Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants