Skip to content

Fix #19537 - Make sure rowCount is always int#19538

Merged
MauricioFauth merged 4 commits into
phpmyadmin:masterfrom
liviuconcioiu:19537
Feb 24, 2025
Merged

Fix #19537 - Make sure rowCount is always int#19538
MauricioFauth merged 4 commits into
phpmyadmin:masterfrom
liviuconcioiu:19537

Conversation

@liviuconcioiu

@liviuconcioiu liviuconcioiu commented Jan 18, 2025

Copy link
Copy Markdown
Contributor

Description

Fixes #19537

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.

@liviuconcioiu
Cast rowCount to int
27a9838 
Signed-off-by: Liviu-Mihail Concioiu <liviu.concioiu@gmail.com>
@codecov

codecov Bot commented Jan 18, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.08%. Comparing base (1280adb) to head (27a9838).
Report is 46 commits behind head on master.

Additional details and impacted files 
@@             Coverage Diff              @@
##             master   #19538      +/-   ##
============================================
- Coverage     60.16%   60.08%   -0.09%     
+ Complexity    15864    15856       -8     
============================================
  Files           675      675              
  Lines         61098    61071      -27     
============================================
- Hits          36760    36693      -67     
- Misses        24338    24378      +40
Flag Coverage Δ
unit-8.2-ubuntu-latest 60.03% <100.00%> (-0.13%) ⬇️
unit-8.3-ubuntu-latest 60.08% <100.00%> (-0.06%) ⬇️
unit-8.4-ubuntu-latest 60.07% <100.00%> (?)
unit-8.5-ubuntu-latest ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@faissaloux

faissaloux commented Jan 18, 2025

Copy link
Copy Markdown
Contributor

This method is used as int everywhere, so I think it's better to change it from here and make sure it always returns int.

phpmyadmin/src/Table/Table.php

Line 1745 in 38bbaff

public function getRealRowCountTable(): int|null

@kamil-tekiela

kamil-tekiela commented Jan 18, 2025

Copy link
Copy Markdown
Contributor

You can even make this function simpler by using fetchValue and removing Util::backquote('row_count').

@liviuconcioiu
Refactor getRealRowCountTable
a99534f 
Signed-off-by: Liviu-Mihail Concioiu <liviu.concioiu@gmail.com>
@liviuconcioiu
Fix tests
6321d32 
Signed-off-by: Liviu-Mihail Concioiu <liviu.concioiu@gmail.com>
@liviuconcioiu
Update baselines
516b364 
Signed-off-by: Liviu-Mihail Concioiu <liviu.concioiu@gmail.com>
kamil-tekiela
kamil-tekiela approved these changes Jan 18, 2025
View reviewed changes

@kamil-tekiela kamil-tekiela left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very good, but I am thinking now whether we should actually show some information to the user that it's impossible to fetch the row count. For example, instead of displaying 0 show a message such as N/A.

@liviuconcioiu

liviuconcioiu commented Jan 18, 2025

Copy link
Copy Markdown
Contributor Author

Wouldn't we run then in more potential bugs? There are other functions that have to be changed for this change.

Also, instead of N/A, probably unknown can be used, since it's also used on Size column.

18.01.2025_19.19.50_REC.mp4

williamdes
williamdes approved these changes Jan 18, 2025
View reviewed changes

@williamdes williamdes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the diff is okay

@MauricioFauth MauricioFauth merged commit c3e4ef0 into phpmyadmin:master Feb 24, 2025
@MauricioFauth MauricioFauth self-assigned this Feb 24, 2025
@MauricioFauth MauricioFauth added this to the 6.0.0 milestone Feb 24, 2025
@liviuconcioiu liviuconcioiu deleted the 19537 branch February 24, 2025 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MauricioFauth MauricioFauth MauricioFauth approved these changes

@williamdes williamdes williamdes approved these changes

+1 more reviewer

@kamil-tekiela kamil-tekiela kamil-tekiela approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@MauricioFauth MauricioFauth

Labels

None yet

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

TypeError: PhpMyAdmin\Util::formatNumber(): Argument #1 ($value) must be of type string|int|float, null given

5 participants

@liviuconcioiu @faissaloux @kamil-tekiela @MauricioFauth @williamdes