Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests

Gets us started with core/lib/Drupal/Core/Template/Loader/StringLoader.php
NW for core/lib/Drupal/Core/Template/TwigSandboxPolicy.php

Moving this to a separate task:

• Decide if we need to touch core/lib/Drupal/Core/Template/Attribute.php too. Only 'black-cat' and 'white-cat' and friends. Seems we could leave those.

Here I have tried to address comment #3.

Re: #5: Thanks, but you didn't touch any of the variables with "whitelist" in their names. :( That's the point of this issue.

Here's a patch that actually completes work for the scope of this issue.

One concern:

-    $this->whitelisted_prefixes = Settings::get('twig_sandbox_whitelisted_prefixes', [
+    $this->allowed_prefixes = Settings::get('twig_sandbox_allowed_prefixes', [

I don't see that setting documented anywhere, so I don't see where to change the example for this.

More concerning, perhaps for BC we need to read and honor the old value, too? And trigger a deprecation warning or something?

At least we'll need a CR that this setting (and these protected variables) have changed.

Draft CR: https://www.drupal.org/node/3162897

#3037436: [random test failure] Make QuickEditIntegrationTest more robust and fail proof strikes again. Re-queuing.

Re: #10 - yup, I thought so at #6.

Like so?

Whoops, I missed. There are 3 such settings. :( New patch coming soon.

Meanwhile, other googling:

https://www.drupal.org/project/twig_domquery mentions the legacy setting in its project description. I guess we can open an issue in there to ask them to move to the new setting once this lands?

Let's try this. ;)

Hope it's self-documenting from the code, but the approach in all 3 cases is we check for the new setting first. Only if it's empty to do we check for the old setting. If that's defined, we @trigger_error() but use it. If that's also empty, we provide the hard-coded defaults.

Updated the CR to document the settings changes.

Also, what's the policy for editing prior CRs in cases like this?

Theme system restricts which object methods can be accessed from templates

Thanks,
-Derek

Whoops, sorry

Re: #11 -- yeah, not clear WTF is going on there, either. ;) Maybe we should open an issue in https://www.drupal.org/project/issues/twig_extender about it?

1. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
   @@ -12,63 +12,96 @@
   +    // Flip the array so we can check using isset().
   +    $this->allowed_classes = array_flip($allowed_classes);
   ...
   +    // Flip the array so we can check using isset().
   +    $this->allowed_methods = array_flip($allowed_methods);
   ...
   +    $this->allowed_prefixes = $allowed_prefixes;
   

   To keep consistency, the last one should be:

   // Flip the array so we can check using isset().
   $this->allowed_prefixes = array_flip($allowed_prefixes);
   

   (Including the comment)

2. Hope it's self-documenting from the code, but the approach in all 3 cases is we check for the new setting first. Only if it's empty to do we check for the old setting. If that's defined, we @trigger_error() but use it. If that's also empty, we provide the hard-coded defaults.

   +1 to the logic.

3. Also, what's the policy for editing prior CRs in cases like this?

   Not found, maybe we can ask it in the #documentation channel.

4. Re #17, +1 to open one new issue there, no obvious harm to have one more issue :)

NW for the point 1, otherwise it's RTBC to me.

Thanks!

      if (!empty($legacy_allowed_prefixes)) {
        @trigger_error('The "twig_sandbox_whitelisted_prefixes" setting is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use "twig_sandbox_allowed_prefixes" instead. See https://www.drupal.org/node/3162897.', E_USER_DEPRECATED);
        $allowed_prefixes = $legacy_allowed_prefixes;
      }
      // Provide a default.
      $allowed_prefixes = [
        'get',
        'has',
        'is',
      ];

In addition to #18.1, missing else

Thanks for the review!

However, please look at the full code for core/lib/Drupal/Core/Template/TwigSandboxPolicy.php:

    // If the method name starts with an allowed prefix, allow it. Note:
    // strpos() is between 3x and 7x faster than preg_match() in this case.
    foreach ($this->allowed_prefixes as $prefix) {
      if (strpos($method, $prefix) === 0) {
        return TRUE;
      }
    }

We don't want to array_flip(), or we'll break this. I *almost* added a comment to that effect in #14, but thought it might be considered scope creep. ;)

Whoops, x-post! #19 is definitely a bug. ;)

1 moment....

#20 is right, ignore #18.1 please.

1. +++ b/core/lib/Drupal/Core/Template/Loader/StringLoader.php
   @@ -8,7 +8,7 @@
   - * This loader is intended to be used in a Twig loader chain and whitelists
   + * This loader is intended to be used in a Twig loader chain and only loads
   
   +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
   @@ -12,63 +12,98 @@
   - * objects can do by whitelisting certain classes, method names, and method
   + * objects can do by only loading certain classes, method names, and method
   

   whitelists -> only loads, whitelisting -> only loading, good.

2. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
   @@ -12,63 +12,98 @@
   -   * An array of whitelisted methods in the form of methodName => TRUE.
   +   * An array of allowed methods in the form of methodName => TRUE.
   ...
   -  protected $whitelisted_methods;
   +  protected $allowed_methods;
   

   whitelisted methods -> allowed methods, good

3. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
   @@ -12,63 +12,98 @@
   -  protected $whitelisted_prefixes;
   +  protected $allowed_prefixes;
   ...
   +    $this->allowed_prefixes = $allowed_prefixes;
   
   @@ -85,20 +120,20 @@ public function checkPropertyAllowed($obj, $property) {}
   +    foreach ($this->allowed_prefixes as $prefix) {
   

   whitelisted prefixes -> allowed prefixes, good

4. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
   @@ -12,63 +12,98 @@
   -  protected $whitelisted_classes;
   +  protected $allowed_classes;
   

   whitelisted classes -> allowed classes, good

Agreed, that'd be better.
#3151093: Replace use of whitelist/blacklist in \Drupal\Core\Security\RequestSanitizer and its test has \Drupal\Core\Site\Settings::handleDeprecations().
Is that how you want that to work?
Do you want to split that part out to a separate issue?
Or just land that as-is and we'll build on it here?

Thanks!
-Derek

Proof of concept for #3163226-13: Add the ability to deprecate a Settings name

Here's #22 cleaned up / simplified, plus #3163226-13.

+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -62,9 +62,10 @@ public function __construct() {
+    $allowed_prefixes = Settings::get('twig_sandbox_allowed_prefixes', [

BTW, missing $this->allowed_prefixes = $allowed_prefixes;

Good catch, thanks! 2 new patches:

1. 3151094-29.with-3163226-21.patch will hopefully run / pass tests now. ;) It's fixed version of #27 plus the latest from #3163226-21: Add the ability to deprecate a Settings name
2. 3151094-29.after-3163226.do-not-test.patch is just the changes for this issue once #3163226 is committed. "do-not-test" since it won't apply until that lands.

New patches based on changes at #3163226: Add the ability to deprecate a Settings name

1. Instead of extending the existing test, add new test plumbing for testing real deprecations.
2. Settings::$deprecatedSettings is now keyed by legacy name.

Here's a do-not-test that will only apply after #3163226-37 is committed and a version with #3163226-37 included and drupalci.yml change to only run the new test.

Also cleaning up the summary + remaining tasks now that this is nearly complete.

Thanks,
-Derek

Open questions:

1. Given that #3163226: Add the ability to deprecate a Settings name is already testing all 6 combinations of settings.php and Settings::get(), and ensuring that we always get the correct value returned and that the deprecation message is generated in the 5 expected cases, do we even need to be testing the real deprecations at all?
2. If so, do we have to test all 6 combos for every new deprecated setting? :( Or can we have a simple test for real deprecations? What do we actually care about testing for these real deprecations?

Testing the real deprecations seems a bit like busy-work to me. If we trust the new API, can't we just use it without re-testing it for every usage? It's more churn when we deprecate, and more stuff to cleanup at the next major.

Thoughts?

Thanks!
-Derek

As in, can we just do this, instead?

Re #31, Good question! How about the following, just to test that it works, no data provider -- triggered expected deprecation message(s).

  /**
   * Tests legacy twig_sandbox_* settings.
   *
   * @group legacy
   *
   * @expectedDeprecation The "twig_sandbox_whitelisted_classes" setting is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use "twig_sandbox_allowed_classes" instead. See https://www.drupal.org/node/3162897.
   * @expectedDeprecation The "twig_sandbox_whitelisted_methods" setting is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use "twig_sandbox_allowed_methods" instead. See https://www.drupal.org/node/3162897.
   * @expectedDeprecation The "twig_sandbox_whitelisted_prefixes" setting is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use "twig_sandbox_allowed_prefixes" instead. See https://www.drupal.org/node/3162897.
   */
  public function testLegacyTwigSandboxSettings(): void {
    $settings = <<<'EOD'
<?php
$settings['twig_sandbox_whitelisted_classes'] = ['a', 'b'];
$settings['twig_sandbox_whitelisted_methods'] = ['aFoo', 'bBar'];
$settings['twig_sandbox_whitelisted_prefixes'] = ['aPrefix', 'bPrefix'];
EOD;
    $class_loader = NULL;
    $vfs_root = vfsStream::setup('root');
    $sites_directory = vfsStream::newDirectory('sites')->at($vfs_root);
    vfsStream::newFile('settings.php')
      ->at($sites_directory)
      ->setContent($settings);
    Settings::initialize(vfsStream::url('root'), 'sites', $class_loader);
    $this->assertEquals(['a', 'b'], Settings::get('twig_sandbox_whitelisted_classes'));
    $this->assertEquals(['a', 'b'], Settings::get('twig_sandbox_allowed_classes'));
    $this->assertEquals(['aFoo', 'bBar'], Settings::get('twig_sandbox_whitelisted_methods'));
    $this->assertEquals(['aFoo', 'bBar'], Settings::get('twig_sandbox_allowed_methods'));
    $this->assertEquals(['aPrefix', 'bPrefix'], Settings::get('twig_sandbox_whitelisted_prefixes'));
    $this->assertEquals(['aPrefix', 'bPrefix'], Settings::get('twig_sandbox_allowed_prefixes'));
  }

Even more, remove assertions, only care deprecation message(s)

    $this->assertEquals(['a', 'b'], Settings::get('twig_sandbox_whitelisted_classes'));
    $this->assertEquals(['a', 'b'], Settings::get('twig_sandbox_allowed_classes'));
    $this->assertEquals(['aFoo', 'bBar'], Settings::get('twig_sandbox_whitelisted_methods'));
    $this->assertEquals(['aFoo', 'bBar'], Settings::get('twig_sandbox_allowed_methods'));
    $this->assertEquals(['aPrefix', 'bPrefix'], Settings::get('twig_sandbox_whitelisted_prefixes'));
    $this->assertEquals(['aPrefix', 'bPrefix'], Settings::get('twig_sandbox_allowed_prefixes'));

Yay, blocker is in! This can proceed. Queued #32 for a test run.

@dww, could you add the test suggested in #33 and #34 if you agree?

Missing test for the newly added deprecated settings, I am afraid I could not RTBC this.

I'm waiting for a core committer to answer #31.1. As I wrote there, I believe if we trust the tests for the Settings::$deprecatedSettings array / API (as we should), then adding "real" deprecation tests for every deprecated setting seems like pointless churn to me. Once we have tests for a given form element, we don't re-test that form element every time we use it. I don't see why we need anything more than #32.

Tagging for framework manager review.

Of course if they want more, we can add them. Not sure if #34/ #35 is enough, or we should have a dataProvider, or what. If they want real deprecation tests, we need clarity on their preferred approach, and an answer to #31.2: do we need to test all 6 possible cases for each setting? If not, which subset is enough? Etc.

Thanks,
-Derek

Re: #38: Groovy, thanks for clarifying!

Re: #33:

+  Since symfony/phpunit-bridge 5.1: Using "@expectedDeprecation" annotations in tests is deprecated, use the "ExpectDeprecationTrait::expectDeprecation()" method instead.

;)

With an eye towards #3151093: Replace use of whitelist/blacklist in \Drupal\Core\Security\RequestSanitizer and its test I'm going with a @dataProvider approach.

Do we want each setting in its own case, or can we group them to test all deprecated twig_sandbox_* settings in 1 case like I'm doing here?

Thanks,
-Derek

Whoops, comment spacing isn't quite right. ;)

This should be better.

Before:

% phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,md,yml SettingsTest.php
------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
------------------------------------------------------------------------------------
  73 | ERROR | [x] Doc comment short description must end with a full stop
  98 | ERROR | [ ] Description for the @return value is missing
 160 | ERROR | [x] There must be exactly one blank line before the tags in a doc
     |       |     comment
 163 | ERROR | [ ] Parameter tags must be defined first in a doc comment
 219 | ERROR | [x] There must be exactly one blank line before the tags in a doc
     |       |     comment
 292 | ERROR | [x] Additional blank lines found at end of doc comment

After:

% phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,md,yml SettingsTest.php
------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
------------------------------------------------------------------------------------
  73 | ERROR | [x] Doc comment short description must end with a full stop
  98 | ERROR | [ ] Description for the @return value is missing
 164 | ERROR | [ ] Parameter tags must be defined first in a doc comment

(these 3 are pre-existing).

If we move each setting into a separate case from the dataProvider, we can simplify some of these even further and shrink the patch.

Also fixing a typo in a @param comment s/stinrg/string/

And if we want to get really clever, we can auto-populate providerTestRealDeprecatedSettings() via Settings::$deprecatedSettings itself. ;) Then we never have to touch this file and we know that all deprecated settings will be tested.

Probably this will be considered too much magic and we'll have to stick with #43, but I'm uploading for comparison / consideration, just in case. ;)

If i change $this->addExpectedDeprecationMessage($expected_deprecation); to $this->expectDeprecationMessage($expected_deprecation);, it won't pass. wondering what's the difference between them. If you know, would you mind to share? Thanks!

I confess to not fully understanding. I can say that addExpectedDeprecationMessage() is from Drupal\Tests\Traits\ExpectDeprecationTrait. Although, yikes, apparently that's on its way out, too: #3157434: Deprecate \Drupal\Tests\Traits\ExpectDeprecationTrait in favour of \Symfony\Bridge\PhpUnit\ExpectDeprecationTrait ;)

However, expectDeprecationMessage() is native to PHPUnit itself.

My (limited) understanding is that the Symfony PHPUnit bridge catches all deprecation warnings thrown and has its own handling of them so we can ignore a huge # of deprecations that we can't do anything about until we stop supporting older versions of PHP, PHPUnit, Symfony, etc, etc. Therefore, we need to use that plumbing to inspect deprecations, since if we ask PHPUnit itself to expect something, it'll never be seen. Or something. ;)

I'm sure @alexpott can completely school us on the details. ;)

Thanks @dww, I do not fully get #46, but after fixing/rerolling the patch for the issue you shared, I think we can try the following changes:

--- a/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
+++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
@@ -3,9 +3,9 @@
 namespace Drupal\Tests\Core\Site;
 
 use Drupal\Core\Site\Settings;
-use Drupal\Tests\Traits\ExpectDeprecationTrait;
 use Drupal\Tests\UnitTestCase;
 use org\bovigo\vfs\vfsStream;
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
 
 /**
  * @coversDefaultClass \Drupal\Core\Site\Settings
@@ -206,7 +206,7 @@ public function testFakeDeprecatedSettings(array $settings_config, string $setti
     $instance_property->setValue($deprecated_settings);
 
     if ($expect_deprecation_message) {
-      $this->addExpectedDeprecationMessage($deprecated_setting['message']);
+      $this->expectDeprecation($deprecated_setting['message']);
     }
 
     Settings::initialize(vfsStream::url('root'), 'sites', $class_loader);
@@ -301,7 +301,7 @@ public function testRealDeprecatedSettings(string $legacy_setting, string $expec
       ->at($sites_directory)
       ->setContent($settings_file_content);
 
-    $this->addExpectedDeprecationMessage($expected_deprecation);
+    $this->expectDeprecation($expected_deprecation);

expectDeprecation() (or similar) is more accurate/suitable than addExpectedDeprecationMessage(), IMO.

Thanks for reviewing. However, #47 is out of scope here. This test was already using Drupal\Tests\Traits\ExpectDeprecationTrait before I started touching it at #3163226: Add the ability to deprecate a Settings name. I don't see any existing references to Symfony\Bridge\PhpUnit\ExpectDeprecationTrait anywhere in the 9.1.x branch (yet). Converting from our custom version of this trait to the Symfony\Bridge version should happen at #3157434: Deprecate \Drupal\Tests\Traits\ExpectDeprecationTrait in favour of \Symfony\Bridge\PhpUnit\ExpectDeprecationTrait, not here. If that lands first, it should make those changes itself to this test as it exists in the 9.1.x branch. If/when that happens, then we can re-roll this to use expectDeprecation() on the new instances. But until then, we should definitely leave this as-is.

Cheers,
-Derek

Thanks @dww.

Then #44 is RTBC to me, one test covers all real deprecated settings magically. Deprecated settings added later on do not need to add tests anymore. However, #33 + #34 is an alternative to me still, so uploading a patch based on it, for committer's reference.

+++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
@@ -158,6 +158,9 @@ public function testGetInstanceReflection() {
+   * @see self::testRealDeprecatedSettings()
+   * @see self::providerTestRealDeprecatedSettings()

@@ -213,6 +216,10 @@ public function testFakeDeprecatedSettings(array $settings_config, string $setti
+   * Note: Tests for real deprecated settings should not be added here.
+   *
+   * @see self::providerTestRealDeprecatedSettings()
+   *

Sorry the above should be removed from #49

IMHO #44 is the best option, and will be zero work for subsequent issues. Re-uploading that, since I think we both agree that's RTBC, whereas #34 / #35 / #50 is an alternative / 2nd choice if committers prefer. Honestly, I'd consider #50 the third choice. If #44 is too much magic, I'd rather go with #43 than #50.

So I'm re-uploading #44 to be the last patch in the issue for both testbot and committer review.
I also put all of this into the summary as the final remaining task.

Thanks,
-Derek

Thanks @alexpott. Added @runInSeparateProcess

Follow up filed

Not sure we should add it to the class or add it to all methods in SettingsTest, as per the official docs, it does not tell that the annotation is applicable to class explicitly.

Comments are welcome over there.

Re: #52: Thanks for making a decision. You wanted more declarative than #51 / #44 -- spell it out, use less magic. ;) Fair enough. More work for each deprecated setting, but we've only got 4 of them at this point. Unlike other kinds of deprecations, there are unlikely to be a huge # of these.

I'm slightly bummed we'll have to repeat all of this in each test:

+++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
@@ -272,4 +272,31 @@ public function providerTestFakeDeprecatedSettings(): array {
+    $class_loader = NULL;
+    $vfs_root = vfsStream::setup('root');
+    $sites_directory = vfsStream::newDirectory('sites')->at($vfs_root);
+    vfsStream::newFile('settings.php')
+      ->at($sites_directory)
+      ->setContent($settings);
+    Settings::initialize(vfsStream::url('root'), 'sites', $class_loader);

Which is why I was advocating for #43 with a @dataProvider as the alternative. That still has the same desired effect:

It's testing that if you have the setting in your setting.php you get the expected deprecation.

That's what all 3 approaches do. ;)

Anyway, if you're happy with copy/pasta on this, we can move on. Just trying to better understand your thinking / preferences to calibrate my own for future work.

Re: #54: Thanks for opening that! Now that I know PHPUnit has this plumbing, I was thinking the same for this whole test class. Commented there with a patch. ;)