Paths

Table of Contentst

Change erroneous loose comparison to strict comparison in tab panels
ClosedPublic
Actions

Authored by Dylsss on Feb 20 2023, 23:40.

Details

Reviewers

valerio.bozzolan
Cigaryno
avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15146: Dashboards: Tab panel sometimes displays the content of two tabs

Commits

rPcbc0e661544a: Change erroneous loose comparison to strict comparison in tab panels

Summary

Fix a loose comparison causing 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:

Tab bug loaded multiple times.jpg (1×1 px, 102 KB)

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

arcpatch-D25067

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 498
Build 498: 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: Dashboards: 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.

sgtm

valerio.bozzolan added inline comments.Mar 13 2023, 07:46

src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php
291	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.

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
291	It doesn't really matter, the logic is correct either-way.

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

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 9 2023, 13:30

Just trying to be useful and having this approved by someone, I proposed a small refactor to introduce a clear $is_selected status, once. So that we fix the fact that the logical business logic was re-implemented twice with different facets - since that was really the problem and we should fix this.

This also increases readability a bit.

What do you think about?

src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php
113–115
118
291

avivey accepted this revision.Apr 9 2023, 18:02

This revision is now accepted and ready to land.Apr 9 2023, 18:02

Hi @Dylsss, feel free to land :) A reviewer approved this patch!

If the original author will not land this in 8 days, I will land that, hoping to be useful.

But, I will probably amend that small thing I proposed as inline comment, also hoping to be useful.

Any feedback in inline is welcome of course

add dedicated variable $is_selected

Harbormaster completed remote builds in B498: Diff 866.Jun 2 2023, 13:53

In D25067#7076, @valerio.bozzolan wrote:

If the original author will not land this in 8 days, I will land that, hoping to be useful.

But, I will probably amend that small thing I proposed as inline comment, also hoping to be useful.

Any feedback in inline is welcome of course

As per this message ↑ I'm landing :) Thanks again for this fix

Closed by commit rPcbc0e661544a: Change erroneous loose comparison to strict comparison in tab panels. · Explain WhyJun 2 2023, 13:57

This revision was automatically updated to reflect the committed changes.

valerio.bozzolan added a commit: rPcbc0e661544a: Change erroneous loose comparison to strict comparison in tab panels.

valerio.bozzolan mentioned this in Next Up (since Week 18).Jun 2 2023, 14:12

valerio.bozzolan mentioned this in Z1: Phorge.Jun 12 2023, 14:09

speck mentioned this in D25291: Fix UX regression in Dashboard Tabs Panel.Jun 21 2023, 17:08