Change erroneous loose comparison to strict comparison in tab panels
Concern Raisedcbc0e661544a
Actions

Description

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.

Reviewers: O1 Blessed Committers, valerio.bozzolan, Cigaryno, avivey

Reviewed By: O1 Blessed Committers, valerio.bozzolan, Cigaryno, avivey

Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Maniphest Tasks: T15146

Differential Revision: https://we.phorge.it/D25067

Details

Auditors

valerio.bozzolan

Provenance

Dylsss	Authored on Jun 2 2023, 13:16
valerio.bozzolan	Committed on Jun 2 2023, 13:57
valerio.bozzolan	Pushed on Jun 2 2023, 13:57

Reviewer

O1: Blessed Committers

Differential Revision

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

Parents

rP6b8ec5014890: Fix PHP 8.1 "preg_match(null)" exception when cloning a repository with no URI…

Branches

Unknown

Tags

Unknown

Tasks

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

Event Timeline

valerio.bozzolan committed rPcbc0e661544a: Change erroneous loose comparison to strict comparison in tab panels (authored by Dylsss).Jun 2 2023, 13:57

valerio.bozzolan added a task: T15146: Dashboards: Tab panel sometimes displays the content of two tabs.

valerio.bozzolan added a revision: D25067: Change erroneous loose comparison to strict comparison in tab panels.

valerio.bozzolan mentioned this in D25291: Fix UX regression in Dashboard Tabs Panel.Jun 12 2023, 14:22

Very interesting, I caused a regression by proposing an improvement.

I probably need to keep my suggestions in my butt.

This commit now has outstanding concerns.Jun 12 2023, 14:23

valerio.bozzolan added inline comments.Jun 12 2023, 14:23

/src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php
291	At this point the `$is_selected` rappresents just the last value of `$is_selected` inside the above loop. So it needs to be re-defined again inside this loop.