Add the ability to deprecate a Settings name

+100 to this. Will be much cleaner and better.

#3151093: Replace use of whitelist/blacklist in \Drupal\Core\Security\RequestSanitizer and its test proposed a static private Settings::handleDeprecations() method that directly had all the knowledge of deprecated settings and therefore core/lib/Drupal/Core/Site/Settings.php had to use Drupal\Core\Security\RequestSanitizer to get constants for the setting keys. :( I assume we don't want to continue having any part of core with deprecated settings directly adding its classes and code to this class, right?

Should we dispatch an event for modules to register deprecated settings?

Some other mechanism to advertise the deprecations? If so, what? I assume we don't want to introduce a hook for this. ;)

We want to do this once, not at each get() call site.

Preference on the API to implement here?

Thanks!
-Derek

Oh right, but of course! Please forgive my 2am brain. ;) We don't have a DB yet, much less a container, much less "modules"...

In fact, the "Settings are really low-level / early that shouldn't depend on other things" thought is what caused my eyebrows to raise at this in the first place:

use Drupal\Core\Security\RequestSanitizer;

But then my "solution" promptly forgot my concern. ;)

Should we really be including other classes to get constants for this stuff, or should we say we just use raw strings for everything?

How about something like this?

Obviously still needs tests, but would love feedback on the proposed API + approach.

Thanks!
-Derek

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -191,4 +193,50 @@ public static function getApcuPrefix($identifier, $root, $site_path = '') {
+    return [
+      [
+        'legacy' => 'sanitize_input_whitelist',
+        'replacement' => 'sanitize_input_safe_keys',
+        'message' => 'The settings key "sanitize_input_whitelist" is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. Use Drupal\Core\Security\RequestSanitizer::SANITIZE_INPUT_SAFE_KEYS instead.',
+      ],
+    ];

A) For example, should this array be keyed by the deprecated setting name, so this looks like:

  return [
    'sanitize_input_whitelist' => [
      'replacement' => 'sanitize_input_safe_keys',
      'message' => 'The settings key "sanitize_input_whitelist" is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. Use Drupal\Core\Security\RequestSanitizer::SANITIZE_INPUT_SAFE_KEYS instead. See https://www.drupal.org/node/xyz',
    ],
  ];

B) Any value in splitting out the parts of the message that vary into separate values and constructing the message dynamically?

E.g.

  return [
    'sanitize_input_whitelist' => [
      'replacement' => 'sanitize_input_safe_keys',
      'version_deprecated' => 'drupal:9.1.0'
      'version_removed' => 'drupal:10.0.0'
      'see_url' => 'https://www.drupal.org/node/xyz',
    ],
  ];

And then the caller does something like:

@trigger_error(sprintf('The settings key "%s" is deprecated in %s and will be removed in %s. Use "%s" instead. See %s.', $legacy, $deprecation['version_deprecated'], $deprecation['version_removed'], $deprecation['replacement'], $deprecation['see_url']));

Maybe it'd be useful to be able to call this function and compare the versions directly or something? But then you can't have stuff like "Use Drupal\Core\Security\RequestSanitizer::SANITIZE_INPUT_SAFE_KEYS instead".

?

Thoughts?

Thanks!
-Derek

Sorry, was hasty uploading #5 as WIP to get feedback on the approach.

Here's a version with (initial) test coverage and no PHP fatal errors. ;)

