Page MenuHomePhorge

Change erroneous loose comparison to strict comparison in tab panels
ClosedPublic

Authored by Dylsss on Feb 20 2023, 23:40.
Tags
None
Referenced Files
F2203275: D25067.1716553130.diff
Thu, May 23, 12:18
Unknown Object (File)
Sun, May 12, 05:03
Unknown Object (File)
Sun, May 12, 04:14
Unknown Object (File)
Sun, May 5, 22:17
Unknown Object (File)
Sun, Apr 28, 19:16
Unknown Object (File)
Sat, Apr 27, 15:50
Unknown Object (File)
Thu, Apr 25, 14:25
Unknown Object (File)
Thu, Apr 25, 14:25

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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