Page MenuHomePhorge

Change erroneous loose comparison to strict comparison in tab panels
Needs ReviewPublic

Authored by Dylsss on Feb 20 2023, 23:40.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 10, 03:24
Unknown Object (File)
Wed, Mar 8, 12:23
Unknown Object (File)
Sat, Mar 4, 03:53
Unknown Object (File)
Thu, Mar 2, 06:58
Unknown Object (File)
Thu, Mar 2, 06:58
Unknown Object (File)
Wed, Mar 1, 16:37
Unknown Object (File)
Feb 22 2023, 13:48
Unknown Object (File)
Feb 22 2023, 13:47

Details

Summary

This loose comparison causes a bug when comparing the selected tab (0) with a tab which has an alphanumeric ID which doesn't begin with an integer.

E.g., (0 == 'kq3p37awi2n5') is true in PHP 7.4 and below, this can sometimes cause multiple tabs to be displayed when a tab panel is first loaded onto a page.

Test Plan

Create a tab panel with at least 3 tabs, add a couple more and ensure that multiple tabs aren't loaded.

Diff Detail

Repository
rP Phorge
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 115
Build 115: arc lint + arc unit

Event Timeline

Dylsss requested review of this revision.Feb 20 2023, 23:40
Dylsss retitled this revision from Change erroneous loose comparison to strict comparison to Change erroneous loose comparison to strict comparison in tab panels.
src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php
289

Thanks. Seems good to me. I have not a strong opinion about this, but above I see:

$tab_view->setSelected( (string)$idx === (string)$selected )

Then I wonder whenever we should do the same string-cast also here.

Having said that (string)$idx === (string)$selected seems legit to me, I'm not sure I can reproduce this issue before this patch.

I'm not sure I can reproduce this issue before this patch.

W32

src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php
289

It doesn't really matter, the logic is correct either-way.