Page MenuHomePhorge

Change erroneous loose comparison to strict comparison in tab panels
ClosedPublic

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

Details

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

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.

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

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