Page MenuHomePhabricator

Add dialog to purge opcode/data caches
ClosedPublic

Authored by meitros on Sep 5 2015, 7:02 PM.
Tags
None
Referenced Files
F13087811: D14064.diff
Thu, Apr 25, 1:06 AM
F13081793: D14064.diff
Wed, Apr 24, 9:50 PM
Unknown Object (File)
Sun, Apr 21, 3:42 PM
Unknown Object (File)
Wed, Apr 17, 2:32 PM
Unknown Object (File)
Fri, Apr 12, 8:43 AM
Unknown Object (File)
Fri, Apr 12, 8:43 AM
Unknown Object (File)
Fri, Apr 12, 8:43 AM
Unknown Object (File)
Fri, Apr 12, 8:43 AM

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Commits
Restricted Diffusion Commit
rPae0348aac931: Add dialog to purge opcode/data caches
Summary

Reachable via the cache config page, restricted to admins only. This makes it convenient to hotfix phabricator without requiring a restart.

Test Plan
  • Local dev machine doesn't have apc, so I get the not installed message.
  • Faked the name and isEnabled parameters, verified dialog shows up as expected.
  • Didn't test clear code

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

meitros retitled this revision from to RFC: Add controller to purge apc.
meitros updated this object.
meitros edited the test plan for this revision. (Show Details)
meitros added a reviewer: epriestley.

Conceptually fine but I want to smooth it down a bit before upstreaming it. Probably put it here with other cache stuff:

purge.png (1×2 px, 328 KB)

When clicked, pop a confirmation dialog with the caveat that only the current web frontend will be purged. Show "no caches are configured" if nothing is purgeable.

When the user confirms, clear both APC and OPCache (if they're installed), then tell the user which cache(s) were cleared.

That's like 200x more work so feel free to not bother and I'll just commandeer this at some point.

I'll do it, but not for a few days

meitros retitled this revision from RFC: Add controller to purge apc to Add dialog to purge opcode/data caches.Sep 10 2015, 3:19 AM
meitros updated this object.
meitros edited the test plan for this revision. (Show Details)
meitros edited edge metadata.

add current web frontend caveat

epriestley edited edge metadata.

Looks great! Couple of minor simplicity/clarity/fragility things in line, if they make sense.

src/applications/cache/spec/PhabricatorDataCacheSpec.php
125–134

This feels a little redundant (both cache specs implement similar code) and a little fragile (changing human-readable text in pht() generally should not affect program behavior, but it will here).

How about this instead?

Add these to PhabricatorCacheSpec (or pick some better names if these feel too clunky):

  • private $clearCacheCallback;
  • setClearCacheCallback($callback)
  • getClearCacheCallback()
  • (Optionally, isClearable(), which just tests if a callback is set, but maybe testing for a null return from getClearCacheCallback() would be good enough.)

Then, when building the caches, use $this->setClearCacheCallback('apc_clear_cache');, etc., to specify how to clear the cache.

src/applications/config/controller/PhabricatorConfigPurgeCacheController.php
5

This should likely extend PhabricatorConfigController instead.

It can then inherit the shouldRequireAdmin() implementation automatically, and doesn't need an explicit implementation.

16

Minor nit, but maybe define $cancel_uri = $this->getApplicationURI('cache/'); here to slightly reduce repetition, as this URI is hard-coded in three places in the remainder of the controller.

55

Consider naming the button "Clear Cache" for clarity.

This revision now requires changes to proceed.Sep 10 2015, 11:22 AM
meitros edited edge metadata.

fixes. Didn't think of a less clunky name for clearCacheCallback

I forgot to retest, so if you accept hold off on pulling until I do

epriestley edited edge metadata.

I think I caught two minor things. This works properly for me locally (with both cache types available).

src/applications/cache/spec/PhabricatorDataCacheSpec.php
51

Incorrect / redundant with the call below inside the ini_get() clause?

src/applications/config/controller/PhabricatorConfigPurgeCacheController.php
24–25

If one of these caches is missing but the other is present (e.g., we have OpCache but not APCu) I think this will try to clear a null callback and produce an error like this:

PHP Warning: call_user_func() expects parameter 1 to be a valid callback, no array or string given in ... on line ...

Likely fix is testing if the callbacks are set before invoking them.

This revision now requires changes to proceed.Sep 10 2015, 5:22 PM
meitros edited edge metadata.

fixes and minor changes. also retested

This revision is now accepted and ready to land.Sep 10 2015, 9:18 PM
This revision was automatically updated to reflect the committed changes.