Changeset View
Standalone View
resources/sql/autopatches/20170131.dashboard.personal.01.php
- This file was added.
| <?php | |||||
| $table_db = new PhabricatorDashboardInstall(); | |||||
| $viewer = PhabricatorUser::getOmnipotentUser(); | |||||
| $applications = id(new PhabricatorApplicationQuery()) | |||||
| ->setViewer($viewer) | |||||
| ->withClasses(array('PhabricatorHomeApplication')) | |||||
| ->withInstalled(true) | |||||
| ->execute(); | |||||
| $home = head($applications); | |||||
epriestley: This will come back `null` if Home is uninstalled, e.g. in the Phacility cluster. Then, we'll… | |||||
| foreach (new LiskMigrationIterator($table_db) as $install) { | |||||
epriestleyUnsubmitted Done Inline ActionsIt would be nice to use LiskRawMigrationIterator here (which returns raw rows, instead of objects), instead of LiskMigrationIterator, so that we can delete PhabricatorDashboardInstall later without revisiting this migration. Right now, we have to keep PhabricatorDashboardInstall around forever. epriestley: It would be nice to use `LiskRawMigrationIterator` here (which returns raw rows, instead of… | |||||
| $item = id(new PhabricatorProfileMenuEngine()) | |||||
| ->setBuiltinKey(null) | |||||
epriestleyUnsubmitted Done Inline ActionsShould be unnecessary. epriestley: Should be unnecessary. | |||||
| ->setMenuItemKey(PhabricatorDashboardProfileMenuItem::MENUITEMKEY); | |||||
Done Inline ActionsFor consistency, prefer $dashboard_phid vs $dashboardPHID. epriestley: For consistency, prefer `$dashboard_phid` vs `$dashboardPHID`. | |||||
| $dashboard = id(new PhabricatorDashboardQuery()) | |||||
Not Done Inline ActionsI think this'll fatal now since $install is an array, not an object, so ->getDashboardPHID() isn't valid. (You probably don't need to load the dashboard at all? It's OK to insert a bad menu item, we'll just say "Invalid dashboard".) epriestley: I think this'll fatal now since `$install` is an array, not an object, so `->getDashboardPHID… | |||||
| ->setViewer($viewer) | |||||
| ->withPHIDs(array($dashboard_install->getDashboardPHID())) | |||||
epriestleyUnsubmitted Done Inline ActionsI think $dashboard_install is never defined? epriestley: I think `$dashboard_install` is never defined? | |||||
| ->needPanels(true) | |||||
epriestleyUnsubmitted Done Inline ActionsWe probably don't need to load the panels, I think. Do we even need to load the dashboard at all? Can we just use the PHID? epriestley: We probably don't need to load the panels, I think. Do we even need to load the dashboard at… | |||||
| ->executeOne(); | |||||
epriestleyUnsubmitted Done Inline ActionsIt's possible that $dashboard will come back null, if someone deleted a dashboard manually or maybe used bin/remove destroy. Slightly cleaner to test for !$dashboard and just continue;. epriestley: It's possible that `$dashboard` will come back null, if someone deleted a dashboard manually or… | |||||
| $custom_phid = null; | |||||
| if ($install->getObjectPHID() != 'dashboard:default') { | |||||
| $custom_phid = $install->getObjectPHID(); | |||||
| } | |||||
| $query = id(new PhabricatorProfileMenuItemConfiguration()) | |||||
| ->setProfilePHID($home->getPHID()) | |||||
| ->setMenuItemKey($item->getMenuItemKey()) | |||||
Done Inline ActionsThe last one here should be %ns ("nullable string"), not just %s ("string"), so that we insert null if $custom_phid is null. As is, this will migrate personal dashboards but not global dashboards. epriestley: The last one here should be `%ns` ("nullable string"), not just `%s` ("string"), so that we… | |||||
| ->attachMenuItem($item) | |||||
| ->attachProfileObject($dashboard) | |||||
| ->setCustomPHID($install->getObjectPHID()); | |||||
chadAuthorUnsubmitted Done Inline Actionsspecifically, will this run and build an object? chad: specifically, will this run and build an object? | |||||
epriestleyUnsubmitted Done Inline ActionsThis doesn't actually save() the object so I think it won't do anything. Ideally, we would do a raw INSERT instead of a save(). With save(), this migration may break in the future if we change the table. For example, if we later add an icon field to the Configuration, this save() will start trying to INSERT it, but it won't exist yet because this will run before 20180913.add-an-icon-to-menu-items.sql or whatever, which actually adds the column. epriestley: This doesn't actually `save()` the object so I think it won't do anything.
Ideally, we would… | |||||
| } | |||||
Not Done Inline ActionsI don't think this syntax is valid -- it's a mixture of UPDATE and INSERT. Should be: INSERT INTO %T (col, col, col, ...) VALUES (%s, %s, %s, ...) epriestley: I don't think this syntax is valid -- it's a mixture of UPDATE and INSERT. Should be:
```… | |||||
This will come back null if Home is uninstalled, e.g. in the Phacility cluster. Then, we'll fatal on line 31. We should probably just migrate no matter what. One advantage is that we won't lose anything if someone uninstalls home, then migrates, then installs it again later. You can get the PHID like this, without worrying whether the application is installed or not.
$home_phid = id(new PhabricatorHomeApplication()) ->getPHID();