fix: remove dead Suhosin warning from HomeController#20123
fix: remove dead Suhosin warning from HomeController#20123somethingwithproof wants to merge 0 commit intophpmyadmin:masterfrom
Conversation
|
I think the setting should be removed too. It was probably the only place it was used and there's no reason to leave an orphaned setting. |
|
Good catch — updated to also remove the SuhosinDisableWarning setting from the config. All references cleaned up. |
somethingwithproof
force-pushed
the
fix/remove-suhosin-warning
branch
from
efd822c to
dec0741
Compare
| Whether to log successful authentication attempts into | ||
| :config:option:`$cfg['AuthLog']`. | ||
|
|
||
| .. config:option:: $cfg['SuhosinDisableWarning'] |
There was a problem hiding this comment.
Please keep the documentation and add a versionremoved statement
| * `suhosin.executor.disable_emodifier <https://suhosin5.suhosin.org/stories/config | ||
| uration.html#suhosin-executor-disable-emodifier>`_ should be enabled. | ||
|
|
||
| You can also disable the warning using the :config:option:`$cfg['SuhosinDisableWarning']`. |
There was a problem hiding this comment.
Why remove this and not the entire block?
I'd say remove nearly everything (contents but not header)
and say that it was removed
| ]; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
@liviuconcioiu i do not remember how can we suggest to the user to remove a deprecated setting?
could we have an array somewhere?
There was a problem hiding this comment.
We can show a notice in footer, like Twig does for example.
And here is the code that should go in HomeController:
$deprecatedSettings = [
'SuhosinDisableWarning' => '6.0.0-dev',
];
foreach ($deprecatedSettings as $key => $version) {
if (array_key_exists($key, $this->config->settings)) {
$this->errors[] = [
'message' => sprintf(
__('Since phpMyAdmin %s: "$cfg[\'%s\']" is deprecated. You can safely remove it.'),
$version,
$key,
),
'severity' => 'notice',
];
}
}
There was a problem hiding this comment.
@liviuconcioiu I think this code is too generic. Each setting should have it's own message.
|
Makes sense — I'll keep the docs with a versionremoved note, trim the FAQ section instead of deleting it, and add the deprecation notice like @liviuconcioiu showed. Will update the PR. |
|
Addressed reviewer feedback:
All unit tests pass locally (5183 tests, 17280 assertions). |
somethingwithproof
force-pushed
the
fix/remove-suhosin-warning
branch
2 times, most recently
from
ba5b064 to
0a52adf
Compare
src/Controllers/HomeController.php
Outdated
| if (array_key_exists($key, $this->config->settings)) { | ||
| $this->errors[] = [ | ||
| 'message' => sprintf( | ||
| __('Since phpMyAdmin %s: "$cfg[\'%s\']" is deprecated. You can safely remove it.'), |
There was a problem hiding this comment.
Please, do not add this. It's out of the scope of this pull request.
docs/config.rst
Outdated
| A warning is displayed on the main page if Suhosin is detected. | ||
| .. deprecated:: 6.0.0 | ||
|
|
||
| You can set this parameter to ``true`` to stop this message from appearing. | ||
| This setting has been removed. The Suhosin extension is no longer | ||
| maintained, and the related warning has been removed from phpMyAdmin. |
There was a problem hiding this comment.
Please do not remove the original description. Leave it after the removed message.
somethingwithproof
force-pushed
the
fix/remove-suhosin-warning
branch
from
2606196 to
6a2f869
Compare
|
Rebased and addressed all three points:
Single clean commit now, only Suhosin-related changes. |
somethingwithproof
force-pushed
the
fix/remove-suhosin-warning
branch
from
6a2f869 to
2354c6b
Compare
|
Duplicate of #20167 |
|
Please avoid red creating pull requests. |
5 participants
Summary
HomeController.phpTest plan
phpstanandphpcsto confirm no regressionsFixes #19912