Problem/Motivation

The following settings were deprecated in Drupal 9 for removal in Drupal 10:

  • sanitize_input_whitelist
  • twig_sandbox_whitelisted_classes
  • twig_sandbox_whitelisted_methods
  • twig_sandbox_whitelisted_prefixes

Steps to reproduce

Proposed resolution

Remove all references to them.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3269149-2.patch4.09 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
FileSize
4.09 KB

Status: Needs review » Needs work

The last submitted patch, 2: 3269149-2.patch, failed testing. View results

longwave’s picture

Status: Needs work » Needs review

Unrelated fail in a JS test.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks great and nice to see deprecatedSettings empty after this lands.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Given this is private - imo we should remove it entirely. Less code runs on every Settings::get()

catch’s picture

Status: Needs work » Needs review

@alexpott don't we need to leave it in for the next setting that gets deprecated?

alexpott’s picture

We can add it back if and when we do. It's all private therefore there's no harm in removing or adding back. Maintaining an empty private static seems unnecessary.

catch’s picture

Yeah I'm not sure, we added an API (albeit tiny) and test coverage for deprecated settings. How do we avoid having to do all the same work again next time someone wants to deprecate one? Could we short circuit the logic and skip the test instead?

Here's the relevant code not in the patch we'd be removing:

  /**
   * Handle deprecated values in the site settings.
   *
   * @param array $settings
   *   The site settings.
   *
   * @see self::getDeprecatedSettings()
   */
  private static function handleDeprecations(array &$settings): void {
    foreach (self::$deprecatedSettings as $legacy => $deprecation) {
      if (!empty($settings[$legacy])) {
        @trigger_error($deprecation['message'], E_USER_DEPRECATED);
        // Set the new key if needed.
        if (!isset($settings[$deprecation['replacement']])) {
          $settings[$deprecation['replacement']] = $settings[$legacy];
        }
      }
      // Ensure that both keys have the same value.
      if (isset($settings[$deprecation['replacement']])) {
        $settings[$legacy] = $settings[$deprecation['replacement']];
      }
    }
  }
  public function testFakeDeprecatedSettings(array $settings_config, string $setting_name, string $expected_value, bool $expect_deprecation_message = TRUE): void {

    $settings_file_content = "<?php\n";
    foreach ($settings_config as $name => $value) {
      $settings_file_content .= "\$settings['$name'] = '$value';\n";
    }
    $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_file_content);

    // 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.',
    ];

    $class = new \ReflectionClass(Settings::class);
    $instance_property = $class->getProperty('deprecatedSettings');
    $instance_property->setAccessible(TRUE);
    $deprecated_settings = $instance_property->getValue();
    $deprecated_settings['deprecated_legacy'] = $deprecated_setting;
    $instance_property->setValue($deprecated_settings);

    if ($expect_deprecation_message) {
      $this->expectDeprecation($deprecated_setting['message']);
    }

    Settings::initialize(vfsStream::url('root'), 'sites', $class_loader);
    $this->assertEquals($expected_value, Settings::get($setting_name));
  }

catch’s picture

Cross-linking #3261245: Remove deprecated views module functions where we have an extremely similar issue.

alexpott’s picture

@catch or is an API based around a private static something we should be testing?

catch’s picture

Well that's a good question, if we can agree we won't spend three weeks and ~50 comments re-implementing the same test coverage again, then it would be OK to remove, but I don't want to be doing that every time we add and then remove it again.

andypost’s picture

Issue tags: +Deprecation Removal
daffie’s picture

Status: Needs review » Reviewed & tested by the community

+1 For keeping the empty deprecation tests.

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Let's discuss the removal of the pointless empty static and related functionality and test in a separate issue. There's no need for that discussion to delay the deprecation removal any longer. (As much as private static $deprecatedSettings = []; makes me shake my head)

Committed 0be587c and pushed to 10.0.x. Thanks!

  • alexpott committed 0be587c on 10.0.x
    Issue #3269149 by longwave, catch, alexpott: Remove deprecated settings
    

Status: Fixed » Closed (fixed)

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