Page MenuHomePhorge
Differential D25067

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

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
View All 9 Files
Subscribers
Cigaryno
Matthew
speck
tobiaswiese
valerio.bozzolan

Details

Reviewers
valerio.bozzolan
Cigaryno
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15146: Tab panel sometimes displays the content of two tabs
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 created this revision.Feb 20 2023, 23:40
Owners added a reviewer: O1: Blessed Committers.Feb 20 2023, 23:40
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptFeb 20 2023, 23:40
Dylsss requested review of this revision.Feb 20 2023, 23:40
Harbormaster completed remote builds in B115: Diff 319.Feb 20 2023, 23:40
Dylsss added a task: T15146: Tab panel sometimes displays the content of two tabs.Feb 20 2023, 23:41
Dylsss retitled this revision from Change erroneous loose comparison to strict comparison to Change erroneous loose comparison to strict comparison in tab panels.
valerio.bozzolan accepted this revision as: valerio.bozzolan.Tue, Mar 7, 19:42

sgtm

valerio.bozzolan added inline comments.Mon, Mar 13, 07:46
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.

valerio.bozzolan added a comment.Mon, Mar 20, 16:16

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

Dylsss added a comment.Thu, Mar 23, 04:16
In D25067#2470, @valerio.bozzolan wrote:

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.

Cigaryno accepted this revision.Thu, Mar 23, 07:39

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)Packages
src/
applications/
dashboard/
paneltype/
2 lines0%
Loading...
O1: Blessed Committers

Diff 319

View Options

src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php

Loading...