Page MenuHomePhabricator

Use PhutilClassMapQuery instead of PhutilSymbolLoader
ClosedPublic

Authored by joshuaspence on Jul 7 2015, 11:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 1:49 PM
Unknown Object (File)
Mon, Mar 25, 1:49 PM
Unknown Object (File)
Mon, Mar 25, 1:49 PM
Unknown Object (File)
Sat, Mar 2, 3:26 PM
Unknown Object (File)
Sat, Mar 2, 3:26 PM
Unknown Object (File)
Sat, Mar 2, 3:26 PM
Unknown Object (File)
Fri, Mar 1, 12:34 PM
Unknown Object (File)
Fri, Mar 1, 11:54 AM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Commits
Restricted Diffusion Commit
rP368f3591142e: Use PhutilClassMapQuery instead of PhutilSymbolLoader
Summary

Use PhutilClassMaQuery instead of PhutilSymbolLoader, mostly for consistency. Depends on D13588.

Test Plan

Poked around a bunch of pages.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

joshuaspence retitled this revision from to Use PhutilClassMapQuery instead of PhutilSymbolLoader.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
epriestley edited edge metadata.

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
12–13

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–12

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–19

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–94

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–111

I think this one may also vary across config (e.g., installed vs uninstalled applications).

src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php
270–271

Variant, per above.

src/applications/settings/panel/PhabricatorEmailPreferencesSettingsPanel.php
196–197

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
joshuaspence edited edge metadata.
joshuaspence marked 5 inline comments as done.
  • Fix undefined variable
  • Remove DarkConsoleCore::shouldStartup()
  • Fix PhabricatorDaemonManagementWorkflow
  • Fix DivinerGenerateWorkflow
  • Use getFieldType() as a unique method
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.

This revision is now accepted and ready to land.Jul 8 2015, 11:36 AM
joshuaspence edited edge metadata.
This revision was automatically updated to reflect the committed changes.