Problem/Motivation

When a deprecated setting has no replacement, Settings::get() currently triggers a deprecation warning on every call, even when the setting is not configured in settings.php.

This causes spurious deprecation warnings across unrelated test classes. For example, every kernel test that instantiates UpdateFetcher or SecurityAdvisoriesFetcher generates a deprecation warning for update_fetch_with_http_fallback (see related issue : #3591513: Deprecate the update_fetch_with_http_fallback setting), even though neither the module nor the site has that setting configured.

The deprecation warning for a setting with no replacement is only meaningful when the site actually has that setting configured in settings.php, since that is the only actionable signal for a site owner: "remove this setting from your settings.php". Triggering it unconditionally is misleading and pollutes test output.

Proposed resolution

Modify Settings::get() to only trigger the deprecation warning when at least one of the following conditions is true:

  • The setting has a replacement key (calling code must be updated to use the new key), or
  • The setting is actually present in storage (the site owner must remove it from settings.php).

Settings with no replacement that are not configured should not trigger any warning.

Remaining tasks

  • Patch Settings::get()
  • Add tests covering the new behavior

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork drupal-3592037

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

macsim created an issue. See original summary.

macsim opened merge request !15872

macsim’s picture

macsim commented
Status: Active » Needs review
macsim’s picture

macsim commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented

So ran the test-only locally and testGetNoDeprecationWhenNotConfiguredAndNoReplacement still passes that expected? Shouldn't both tests fail?

macsim’s picture

macsim commented

Thanks for having a look!
I think the confusion might come from which branch the tests were run against. testGetNoDeprecationWhenNotConfiguredAndNoReplacement does call Settings::get() (via assertNull()), and since it has no #[IgnoreDeprecations] attribute, any triggered deprecation would cause it to fail.
So the expected behavior is:

  • On main (without the patch): the test fails, because Settings::get() unconditionally triggers the deprecation.
  • On the issue branch (with the patch applied): the test passes, because no deprecation is triggered when the setting is not configured and has no replacement.

Could you confirm whether you ran the tests on main or on the issue branch?

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative 
1) /var/www/html/core/lib/Drupal/Core/Site/Settings.php:105
The fake_no_replacement setting is deprecated.

Triggered by:

* Drupal\Tests\Core\Site\SettingsTest::testGetNoDeprecationWhenNotConfiguredAndNoReplacement
  /var/www/html/core/tests/Drupal/Tests/Core/Site/SettingsTest.php:281

Sorry I got it twisted that one did fail without the fix. testGetDeprecationWhenConfiguredAndNoReplacement was the one that passes but I see now why

My mistake. This LGTM

alexpott made their first commit to this issue’s fork.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

For the PHPCS changes see https://www.drupal.org/project/coder/issues/3226902

  • catch committed f29f902d on 11.4.x 
    task: #3592037 Settings::get() should not trigger a deprecation for...

  • catch committed f0c052b9 on 11.x 
    task: #3592037 Settings::get() should not trigger a deprecation for...

  • catch committed bfdab165 on main 
    task: #3592037 Settings::get() should not trigger a deprecation for...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, 11.x and 11.4.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !15872

macsim’s picture

macsim commented

#3591513: Deprecate the update_fetch_with_http_fallback setting is in a Needs review status because we don’t know if it’s still ok or too late for a 11.4 deprecation. Feel free to give your opinion :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.