Page MenuHomePhabricator

Addressing some PHP8 incompatibilities - ProfileMenuItem

Authored by cspeckmim on May 9 2023, 3:59 AM.
Referenced Files
Unknown Object (File)
Mon, Apr 15, 7:51 PM
Unknown Object (File)
Thu, Apr 11, 4:32 AM
Unknown Object (File)
Sat, Mar 30, 10:33 AM
Unknown Object (File)
Sat, Mar 30, 10:33 AM
Unknown Object (File)
Sat, Mar 30, 10:33 AM
Unknown Object (File)
Sat, Mar 30, 10:33 AM
Unknown Object (File)
Sat, Mar 30, 10:33 AM
Unknown Object (File)
Sat, Mar 23, 6:08 PM



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

rP Phabricator
Lint Not Applicable
Tests Not Applicable

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.


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.

20–26 ↗(On Diff #52117)

Oops I missed I introduced this here. I can remove the unnecessary function


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


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.

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