Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T13588: PHP 8 Compatibility
- Commits
- rP58995268dd97: Addressing some PHP8 incompatibilities - ProfileMenuItem
I loaded the profile page for a user and saw all the items I expected to see.
I navigated through the different profile nav items.
I loaded the settings page for a user and saw all the items I expected to see.
I navigated through the differnt setting nave items.
I added Cat Facts to my navigation.
Diff Detail
- Repository
- rP Phabricator
- Branch
- cspeck-php8-pmi
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 25784 Build 35612: arc lint + arc unit
Event Timeline
I changed the approach from what I initially had in D21862, which was to add the PhabricatorProfileMenuItem::getDefaultName() as an abstract function and make the default implementation of getDisplayName() to read the 'name' menu property from the config.
Making all implementations require a getDefaultName() didn't feel quite right when I found out that only ~80% had that to begin with. This approach adds a utility method for the default implementation for which ~95% of implementations can make use of.
src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php | ||
---|---|---|
108–114 | Other implementations didn't make the distinction between restricted or invalid (and in theory doesn't leak info maybe?) so I updated this to behave as the others did. I can revert if this if it's intentional to distinguish here. | |
src/applications/search/menuitem/PhabricatorDividerProfileMenuItem.php | ||
20–26 ↗ | (On Diff #52117) | Oops I missed I introduced this here. I can remove the unnecessary function |
src/applications/search/menuitem/PhabricatorMotivatorProfileMenuItem.php | ||
24 | This was the only profile menu item that uses the 'source' property instead of 'name' |
(Haven't forgotten about this stuff, but the kids got sick and family is flying in soon.)
src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php | ||
---|---|---|
108–114 | It's intentional to distinguish (and generally desirable to distinguish), the other implementations are just being lazy (and/or the distinction is almost impossible to hit). Phabricator (broadly, at least) allows you to know about the existence of things you can't view (e.g., you can deduce that T123 must exist if T122 and T124 exist), and learn generic information about those objects (e.g., you can learn the PHIDs of objects you can't view by querying edges, etc). In particular, Phabricator allows users to receive a "This object is in a good state, everything is working properly, you just don't have permission to view it." error, and treats this as a distinct state from "This object is broken/misconfigured/buggy/etc". I think this distinction is pretty helpful in practice to users, since it gives them a much clearer sense of what their next step should be. So the ideal behavior is for all items to make this distinction, but it's probably moot in most cases. |
src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php | ||
---|---|---|
108–114 | Great info! I'll revert this change. |