Details
- Reviewers
epriestley btrahan - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5655: Standardize naming conventions
- Commits
- Restricted Diffusion Commit
rP86c399b65783: Rename `PhabricatorApplication` subclasses
- Pinned and uninstalled some applications.
- Applied patch and performed migrations.
- Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
- Performed a sanity check on the database contents.
Diff Detail
- Repository
- rP Phabricator
- Branch
- renameapp
- Lint
Lint Passed - Unit
Test Failures - Build Status
Buildable 1778 Build 1779: [Placeholder Plan] Wait for 30 Seconds
| Time | Test | |
|---|---|---|
| 0 ms | testControllerAccessControls | |
| 0 ms | testApplicationsInstalled | |
| 0 ms | testEverythingImplemented | |
| 0 ms | testFlattenRequestData | |
| 0 ms | testGetHTTPHeader | |
| View Full Test Results (1 Failed · 11 Passed) |
Event Timeline
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?
Finished the rest of the search-and-replace stuff. Still need to test and do migrations.
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 ↗ | (On Diff #24005) | 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? |
| 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.
- 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 ↗ | (On Diff #24005) |
Yeah I agree with that... I'll submit a separate diff. |