On portals, make the "selected" / "default" logic more straightforward
ClosedPublic
Actions

Authored by epriestley on Mar 30 2019, 5:31 PM.

Details

Reviewers

amckinley

Maniphest Tasks

Commits

Summary

Depends on D20349. Ref T13275. Currently, a default item is selected as a side effect of generating the full list of items, for absolutely no reason.

The logic to pick the currently selected item can also be separated out pretty easily.

(And fix a bug in with a weird edge case in projects.)

This doesn't really change anything, but it will probably make T12949 a bit easier to fix.

Test Plan

Viewed Home / projects / portals, clicked various links, got same default/selection behavior as before.

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

epriestley requested review of this revision.Mar 30 2019, 5:32 PM

amckinley added inline comments.

src/applications/search/engine/PhabricatorProfileMenuEngine.php
1336	Is this going to result in a stable choice of default? Presumably `items` is an ordered list.
1347	So I understand the cast to `int` is because everything comes out of the MySQL layer as a string, but why cast the target ID initially instead of just using the `==` operator?

This revision is now accepted and ready to land.Apr 1 2019, 6:27 PM

src/applications/search/engine/PhabricatorProfileMenuEngine.php
1336	Yeah, the stored items have an explicit order (like `<someOrderColumn, id>`).
1347	If the ID is some kind of junk value like "quack", it will match `0` with `==`. There's probably no way we can reach this code and actually get a bad comparison, this is more just a general PHP hygiene thing where code that compares `if ("quack" == 0) { ... }` and enters the condition is never desirable even in cases where it doesn't cause any actual problems.