Problem/Motivation

Lets remove usage of "blacklist" and "whitelist" in \Drupal\Core\Utility\Error and its test.

They are:

  • An historic bad labelling of people
  • Provide no context: "what is listed in them"?

Proposed resolution

s/blacklistFunctions/ignoredFunctions/

Remaining tasks

  1. Fix the class and test.
  2. Decide if we need BC for protected static $blacklistFunctions
  3. Reviews.
  4. RTBC.
  5. Commit.

User interface changes

None

API changes

None. Only changing a protected member's name.

Data model changes

None

Release notes snippet

@todo

CommentFileSizeAuthor
#3 3151095-3.patch2.12 KBdww

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Issue summary: View changes
dww’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new2.12 KB
alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record
+++ b/core/lib/Drupal/Core/Utility/Error.php
@@ -19,11 +19,11 @@ class Error {
-  protected static $blacklistFunctions = ['debug', '_drupal_error_handler', '_drupal_exception_handler'];
+  protected static $ignoredFunctions = ['debug', '_drupal_error_handler', '_drupal_exception_handler'];

I think we need a change record for this. Fortunately it does not appear that anyone has extended this but it has been copied in contrib - http://grep.xnddx.ru/search?text=%24blacklistFunctions&filename=

dww’s picture

CR for each of these separately, or should we do a single CR for all such protected member renaming that's happening under #2993575: [meta] Remove usage of "blacklist", "whitelist", use better terms instead?

alexpott’s picture

I think one CR per change. There are different concepts. The important thing about the CR is to make anyone who has extended the Error class in custom code to check. That's easier when the CR title has the class in.

dww’s picture

alexpott’s picture

The CR looks great.

jungle’s picture

Status: Needs review » Reviewed & tested by the community
  1. @internal

    The following parts of the code base should always be treated as internal and are not guaranteed to not change between minor versions. For patch versions we will aim to avoid any breaking changes even to @internal code unless required to fix a serious bug:

    ...

    # Protected properties
    Protected properties on a class are always considered @internal unless they're on a base class marked with @api

    -- See https://www.drupal.org/core/d8-bc-policy

    Not sure if we need BC for the property $blacklistFunctions, as no classes in contributed modules has extended it pointed by @alexpott in #3, maybe, the CR is enough. BTW, found $blacklistFunctions occurrences in the tag1quo module (http://grep.xnddx.ru/search?text=%24blacklistFunctions&filename=) , but the code is irrelevant to the class \Drupal\Core\Utility\Error.

  2. +++ b/core/lib/Drupal/Core/Utility/Error.php
    @@ -19,11 +19,11 @@ class Error {
    -   * An array of blacklisted functions.
    +   * An array of ignored functions.
    
    @@ -110,9 +110,9 @@ public static function renderExceptionSafe($exception) {
    -    // information about file and line. Ignore black listed functions.
    +    // information about file and line. Ignore the ignored functions.
    

    blacklisted/black listed functions -> ignored functions, makes sense to me.

  3. +++ b/core/lib/Drupal/Core/Utility/Error.php
    @@ -19,11 +19,11 @@ class Error {
    -  protected static $blacklistFunctions = ['debug', '_drupal_error_handler', '_drupal_exception_handler'];
    +  protected static $ignoredFunctions = ['debug', '_drupal_error_handler', '_drupal_exception_handler'];
    
    @@ -110,9 +110,9 @@ public static function renderExceptionSafe($exception) {
    -      (isset($backtrace[1]['function']) && in_array($backtrace[1]['function'], static::$blacklistFunctions))) {
    +      (isset($backtrace[1]['function']) && in_array($backtrace[1]['function'], static::$ignoredFunctions))) {
    
    +++ b/core/tests/Drupal/Tests/Core/Utility/ErrorTest.php
    @@ -49,7 +49,7 @@ public function providerTestGetLastCaller() {
    -    // Add blacklist functions to backtrace. They should get removed.
    +    // Add ignored functions to backtrace. They should get removed.
    

    blacklist functions -> ignored functions, not 100% sure. However, @dww is a native English speaker, this change should be good.

Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5f61f0f and pushed to 9.1.x. Thanks!

  • alexpott committed 5f61f0f on 9.1.x
    Issue #3151095 by dww, alexpott, jungle: Replace use of whitelist/...

Status: Fixed » Closed (fixed)

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