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

Don't disable FPC for maintenance, instead send "no cache" headers #25790

Merged

Conversation

theCapypara
Copy link
Member

@theCapypara theCapypara commented Nov 28, 2019

Description (*)

This Pull Request removes the need to have a writable env.php while switching the maintenance mode. Previously Magento tried to disable the FPC when switching into maintenance mode. This removes that and instead, to maintain functionality, sets the "no-cache" headers for all frontend requests in maintenance mode (Cache-Control etc.). The effect is, that pages are not cached while in maintenance mode.

  • Removes the observer for disabling and re-enabling the FPC during
    maintenande mode switch

  • Adds a new observer, that adds the "no cache" headers for all HTTP
    requests during maintenance mode

Fixed Issues (if relevant)

  1. Unable to enable maintenance mode when env.php is read only #24229: Unable to enable maintenance mode when env.php is read only

Manual testing scenarios (*)

With Builtin FPC:

  1. Keep maintenance mode disabled, access a cacheable Magento page
    • Page should be cached and delivered by FPC on next request
  2. Make the app/etc/env.php file read-only
  3. Enable maintenance mode
    • This should now not fail, previously it complained about the env.php not being writable
  4. Add the local IP to the maintenance whitelist
  5. Flush the FPC
  6. Access a cacheable Magento page
    • Page should not be cached
  7. Disable maintenance again and repeat 1.

Repeat tests for Varnish as FPC.

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)

@m2-assistant
Copy link

m2-assistant bot commented Nov 28, 2019

Hi @Parakoopa. 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.

@theCapypara theCapypara changed the title Don't disable FPC for maintenance, instead send no cache headers Don't disable FPC for maintenance, instead send "no cache" headers Nov 28, 2019
@theCapypara theCapypara force-pushed the maintenance-mode-fpc branch 2 times, most recently from a134587 to 72c0e05 Compare November 28, 2019 13:09
@sivaschenko
Copy link
Member

@magento run Unit Tests

@theCapypara
Copy link
Member Author

After a recommendation by @sivaschenko, I changed it so, that public headers aren't even set in maintenance mode.

@ihor-sviziev ihor-sviziev self-assigned this Nov 29, 2019
@ihor-sviziev ihor-sviziev added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: complex Award: test coverage labels Nov 29, 2019
@@ -1,74 +0,0 @@
<?php
Copy link
Contributor

@ihor-sviziev ihor-sviziev Nov 29, 2019

Choose a reason for hiding this comment

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

Please revert changes in this file as this is not observer and removing it will be backward incompatible. If it’s not used anymore - please mark it with tag @deprecated with description why it was deprecated.

/**
* @var \Magento\Framework\App\MaintenanceMode|\PHPUnit\Framework\MockObject\MockObject
*/
protected $maintenanceModeMock;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make it private

…ublic cache headers

- Removes the observer for disabling and re-enabling the FPC during
  maintenande mode switch

- Disables setting public cache headers, if maintenance mode is enabled

- phpcs:ignore entries were added in places where no actual code was
  changed by this commit, but static tests failed
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-6367 has been created to process this Pull Request

@engcom-Bravo
Copy link
Contributor

✔️ QA Passed

@m2-assistant
Copy link

m2-assistant bot commented Dec 8, 2019

Hi @Parakoopa, 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.

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.

None yet

7 participants