One broad issue is that I'd like to leave the door open for eventually reusing PHP interpreters between requests (see T2312). This would mean that the value of static variables might persist across requests, so we can't put anything in a static that could change between requests (putting stuff that can only change if the codebase is updated is fine).

In the Phacility cluster, the prior request might have been for a different Phabricator instance, so we specifically can't assume configuration (even locked configuration) is stable between requests, or that config changes will be so rare that it's fine to require a server restart.

I'm not sure we'll ever get there, but this is sort of a "future headroom" situation similar to read/write connections, where we aren't using the thing yet but it seems like we're within arm's reach of being able to if it's ever a priority.

Some of these isEnabled() methods could potentially change results between requests, because they may depend on configuration. In cases where there's a plausible argument that the value may differ between requests, I think we should avoid caching the result in ClassMapQuery and do explicit post-filtering instead.

Generally looks good, a couple of inlines.

scripts/ssh/ssh-exec.php
212	`$workflow_names` is used here, but no longer generated. (This didn't trigger lint because this stuff is at top level.)
src/applications/console/core/DarkConsoleCore.php
14–16	It looks like no plugins actually use this, so we could potentially just remove it and avoid the question of whether this is cacheable between requests or not.
src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php
9–11	I'd expect that this previously loaded a list of symbol dictionaries, and now loads a map of objects. Callers likely need changes to swap `newv($symbol['name'], array())` out or whatever.
src/applications/dashboard/customfield/PhabricatorDashboardPanelSearchApplicationCustomField.php
17	I think it's reasonable to assume that this is invariant across requests and configuration can't reasonably affect a SearchEngine's suitability for use in panel contexts.
src/applications/diviner/workflow/DivinerGenerateWorkflow.php
180–184	Definitely incomplete / not correct.
src/applications/doorkeeper/engine/DoorkeeperImportEngine.php
93	I think this one should be post-filtered: it's reasonable for configuration (e.g., an external auth provider set up vs not set up) to affect bridge availability.
src/applications/feed/worker/FeedPublisherWorker.php
26–29	Should be fine.
src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php
110	I think this one may also vary across config (e.g., installed vs uninstalled applications).
src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php
270	Variant, per above.
src/applications/settings/panel/PhabricatorEmailPreferencesSettingsPanel.php
196	This one seems OK.
src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php
26–28	`getFieldType()` can safely be unique'd, I think.

This revision now requires changes to proceed.Jul 8 2015, 8:24 AM

Fix undefined variable
Remove DarkConsoleCore::shouldStartup()
Fix PhabricatorDaemonManagementWorkflow
Fix DivinerGenerateWorkflow
Use getFieldType() as a unique method

epriestley accepted this revision.Jul 8 2015, 11:36 AM

epriestley edited edge metadata.

epriestley added inline comments.

src/applications/doorkeeper/engine/DoorkeeperImportEngine.php
93–94	Filtering here is still being cached, but I think we should filter after getting results back from the `Query` because this may vary across requests.