Probably this is too unholy using Settings::getDeprecatedSettings() to power the dataProvider. ;) But it works for now.

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -191,4 +193,57 @@ public static function getApcuPrefix($identifier, $root, $site_path = '') {
+  public static function getDeprecatedSettings(): array {

Yeah, I don't think we should be making this public just for testing sake, but not sure if you can use reflection in the test to execute it

Other than that, looks like a good approach to me

Based on very fruitful Slack chat with @alexpott, here's a new approach. Pasting some comments for posterity:

alexpott: I’ve ummed and ahhed on a message versus splitting it up
alexpott: I think it is not worth it
dww: Do you care if it's all in the array, or if we should key on the deprecated setting name? (#6A)
alexpott: So I think I prefer what’s there over 6A
dww: Seemed a bit yucky to add the public initializeDeprecations() method in #7, but I'm not clear how to make this work given that the unit test never calls ::initialize(). Seems the only other approach would be putting something in ::get() to track if deprecations were initialized and if not, to handle them. But that seems yucky, too, since it's more comparisons and such in a relatively critical path (no?).
alexpott: So I think we should make getDeprecatedSettings a static property on the class and not a method
alexpott: Look at \Drupal\Tests\Core\Site\SettingsTest::testConfigDirectoriesBcLayer() on 8.9.x for how to test this
alexpott: Also once the list is a static property we can use reflection to alter it for tests
alexpott: In this issue we should add an empty list in the real class.

And lo. ;) Probably easier to review the patch than the interdiff, since a lot changed. ;)

p.s. #3037436: [random test failure] Make QuickEditIntegrationTest more robust and fail proof strikes again. :(

