Page MenuHomePhabricator

Addressing some PHP8 incompatibilities - ProfileMenuItem
ClosedPublic

Authored by cspeckmim on May 9 2023, 3:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 6:08 PM
Unknown Object (File)
Tue, Mar 19, 8:44 PM
Unknown Object (File)
Sun, Mar 17, 3:20 AM
Unknown Object (File)
Thu, Mar 14, 5:59 PM
Unknown Object (File)
Feb 26 2024, 9:16 PM
Unknown Object (File)
Feb 22 2024, 8:23 AM
Unknown Object (File)
Feb 20 2024, 11:40 PM
Unknown Object (File)
Feb 20 2024, 4:25 PM
Subscribers

Details

Summary

Updates to all of the ProfileMenuItem classes to be compatible with PHP 8.

These changes were moved from D21862.

Refs T13588

Test Plan

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'

Remove unnecessary function

(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.

cspeckmim added inline comments.
src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php
108–114

Great info! I'll revert this change.

Revert change that doesn't distinguish between invalid vs. inaccessible dashboards

This revision is now accepted and ready to land.May 24 2023, 4:12 PM