Skip to content

Replace remaining request globals in CentralColumns structure control… - #20478

Open
thomandre wants to merge 1 commit into
phpmyadmin:masterfrom
thomandre:central-columns-server-request
Open

thomandre wants to merge 1 commit into
phpmyadmin:masterfrom
thomandre:central-columns-server-request

Conversation

@thomandre

@thomandre thomandre commented Sep 8, 2026 •

Copy link
Copy Markdown

Description

Part of #17769.

Replaces the last $_POST access in the CentralColumns structure controllers.
RemoveController now takes the database from Current::$database, like
MakeConsistentController already does. The unset($_POST['submit_mult'])
in the three controllers is removed: the parsed body is a copy of $_POST
taken in ServerRequestFactory::fromGlobals(), and StructureController
does not read submit_mult, so it had no effect.

Added RemoveControllerTest to cover the changed line, and dropped the
phpstan/psalm baseline entries that no longer apply.

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.

…lers

Part of phpmyadmin#17769.

RemoveController passed $_POST['db'] straight to
CentralColumns::deleteColumnsFromList(). Current::$database holds the same
value, already validated by the DatabaseAndTableSetting middleware, and is
what the sibling MakeConsistentController uses, so use it here too. The
corresponding phpstan (argument.type) and psalm (PossiblyInvalidArgument,
PossiblyInvalidCast) baseline entries are dropped with it.

The three controllers also did unset($_POST['submit_mult']) before
delegating to StructureController. That has been a no-op since the
controllers switched to ServerRequest: the parsed body is a copy of $_POST
taken once in ServerRequestFactory::fromGlobals(), and nothing in
StructureController reads submit_mult from either source.

RemoveControllerTest covers the changed line: DbiDummy only answers the
exact central-columns SELECT and DELETE for db_name = 'test_db', so passing
any other database fails the test. Instantiating the controller from a test
also makes psalm's PossiblyUnusedMethod entry for its constructor obsolete,
hence that baseline block goes too.

Verification: phpcs clean on the changed files, phpstan reports no errors,
psalm reports only the UnusedBaselineEntry on src/Utils/HttpRequest.php that
is already present on master, phpunit tests/unit/Controllers/Database and
tests/unit/Routing pass (67 tests, 210 assertions).

Signed-off-by: Thomas André <thomandre@gmail.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
]));
$_SESSION['tmpval'] = [];
$_SERVER['SCRIPT_NAME'] = 'index.php';
$_REQUEST['db'] = Current::$database = 'test_db';

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.

Do we need $_REQUEST['db'] in that case?

$_REQUEST['db'] = Current::$database = 'test_db';

$dbiDummy = $this->createDbiDummy();
// CentralColumns::deleteColumnsFromList() must target Current::$database

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.

Suggested change
// CentralColumns::deleteColumnsFromList() must target Current::$database

. " WHERE db_name = 'test_db' AND col_name IN ('id','name','datetimefield');",
true,
);
// StructureController renders the database structure page afterwards

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.

Suggested change
// StructureController renders the database structure page afterwards

@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.84%. Comparing base (089bfe5) to head (7b1bec6).
⚠️ Report is 47 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #20478      +/-   ##
============================================
- Coverage     63.96%   63.84%   -0.13%     
  Complexity    16092    16092              
============================================
  Files           677      677              
  Lines         57844    57841       -3     
============================================
- Hits          37000    36928      -72     
- Misses        20844    20913      +69     
Flag Coverage Δ
dbase-extension ?
unit-8.2-ubuntu-latest ?
unit-8.3-ubuntu-latest ?
unit-8.4-ubuntu-latest ?
unit-8.5-ubuntu-latest ?
unit-8.6-ubuntu-latest 63.84% <100.00%> (-0.06%) ⬇️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

2 participants