+++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
@@ -150,4 +151,80 @@ public function testGetInstanceReflection() {
+   * @param array $deprecated_settings

s/settings/setting/ (I missed this when renaming it to be a flat array, not nested).

Fix #11.
Big cleanup of summary now that we know what we're doing. ;)

This is looking nice - should we add at least one use-case for the new API?

With an eye towards expanding this test coverage in #3151093: Replace use of whitelist/blacklist in \Drupal\Core\Security\RequestSanitizer and its test and #3151094: Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests , it occurs to me that we probably don't want to clobber the default Settings::$deprecatedSettings from the real class, we're going to want to append to it (if the test case wants us to). Something like this.

This would allow the provider to also test the real deprecation messages once one of those issues lands.

It's slightly unholy, since I learned (the hard way) that in raw Unit tests, there's no completely clean reset for each test case. Things we do in each case leak into the others. So we get into trouble pretty quick once we start adding more test cases. So I'm adding a setUpBeforeClass() to capture the original value of Settings::$deprecatedSettings before any tests run, so we can always reset to a clean slate for each test case.

Thoughts?

Thanks!
-Derek

Sorry, wrong version of the interdiff. The one in #14 is very bogus. Here's the real one.

Hah, #14 x-post'ed with #13. ;)

Yes, I'm already thinking about how this is going to work when we circle back to the other issues.

However, @alexpott requested that this issue only adds the plumbing and test coverage, but not any of the actual deprecated settings:

alexpott: In this issue we should add an empty list in the real class.

I'm happy to roll a combined patch for one of the child issues showing how this would work once we expand it or something?

Showing the new API in action: #3151094-27: Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests

Crediting @cburschka since this was partly inspired by and based on their work at #3151093: Replace use of whitelist/blacklist in \Drupal\Core\Security\RequestSanitizer and its test
Also crediting Alex and Lee for reviews and invaluable insights via Slack.

More POC up at #3151093-26: Replace use of whitelist/blacklist in \Drupal\Core\Security\RequestSanitizer and its test for the interested reader.

+++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
@@ -150,4 +151,82 @@ public function testGetInstanceReflection() {
+   * Tests deprection messages and values when using deprecated settings.

Typo? deprection -> depreciation, not sure.

Yup, typo. Should be "deprecation".

Thanks,
-Derek

+++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
@@ -150,4 +176,88 @@ public function testGetInstanceReflection() {
+    $deprecated_settings = self::$originalDeprecatedSettings;
+    if ($update_deprecated_settings) {
+      $deprecated_settings[] = $deprecated_setting;
+    }
+    $instance_property->setValue($deprecated_settings);

Not sure the logic here. If $originalDeprecatedSettings is non-empty initially, it triggers deprecated message(s) too, maybe $this->addExpectedDeprecationMessage($deprecated_setting['message']); does the magic making the test passes.

Suggestion (partially):

-    $deprecated_settings = self::$originalDeprecatedSettings;
-    if ($update_deprecated_settings) {
-      $deprecated_settings[] = $deprecated_setting;
-    }
+    $deprecated_settings = [];
+    $deprecated_settings[] = $deprecated_setting;
     $instance_property->setValue($deprecated_settings);

+++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
@@ -150,4 +176,88 @@ public function testGetInstanceReflection() {
+        TRUE,
...
+        TRUE,

Missing coverage/test case/data provider for "FALSE" if we go for this or not a case if the test gets rewritten following #22

@jungle: Thanks for the reviews!

Re: #22 -

If $originalDeprecatedSettings is non-empty initially, it triggers deprecated message(s) too

Not exactly. If $originalDeprecatedSettings is non-empty, it means another issue that's adding real deprecated settings has landed (either #3151093: Replace use of whitelist/blacklist in \Drupal\Core\Security\RequestSanitizer and its test or #3151094: Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests). That doesn't automatically trigger deprecation warnings. Only if settings.php contains one of the deprecated settings. The whole point of the complications in this issue starting from comment #14 is to make this test plumbing easier to extend in subsequent issues. Further confusing matters is that I uploaded the wrong interdiff and patch at comment #14. :( Sorry! At least the patch at #21 is actually valid and what I'm intending, so that's progress. ;)

Maybe this test is trying to be too clever and reusing things in a confusing way. The $deprecated_setting array passed from the @dataProvider is defining 'legacy' and 'replacement' so we know what settings to call Settings::get() on, and the 'message' for the deprecation message we're expecting. If the $update_deprecated_settings bool is TRUE, it means we also want to add that array to the Settings::$deprecatedSettings protected static array so that the setting is now considered deprecated. So setting that bool to TRUE means "alter the active array of deprecated settings in the real Settings object to test fake deprecations". That's all we can do in this issue, since @alexpott just wants the scope here to be the plumbing, not any actual new deprecations:

alexpott: In this issue we should add an empty list in the real class

Re: #23 - This is why core issue scoping is sometimes a PITA. 😉 We can't test the FALSE case here because at this point there are *no* default / real deprecations to try to test. All we can do is test "fake" deprecations. If you want to see it working with both TRUE and FALSE, see #3151094-29: Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests.

So we either commit this as-is, or go back to #12 for now, then do everything else in here in one of the blocked issues to re-factor the plumbing to allow testing real deprecations. I don't know what's better. IMHO, I'd rather get the plumbing "right" in here, knowing those other issues are basically ready as soon as this lands.

Thanks again,
-Derek

To hopefully help things, here's the interdiff from what I actually uploaded as "#13" (in comment #14) vs. #21.

Also attaching a real interdiff between #12 and #21 to be more clear about the additional complications I'm referring to in #24 that should either happen now, or be moved to a child issue once we have real deprecations we're also trying to test.

Thanks/sorry,
-Derek

@dww, many thanks for the detailed explanations. #21 could test against the real $originalDeprecatedSettings, while #12 can't, but to me, maybe, without test coverage of the real $originalDeprecatedSettings is fine, and #12 is easier for my mind to understand.

So both #21 and #12 are RTBC to me.

If we go for #12, remember to fix the typo "deprection -> depreciation" mentioned in #20 on committing.

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -27,6 +27,20 @@
+   *   - 'replacement': The new name for the setting.

Should this be optional? Is there a use case for deprecating settings with no replacement, or a more complex replacement that means just copying the old setting to the new is not viable? (where the deprecating code would have to provide BC by itself, I guess)

Or should we leave this until we have a need for it?

Also, this is only for core, right? Would contrib want to be able to add deprecations here? Should we have a static method Settings::addDeprecation() that adds to the array? (this could also be used by the test)

1. Addressing #28.2, 3 and 4.
2. Does #28.1 mean that the way #12 does is preferred?

Thanks for all the reviews and insights, everyone!

#28:

1. Okay, that all sounds good. Happy for this to be specifically testing the fake deprecation (thoroughly) and handling real deprecation testing in other test/provider. +1
2. @runInSeparateProcess -- who knew!?! ;) TIL. Much better! Thanks.
3. 👍
4. Done here. We now test all 6 possibilities: Get old name vs. new name (2) X 3 configs (only legacy, only replacement, both defined).

#29: Agreed. Let's KISS until we know we need something else (YAGNI).

#30: Right, that wouldn't fly. By the time contrib could call Settings::addDeprecation() we'd have already called Settings::initialize() and we wouldn't notice the additions.

#31: 👍

#32: Thanks for moving this forward!

#33: I didn't fully understand the 2nd half of your comment. But now that I've fixed this test and provider to cover all 6 combos, I see that the case where settings.php only includes the new value, but the test calls Settings::get() on the legacy value is not triggering the deprecation message as expected. I'm uploading a clean version of the latest work to fix everything else as do-not-test, and another copy with drupalci configured to only run the new test. That should fail. I'll fix Settings.php itself next, and hopefully won't have to change the dataProvider at all and we should see it passing. Stay tuned. ;)

Thanks!
-Derek

p.s. Running phpcs locally, I noticed this on core/lib/Drupal/Core/Site/Settings.php :

  39 | ERROR | The @var tag must be the first tag in a member variable comment

Not sure why the bot's not flagging that. But fixed here.

Yay, this works. Phew! ;) No changes to the dataProvider and now all 6 cases pass locally.

Here's a full patch (hopefully RTBC) and an instant-gratification version that will only run the new test. Both should be green.

Oh, poo. We missed "Plus we can use scalar typehints." from #28.3

Fixed here, along with return types for both the new test and provider. *This* should be RTBC. ;)

Thanks @dww for the new patch!

1. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -84,6 +97,11 @@ public function __sleep() {
      public static function get($name, $default = NULL) {
   +    // If the caller is asking for the value of a deprecated setting, trigger a
   +    // deprecation message about it.
   +    if (isset(self::$deprecatedSettings[$name])) {
   +      @trigger_error(self::$deprecatedSettings[$name]['message'], E_USER_DEPRECATED);
   +    }
   

   ++100++ to this

2. +++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
   @@ -150,4 +151,130 @@ public function testGetInstanceReflection() {
   +    // This is the deprecated setting used by all cases for this test method.
   +    $deprecated_setting = [
   +      'replacement' => 'happy_replacement',
   +      'message' => 'The settings key "deprecated_legacy" is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. Use "happy_replacement" instead. See https://www.drupal.org/node/3163226.',
   +    ];
   

   Use case of no replacement does not get tested, for example:

   $deprecatedSettings = [
     'only_deprecated_legacy_no_replacement' => [
       'message' = 'The settings key "only_deprecated_legacy_no_replacement" is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. No replacement. See https://www.drupal.org/node/3163226.',
   ]

   • No replacement key, or it's empty
   • deprecated_legacy -> only_deprecated_legacy_no_replacement
   • Use "happy_replacement" instead -> No replacement

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -27,6 +27,19 @@
+   *   - 'replacement': The new name for the setting.

In addition to #38.2 here could be - 'replacement': (Optional) The new name for the setting.

Thanks for reviewing, @jungle!

Re: #38.2 and #39:
See #27 and @alexpott's answer in #29.

All existing cases of core needing to deprecate a setting involve a replacement. If that ever changes, we can change this. Until then, YAGNI. ;)

Back to NR.

Thanks,
-Derek

Sorry, @dww, got it now. #37 is green, no CS violations, I think #33 is addressed. Setting back to RTBC.

p.s. Running phpcs locally, I noticed this on core/lib/Drupal/Core/Site/Settings.php :

39 | ERROR | The @var tag must be the first tag in a member variable comment

Not sure why the bot's not flagging that. But fixed here.

  39 | ERROR   | The @var tag must be the first tag in a member variable comment (Drupal.Commenting.VariableComment.VarOrder)

BTW, the rule Drupal.Commenting.VariableComment.VarOrder is excluded ATM, see core/phpcs.xml.dist#L83

@jungle Thanks!!
Re: #41: 👍🙏🎉
Re: #42: Duly noted, thanks.

Meanwhile, I uploaded a PoC for testing real deprecations at #3151094-30: Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests. Comments welcome over there.

Thanks again,
-Derek

Since $expect_deprecation_message defaults to TRUE, we don't need to set it manually in each provider case, right?

Also, the test failure from #34 is interesting. All you see from the testbot is:

There was 1 failure:

1) Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only replacement defined, get legacy" ('

If you run it locally, you see the full thing:

There was 1 failure:
1) Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only replacement defined, get legacy" ('<?php\n$settings['happy_repla...'new';', 'deprecated_legacy', 'new', true)
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  The settings key "deprecated_legacy" is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. Use "happy_replacement" instead. See https://www.drupal.org/node/3163226.
+

Note that the d.o version dies as soon as it sees the <?php ;)

This got me thinking that maybe it'd be safer/clearer/simpler for the provider to pass an array of settings config, and let the test create the actual settings file content dynamically. Then you can see more useful into in each test case:

Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only legacy defined, get legacy" (array('old'), 'deprecated_legacy', 'old')' started
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only legacy defined, get legacy" (array('old'), 'deprecated_legacy', 'old')' ended
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only legacy defined, get replacement" (array('old'), 'happy_replacement', 'old')' started
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only legacy defined, get replacement" (array('old'), 'happy_replacement', 'old')' ended
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Both legacy and replacement defined, get legacy" (array('old', 'new'), 'deprecated_legacy', 'new')' started
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Both legacy and replacement defined, get legacy" (array('old', 'new'), 'deprecated_legacy', 'new')' ended
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Both legacy and replacement defined, get replacement" (array('old', 'new'), 'happy_replacement', 'new')' started
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Both legacy and replacement defined, get replacement" (array('old', 'new'), 'happy_replacement', 'new')' ended
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only replacement defined, get legacy" (array('new'), 'deprecated_legacy', 'new')' started
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only replacement defined, get legacy" (array('new'), 'deprecated_legacy', 'new')' ended
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only replacement defined, get replacement" (array('new'), 'happy_replacement', 'new', true)' started
Test 'Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only replacement defined, get replacement" (array('new'), 'happy_replacement', 'new', true)' ended

So here's a new full patch with both of those changes. Plus a fail-only where I intentionally broke the last case so we see if the testbot can handle the new failure output. ;) Interdiffs from #37 and between these two. For the broken-test-only, we should see what it actually was:

There was 1 failure:

1) Drupal\Tests\Core\Site\SettingsTest::testFakeDeprecatedSettings with data set "Only replacement defined, get replacement" (array('new'), 'happy_replacement', 'new', true)
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  The settings key "deprecated_legacy" is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. Use "happy_replacement" instead. See https://www.drupal.org/node/3163226.
+

Thoughts?

Thanks,
-Derek

Re: #46: Thanks! CR mostly looked great. I did a few edits for minor typos and formatting.

Re: #30 and contrib: Everything we've said so far is true. ;) But now that we're checking deprecations on Settings::get(), we could potentially tell contrib to listen to the kernel.request event, call Settings::addDeprecation() then, and know that any calls to Settings::get() after that would be handled normally. So contrib couldn't deprecate any settings used prior to that event being dispatched, but that seems reasonable. ;) Then they wouldn't need to replicate their call-site code (like the mess I wrote at #3151094-22: Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests), and could handle it more "cleanly"? Then again, the plumbing required to make this work would need ~30 lines of code and yml config, so that's probably worse than doing it yourself when you call Settings::get(). 😉/shrug. Probably this point should be continued in a follow-up if there's interest. What we have here now is good enough to unblock the other issues, so let's get it in as-is, and expand it later if we decide we want to start doing contrib the favor...

Thanks!
-Derek

Should we mention this at https://www.drupal.org/core/deprecation ?

Re: #50/ #51:

Done: https://www.drupal.org/core/deprecation#how-core-settings

Also updated the CR to explain the Settings::get() behavior with both legacy and replacement names.

Cheers,
-Derek