Page MenuHomePhabricator

Rename `PhabricatorApplication` subclasses
ClosedPublic

Authored by joshuaspence on Jul 18 2014, 10:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 22, 8:05 PM
Unknown Object (File)
Thu, Apr 11, 10:50 AM
Unknown Object (File)
Sat, Apr 6, 4:53 AM
Unknown Object (File)
Fri, Apr 5, 1:47 AM
Unknown Object (File)
Wed, Apr 3, 8:34 PM
Unknown Object (File)
Wed, Apr 3, 4:11 PM
Unknown Object (File)
Mon, Apr 1, 6:31 AM
Unknown Object (File)
Mar 24 2024, 5:05 PM

Details

Summary

Ref T5655. Some discussion in D9839. Generally speaking, Phabricator{$name}Application is clearer than PhabricatorApplication{$name}.

Test Plan
  1. Pinned and uninstalled some applications.
  2. Applied patch and performed migrations.
  3. Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
  4. Performed a sanity check on the database contents.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to [RFC] Rename `PhabricatorApplication` instances.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added reviewers: epriestley, btrahan.

I'm okay with this if you want to do the legwork, but I think it involves some migrations, because class names are used to associate things with applications and to construct pseudo-PHIDs in some cases. Offhand:

  • phabricator.application-settings config
  • phabricator.uninstalled-applications config
  • Pinned applications in user preferences
  • Maybe more things I don't remember?
joshuaspence retitled this revision from [RFC] Rename `PhabricatorApplication` instances to Rename `PhabricatorApplication` instances.Jul 19 2014, 1:23 AM
joshuaspence updated this object.
joshuaspence edited edge metadata.

Finished the rest of the search-and-replace stuff. Still need to test and do migrations.

  • Ugly hack to determine application name
  • Don't migrate users without any pinned apps
  • Actually call save()

Fix ugly regex hack to allow application names containing uppercase characters

joshuaspence retitled this revision from Rename `PhabricatorApplication` instances to Rename `PhabricatorApplication` subclasses.Jul 20 2014, 9:54 AM
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
epriestley edited edge metadata.

Some minor inlines to make this go more smoothly.

resources/sql/autopatches/20140720.appname.php
94–96 ↗(On Diff #24005)

In the general case, save() isn't safe to do in a migration. Future changes may add columns to the object, and save() will try to update them, even though the migration which adds them hasn't run yet. In cases where the update is straightforward, prefer to UPDATE ... the column explicitly.

115–116 ↗(On Diff #24005)

This should setIsDeleted(0), see 20130919.mfieldconf.php. This is also unsafe, but saving this object is relatively complex so just calling save() here is probably the lesser evil.

138 ↗(On Diff #24005)

PhabricatorDashboardInstall should also be migrated. It has application PHIDs in the objectPHID column, and application class names in the applicationClass column.

src/applications/base/PhabricatorApplication.php
32

This will produce the wrong result for existing third-party applications.

We should check if the application name is in the form PhabricatorApplicationX and continue working, but raise a deprecation warning.

For upstream applications, maybe we should stop trying to do this automagically?

This revision now requires changes to proceed.Jul 21 2014, 3:40 PM
resources/sql/autopatches/20140720.appname.php
94–96 ↗(On Diff #24005)

Ok sure... it's maybe better that I just read from (and write to) the database directly then? Because the preferences are JSON serialised and the PhabricatorUserPreferences class doesn't give me a getAllPreferences() method.

Reading is (relatively) safe (extra/missing columns are handled reasonably), so either direct reads or object reads are normally both fine. You can use whichever is easier.

joshuaspence edited edge metadata.
  • Use setIsDeleted(0) in migration
  • Modify PhabricatorApplication::getName to work with applications that are named the old way
  • Update table via queryfx rather than relying on save
resources/sql/autopatches/20140720.appname.php
103 ↗(On Diff #24045)

Oh, this returns an array. Is there an easy way that I can convert this back to the expect format? Should I just json_encode it?

Are you sure its the objectPHID column for PhabricatorDashboardInstall?

mysql> SELECT * FROM phabricator_dashboard.dashboard_install;
+----+--------------------------------+-------------------+----------------------------+--------------------------------+-------------+--------------+
| id | installerPHID                  | objectPHID        | applicationClass           | dashboardPHID                  | dateCreated | dateModified |
+----+--------------------------------+-------------------+----------------------------+--------------------------------+-------------+--------------+
|  1 | PHID-USER-lioqffnwn6y475mu5ndb | dashboard:default | PhabricatorApplicationHome | PHID-DSHB-snznoqogptakanwm4amz |  1405980093 |   1405980093 |
+----+--------------------------------+-------------------+----------------------------+--------------------------------+-------------+--------------+
src/applications/base/PhabricatorApplication.php
32

For upstream applications, maybe we should stop trying to do this automagically?

Yeah I agree with that... I'll submit a separate diff.

epriestley edited edge metadata.

Are you sure its the objectPHID column for PhabricatorDashboardInstall?

Oh, I think I misread. That looks fine.

resources/sql/autopatches/20140720.appname.php
103 ↗(On Diff #24045)

Yeah, just json_encode() it.

123 ↗(On Diff #24045)

This one should setIsDeleted(0) too.

This revision is now accepted and ready to land.Jul 21 2014, 10:20 PM

Are you sure its the objectPHID column for PhabricatorDashboardInstall?

Oh, I think I misread. That looks fine.

Well, I will need to migrate the applicationClass field... but otherwise fine.

joshuaspence edited edge metadata.
  • json_encode
  • setIsDeleted
  • Migrate dashboard installs
  • Minor change
  • Remove some unrelated changes