Skip to content

Fix #17869 - Removing the stale table header cell. - #20335

Open
MatthewOscar wants to merge 3 commits into
phpmyadmin:QA_5_2from
MatthewOscar:fix/17869-extra-table-header-cell
Open

Fix #17869 - Removing the stale table header cell.#20335
MatthewOscar wants to merge 3 commits into
phpmyadmin:QA_5_2from
MatthewOscar:fix/17869-extra-table-header-cell

Conversation

@MatthewOscar

@MatthewOscar MatthewOscar commented Jun 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Description

In Browse mode, the results <thead> gained one extra cell on the right that
the body rows lacked. getColumnAtRightSide() emitted a right-side header cell
whenever RowActionLinks was left (the default), due to an operator-precedence
bug (A || (B && …)) and the wrong position constant in the branch guard.

Fix: mirror the correct sibling getFieldVisibilityParams() by gating both
branches on a $rightOrBoth check, so the right-side cell is only produced for
right/both layouts — matching the body. The placeholder is also emitted as
<th> (valid <thead> markup). Updated the stale ResultsTest expectation and
added a regression test across all four RowActionLinks values.

The second commit switches the left-side empty placeholder from <td> to <th>
for the same validity reason (the lone <td> left in the header); happy to drop
it if you'd prefer to keep this strictly scoped.

🤖 AI Disclaimer: Claude Opus 4.8 (1M context) was used in drafting the code for this pull request.

Fixes #17869

Before submitting pull request, please review the following checklist:

  • Make sure you have read our CONTRIBUTING.md document.
  • Make sure you are making a pull request against the correct branch. For example, for bug fixes in a released version use the corresponding QA branch and for new features use the master branch. If you have a doubt, you can ask as a comment in the bug report or on the mailing list.
  • Every commit has proper Signed-off-by line as described in our DCO. This ensures that the work you're submitting is your own creation.
  • Every commit has a descriptive commit message.
  • Every commit is needed on its own, if you have just minor fixes to previous commits, you can squash them.
  • Any new functionality is covered by tests.

@williamdes

williamdes commented Jun 22, 2026

Copy link
Copy Markdown
Member

Thanks for the PR, it looks good.
But I am unsure if there is any impact after this changes.
Did you use AI ? Please disclose it in the PR body, no issue about that :)

@MatthewOscar

MatthewOscar commented Jun 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

I will look into why the tests are failing and resolve the conflicts

Yes, I used Claude Code Opus to help. I will be updating the PR body soon. I think the scope of the changes is pretty tight as can be.

@williamdes

This comment was marked as resolved.

Sign in to view
@MatthewOscar

MatthewOscar commented Jun 23, 2026

Copy link
Copy Markdown
Author

@williamdes I have addressed the suggested changes and implemented them. What do you think?

@MatthewOscar

MatthewOscar commented Jun 23, 2026

Copy link
Copy Markdown
Author

@williamdes

The red lint-node /Test javascript files/ build-release jobs aren't from this PR but from the Update Yarn dependencies commit on QA_5_2 (0cab39f6), which bumped node-releases to 2.0.48 (engines: node >=18) while CI still pins node 12 (failing run). This PR only touches PHP/test files. Happy to send a separate PR bumping the CI node version if helpful.

@williamdes

williamdes commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

🤖 Follow-up written by me with Claude Opus 4.8 (1M context); reviewed by me.

Thanks for the quick turnaround — the new commit resolves the R1 concern cleanly. Confirming the logic across all layouts:

  • right/both + no edit/delete links + text_btn=1 (JOINs / SHOW / keyless): the full/partial-text toggle is back in the right header, and the new getTableBody() elseif emits a matching empty <td class="d-print-none"> so every body row stays aligned with it.
  • left/none: still nothing on the right — header and body agree.
  • emptyafter/repeating-header alignment holds in each case (1 right cell when the toggle/links are present, 0 otherwise).

The new testGetColumnAtRightSideShowsTextButtonWithoutEditDeleteLinks covers exactly the case I was worried about. 👍

Two non-blockers:

  1. The <td>→<th> change on the left placeholder (getFieldVisibilityParams branch 3) is fine — that branch is effectively unreachable for SELECTs (text_btn is always 1), so makegrid.js's find('th').first() action-column detection is unaffected in practice.
  2. Same buggy ||/&& precedence exists on master (6.0) — worth forward-porting this fix.

LGTM from my side once CI is green.

@MatthewOscar
Fix phpmyadmin#17869 - Remove extra table header cell in browse results
0a1c170 
getColumnAtRightSide() added a stray right-side cell to the results
header whenever $cfg['RowActionLinks'] was 'left' (the default): the
branch guard relied on operator precedence (A || B && C) and keyed off
POSITION_LEFT, so the cell was emitted even though left-positioned row
actions leave no matching cell in the body rows. It was also emitted as
a <td> inside <thead>.

Mirror the sibling getFieldVisibilityParams() by gating both branches on
a $rightOrBoth check, so the right-side header cell is only produced for
the 'right'/'both' layouts (matching the body), and emit it as <th>.

Signed-off-by: Matthew Wyatt <mwyatt2023@fau.edu>
@MatthewOscar
Use <th> for the left-side empty action-column header placeholder
f52e999 
The empty placeholder for the left action column was emitted as a <td>
inside <thead> -- the only header cell not using <th>. The column
headers, the action-button branch and POSITION_NONE branch in the same
method, getRepeatingHeaders(), the now-fixed right-side placeholder, and
every other table template all use <th>. It has been a <td> since the
initial revision (2001) and was carried through later refactors
uncorrected. makegrid.js reads header cells via 'thead th', so a <td>
first cell is skipped; switching to <th> aligns the markup with the JS
and with valid <thead> semantics.

Related to phpmyadmin#17869.

Signed-off-by: Matthew Wyatt <mwyatt2023@fau.edu>
@MatthewOscar
Show full/partial-text toggle on right/both without misaligning rows
cf5711a 
getColumnAtRightSide emits the toggle for right/both when text_btn=1 even
without edit/delete links (review follow-up to phpmyadmin#17869). To keep the header
from becoming wider than the body for keyless/SHOW/JOIN results,
getTableBody emits a matching empty cell under that toggle so the columns
stay aligned.

Also adds testGetColumnAtRightSideShowsTextButtonWithoutEditDeleteLinks
(all four RowActionLinks positions) and makes the test closures Psalm- and
PHPStan-clean.

Signed-off-by: Matthew Wyatt <mwyatt2023@fau.edu>
@MatthewOscar
MatthewOscar force-pushed the fix/17869-extra-table-header-cell branch from 8512d05 to cf5711a Compare June 26, 2026 16:37
@MatthewOscar

MatthewOscar commented Jun 26, 2026

Copy link
Copy Markdown
Author

@williamdes I rebased this branch onto the latest QA_5_2, which includes fea3336d64 (Downgrade node-releases package to <2.0.45). That should address the previous CI failures from node-releases@2.0.48 requiring Node >=18 while CI used Node 12. Once it runs, we should expect to see all checks passing!

@MatthewOscar

MatthewOscar commented Jun 29, 2026

Copy link
Copy Markdown
Author

@williamdes All checks have passed! Is this ready to be merged?

@MatthewOscar

MatthewOscar commented Jul 7, 2026

Copy link
Copy Markdown
Author

@williamdes Just want to notify you again since a week has passed. Is everything good to go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MatthewOscar @williamdes