Skip to content

Fix #18204 - Update getInnodbPluginVersion for MariaDB compatibility#20220

Open
gs71 wants to merge 1 commit intophpmyadmin:QA_5_2from
gs71:patch-1
Open

Fix #18204 - Update getInnodbPluginVersion for MariaDB compatibility#20220
gs71 wants to merge 1 commit intophpmyadmin:QA_5_2from
gs71:patch-1

Conversation

@gs71
Copy link
Copy Markdown
Contributor

@gs71 gs71 commented Mar 7, 2026

Description

Starting from version 10.10 MariaDB removed the system variable 'innodb_version' (https://jira.mariadb.org/browse/MDEV-28554), so clicking on the 'Operations' link produces an error in the SQL errors log (if enabled):

ERROR 1193: Unknown system variable 'innodb_version' : SELECT @@innodb_version

Fixes #18204

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.

Signed-off-by: Guido Selva guido.selva@gmail.com

@gs71 gs71 changed the title Update getInnodbPluginVersion for MariaDB compatibility Fix #18204 - Update getInnodbPluginVersion for MariaDB compatibility Mar 7, 2026
global $dbi;

return $dbi->fetchValue('SELECT @@innodb_version;') ?: '';
return ($dbi->isMariaDB() && $dbi->getVersion() >= 101000) ? '' : ($dbi->fetchValue('SELECT @@innodb_version') ? : '');
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.

Suggested change
return ($dbi->isMariaDB() && $dbi->getVersion() >= 101000) ? '' : ($dbi->fetchValue('SELECT @@innodb_version') ? : '');
return ($dbi->isMariaDB() && $dbi->getVersion() >= 101000) ? '' : ($dbi->fetchValue('SELECT @@innodb_version;') ? : '');

global $dbi;

return $dbi->fetchValue('SELECT @@innodb_version;') ?: '';
return ($dbi->isMariaDB() && $dbi->getVersion() >= 101000) ? '' : ($dbi->fetchValue('SELECT @@innodb_version') ? : '');
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.

if you return empty it will break

$innodb_file_format = $innodbEnginePlugin->getInnodbFileFormat() ?? '';

was this variable also removed ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Would you mind refactoring this so we have a clear path that the two variables are only displayed and fetched if the database version is the right one ?
Like you did in the other PR

Copy link
Copy Markdown
Contributor Author

@gs71 gs71 Mar 17, 2026

Choose a reason for hiding this comment

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

I just noticed that even in MySQL this variable is quite useless:
https://dev.mysql.com/doc/refman/8.4/en/innodb-parameters.html#sysvar_innodb_version

Maybe it's better to remove it completely?

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.

It depends if it useless since before the minimum MySQL version for phpMyAdmin or not
They say we can use @@version instead

Copy link
Copy Markdown
Contributor Author

@gs71 gs71 Mar 17, 2026

Choose a reason for hiding this comment

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

OK, then we can juste replace :

return $dbi->fetchValue('SELECT @@innodb_version;') ?: '';

With:

return $dbi->fetchValue('SELECT @@version;') ?: '';

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.

Probably yes, but it needs to be checked on the lowest version supported

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.

If you just replace it then it becomes useless, right? I think the point was to check whether this setting exists and is not empty.

Copy link
Copy Markdown
Contributor Author

@gs71 gs71 Mar 17, 2026

Choose a reason for hiding this comment

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

I think that the function getInnodbPluginVersion() is useless, since it is only used in libraries/classes/Operations.php as follows:

        $innodbPluginVersion = $innodbEnginePlugin->getInnodbPluginVersion();
        $innodb_file_format = '';
        if (! empty($innodbPluginVersion)) {
            $innodb_file_format = $innodbEnginePlugin->getInnodbFileFormat() ?? '';
        }

This block could just be replaced by:

$innodb_file_format = $innodbEnginePlugin->getInnodbFileFormat() ?? '';

And the function getInnodbFileFormat() could be optimized as:

    public function getInnodbFileFormat(): ?string
    {
        global $dbi;

        return (
          ($dbi->isMariaDB() && $dbi->getVersion() >= 100600)
          || ($dbi->isMySql() && $dbi->getVersion() >= 80000) 
        ) ? '' : $dbi->fetchValue("SELECT @@innodb_file_format;");
    }

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.

Indeed, please apply this

@gs71
Copy link
Copy Markdown
Contributor Author

gs71 commented Mar 30, 2026

Created new PR #20261

@gs71 gs71 closed this Mar 30, 2026
@williamdes williamdes reopened this Mar 31, 2026
@williamdes
Copy link
Copy Markdown
Member

Created new PR #20261

Please avoid, use this one and force push your work if needed

@gs71
Copy link
Copy Markdown
Contributor Author

gs71 commented Mar 31, 2026

Actually these modifications have already been pushed by another PR (not mine).
So this PR can just be closed.

@williamdes
Copy link
Copy Markdown
Member

Actually these modifications have already been pushed by another PR (not mine). So this PR can just be closed.

I could not find it, are you sure ?

@gs71
Copy link
Copy Markdown
Contributor Author

gs71 commented Mar 31, 2026

Actually these modifications have already been pushed by another PR (not mine). So this PR can just be closed.

I could not find it, are you sure ?

You are right, but I don't know how to do it.

My code https://github.com/gs71/phpmyadmin/tree/QA_5_2 is fine, with 2 proper commits ready for production. But I don't know how to apply them to this PR.

@williamdes
Copy link
Copy Markdown
Member

My code https://github.com/gs71/phpmyadmin/tree/QA_5_2 is fine, with 2 proper commits ready for production. But I don't know how to apply them to this PR.

Probably: git push origin --force --dry-run QA_5_2:patch-1
Remove dry run mode when you are sure

Or
Probably: git push origin --force --dry-run HEAD:patch-1
if you are on the commit you want to send to this PR

@gs71
Copy link
Copy Markdown
Contributor Author

gs71 commented Mar 31, 2026

I tried with PR #20262, it always fails: I give up.

@williamdes
Copy link
Copy Markdown
Member

I tried with PR #20262, it always fails: I give up.

Okay, do not change anything. I will manage this myself

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants