-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Stop flushing fpc when not asked for #38215
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
Stop flushing fpc when not asked for #38215
Conversation
Hi @hostep. Thank you for your contribution! Add the comment under your pull request to deploy test or vanilla Magento instance:
❗ Automated tests can be triggered manually with an appropriate comment:
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
01b8366
to
53b87ab
Compare
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
@magento create issue |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
✔️ QA PassedPreconditions:
Manual testing scenario:
Builds are failing. Hence moving it to |
@magento run Integration Tests,Functional Tests EE,Functional Tests CE,Functional Tests B2B |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
@magento run Semantic Version Checker |
Description (*)
We found out by surprise today on Slack (thanks @benjamin-volle & @robinero) that running
bin/magento cache:clean
orbin/magento cache:flush
will always flush FPC (built-in or Varnish) entirely, even if you've only provided a type to be cleaned likeconfig
,layout
, ...I understand that yes, probably if one of those types get flushed, you'll most likely want to flush FPC as well, but let that be up to the user to decide by seeing if they provided the type
full_page
explicitly in the command.This fixes this.
Some history of this problem, flushing FPC via CLI got introduced at the request of #2025 and got fixed by c75c203
This PR enhances this a bit and will only dispatch those 2 events when either no types were provided in the command line or when
full_page
is one of the types provided.Related Pull Requests
Fixed Issues (if relevant)
No idea
Manual testing scenarios (*)
bin/magento cache:flush
andbin/magento cache:clean
=> expected to hit one of thosedie
statements introduced in step 1bin/magento cache:flush full_page
andbin/magento cache:clean full_page
=> expected to hit one of thosedie
statements introduced in step 1bin/magento cache:flush layout
andbin/magento cache:clean layout
=> expected to not hit one of thosedie
statements introduced in step 1Questions or comments
Since we introducedMagento\PageCache
in theMagento_Backend
module, should we addmagento/module-page-cache
as dependency in the Backend module'scomposer.json
file? We can also choose to hardcodefull_page
and don't use the constant, that's also an option ...Update: looks like the automated tests also picked this up, I already force-pushed a change to have it hardcoded instead of using a constant from another module, adding a dependency might be out of scope here.
I noticed that the New Relic module also listens for that
adminhtml_cache_flush_system
event, but it's done in theadminhtml/events.xml
file, so that should be fine since it's in theadminhtml
area. Running those cli commands wouldn't have triggered this observer anyways. So no regression there.Contribution checklist (*)
Resolved issues: