Merge pull request #10559 from creative-commoners/pulls/4/deprecated-config

NEW Record deprecated config

src/Dev/Deprecation.php

@@ -3,7 +3,6 @@
 namespace SilverStripe\Dev;
 
 use BadMethodCallException;
-use Exception;
 use SilverStripe\Control\Director;
 use SilverStripe\Core\Environment;
 use SilverStripe\Core\Injector\InjectorLoader;
@@ -41,6 +40,7 @@ class Deprecation
     const SCOPE_METHOD = 1;
     const SCOPE_CLASS = 2;
     const SCOPE_GLOBAL = 4;
+    const SCOPE_CONFIG = 8;
 
     /**
      * @var string
@@ -67,8 +67,10 @@ class Deprecation
      * must be available before this to avoid infinite loops.
      *
      * This will be overriden by the SS_DEPRECATION_ENABLED environment if present
+     *
+     * @internal - Marked as internal so this and other private static's are not treated as config
      */
-    protected static bool $is_enabled = false;
+    private static bool $is_enabled = false;
 
     /**
      * @var array
@@ -76,20 +78,35 @@ class Deprecation
      */
     protected static $module_version_overrides = [];
 
-    protected static bool $inside_notice = false;
+    /**
+     * @internal
+     */
+    private static bool $inside_notice = false;
 
     /**
-     * Switched out by unit-testing to E_USER_NOTICE because E_USER_DEPRECATED is not
+     * @var array
-     * caught by $this->expectDeprecated() by default
+     * @deprecated 4.12.0 Will be removed without equivalent functionality to replace it
-     * https://github.com/laminas/laminas-di/pull/30#issuecomment-927585210
-     *
-     * @var int
      */
     public static $notice_level = E_USER_DEPRECATED;
 
+    /**
+     * Buffer of user_errors to be raised when enabled() is called
+     *
+     * This is used when setting deprecated config via yaml, before Deprecation::enable() has been called in _config.php
+     * Deprecated config set via yaml will only be shown in the browser when using ?flush=1
+     * It will not show in CLI when running dev/build flush=1
+     *
+     * @internal
+     */
+    private static array $user_error_message_buffer = [];
+
     public static function enable(): void
     {
         static::$is_enabled = true;
+        foreach (self::$user_error_message_buffer as $message) {
+            user_error($message, E_USER_DEPRECATED);
+        }
+        self::$user_error_message_buffer = [];
     }
 
     public static function disable(): void
@@ -167,10 +184,7 @@ class Deprecation
         if (!Director::isDev()) {
             return false;
         }
-        if (Environment::getEnv('SS_DEPRECATION_ENABLED')) {
+        return static::$is_enabled || Environment::getEnv('SS_DEPRECATION_ENABLED');
-            return true;
-        }
-        return static::$is_enabled;
     }
 
     /**
@@ -202,40 +216,44 @@ class Deprecation
         // try block needs to wrap all code in case anything inside the try block
         // calls something else that calls Deprecation::notice()
         try {
-            if (!self::get_is_enabled()) {
+            if ($scope === self::SCOPE_CONFIG) {
-                return;
+                if (self::get_is_enabled()) {
-            }
+                    user_error($string, E_USER_DEPRECATED);
-
+                } else {
-            // If you pass #.#, assume #.#.0
+                    self::$user_error_message_buffer[] = $string;
-            if (preg_match('/^[0-9]+\.[0-9]+$/', $atVersion ?? '')) {
+                }
-                $atVersion .= '.0';
-            }
-
-            // Getting a backtrace is slow, so we only do it if we need it
-            $backtrace = null;
-
-            // Get the calling scope
-            if ($scope == Deprecation::SCOPE_METHOD) {
-                $backtrace = debug_backtrace(0);
-                $caller = self::get_called_method_from_trace($backtrace);
-            } elseif ($scope == Deprecation::SCOPE_CLASS) {
-                $backtrace = debug_backtrace(0);
-                $caller = isset($backtrace[1]['class']) ? $backtrace[1]['class'] : '(unknown)';
             } else {
-                $caller = false;
+                if (!self::get_is_enabled()) {
-            }
+                    // Do not add to self::$user_error_message_buffer, as the backtrace is too expensive
+                    return;
+                }
+    
+                // Getting a backtrace is slow, so we only do it if we need it
+                $backtrace = null;
+    
+                // Get the calling scope
+                if ($scope == Deprecation::SCOPE_METHOD) {
+                    $backtrace = debug_backtrace(0);
+                    $caller = self::get_called_method_from_trace($backtrace);
+                } elseif ($scope == Deprecation::SCOPE_CLASS) {
+                    $backtrace = debug_backtrace(0);
+                    $caller = isset($backtrace[1]['class']) ? $backtrace[1]['class'] : '(unknown)';
+                } else {
+                    $caller = false;
+                }
+    
+                // Then raise the notice
+                if (substr($string, -1) != '.') {
+                    $string .= ".";
+                }
 
-            // Then raise the notice
+                $string .= " Called from " . self::get_called_method_from_trace($backtrace, 2) . '.';
-            if (substr($string, -1) != '.') {
-                $string .= ".";
-            }
 
-            $string .= " Called from " . self::get_called_method_from_trace($backtrace, 2) . '.';
+                if ($caller) {
-
+                    user_error($caller . ' is deprecated.' . ($string ? ' ' . $string : ''), E_USER_DEPRECATED);
-            if ($caller) {
+                } else {
-                user_error($caller . ' is deprecated.' . ($string ? ' ' . $string : ''), self::$notice_level);
+                    user_error($string, E_USER_DEPRECATED);
-            } else {
+                }
-                user_error($string, self::$notice_level);
             }
         } catch (BadMethodCallException $e) {
             if ($e->getMessage() === InjectorLoader::NO_MANIFESTS_AVAILABLE) {

tests/php/Dev/DeprecationTest.php

@@ -2,16 +2,43 @@
 
 namespace SilverStripe\Dev\Tests;
 
+use PHPUnit\Framework\Error\Deprecated;
+use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\Deprecation;
 use SilverStripe\Dev\SapphireTest;
-use SilverStripe\Dev\Tests\DeprecationTest\TestDeprecation;
+use SilverStripe\Dev\Tests\DeprecationTest\DeprecationTestObject;
 
 class DeprecationTest extends SapphireTest
 {
+    protected static $extra_dataobjects = [
+        DeprecationTestObject::class,
+    ];
+
+    private $oldHandler = null;
+
+    protected function setup(): void
+    {
+        // Use custom error handler for two reasons:
+        // - Filter out errors for deprecated class properities unrelated to this unit test
+        // - Allow the use of expectDeprecation(), which doesn't work with E_USER_DEPRECATION by default
+        //   https://github.com/laminas/laminas-di/pull/30#issuecomment-927585210
+        parent::setup();
+        $this->oldHandler = set_error_handler(function (int $errno, string $errstr, string $errfile, int $errline) {
+            if (str_contains($errstr, 'SilverStripe\\Dev\\Tests\\DeprecationTest')) {
+                throw new Deprecated($errstr, $errno, '', 1);
+            }
+            if (is_callable($this->oldHandler)) {
+                return call_user_func($this->oldHandler, $errno, $errstr, $errfile, $errline);
+            }
+            return false;
+        });
+    }
+
     protected function tearDown(): void
     {
-        Deprecation::$notice_level = E_USER_DEPRECATED;
         Deprecation::disable();
+        set_error_handler($this->oldHandler);
+        $this->oldHandler = null;
         parent::tearDown();
     }
 
@@ -22,9 +49,8 @@ class DeprecationTest extends SapphireTest
             'My message.',
             'Called from PHPUnit\Framework\TestCase->runTest.'
         ]);
-        $this->expectError();
+        $this->expectDeprecation();
-        $this->expectErrorMessage($message);
+        $this->expectDeprecationMessage($message);
-        Deprecation::$notice_level = E_USER_NOTICE;
         Deprecation::enable();
         Deprecation::notice('1.2.3', 'My message');
     }
@@ -34,8 +60,71 @@ class DeprecationTest extends SapphireTest
      */
     public function testDisabled()
     {
-        Deprecation::$notice_level = E_USER_NOTICE;
         // test that no error error is raised because by default Deprecation is disabled
         Deprecation::notice('4.5.6', 'My message');
     }
+
+    // The following tests would be better to put in the silverstripe/config module, however this is not
+    // possible to do in a clean way as the config for the DeprecationTestObject will not load if it
+    // is inside the silverstripe/config directory, as there is no _config.php file or _config folder.
+    // Adding a _config.php file will break existing unit-tests within silverstripe/config
+    // It is possible to put DeprecationTestObject in framework and the unit tests in config, however
+    // that's probably messier then just having everything within framework
+
+    public function testConfigGetFirst()
+    {
+        $message = implode(' ', [
+            'Config SilverStripe\Dev\Tests\DeprecationTest\DeprecationTestObject.first_config is deprecated.',
+            'My first config message.'
+        ]);
+        $this->expectDeprecation();
+        $this->expectDeprecationMessage($message);
+        Deprecation::enable();
+        Config::inst()->get(DeprecationTestObject::class, 'first_config');
+    }
+
+    public function testConfigGetSecond()
+    {
+        $message = implode(' ', [
+            'Config SilverStripe\Dev\Tests\DeprecationTest\DeprecationTestObject.second_config is deprecated.',
+            'My second config message.'
+        ]);
+        $this->expectDeprecation();
+        $this->expectDeprecationMessage($message);
+        Deprecation::enable();
+        Config::inst()->get(DeprecationTestObject::class, 'second_config');
+    }
+
+    public function testConfigGetThird()
+    {
+        $message = 'Config SilverStripe\Dev\Tests\DeprecationTest\DeprecationTestObject.third_config is deprecated.';
+        $this->expectDeprecation();
+        $this->expectDeprecationMessage($message);
+        Deprecation::enable();
+        Config::inst()->get(DeprecationTestObject::class, 'third_config');
+    }
+
+    public function testConfigSet()
+    {
+        $message = implode(' ', [
+            'Config SilverStripe\Dev\Tests\DeprecationTest\DeprecationTestObject.first_config is deprecated.',
+            'My first config message.'
+        ]);
+        $this->expectDeprecation();
+        $this->expectDeprecationMessage($message);
+        Deprecation::enable();
+        Config::modify()->set(DeprecationTestObject::class, 'first_config', 'abc');
+    }
+
+    public function testConfigMerge()
+    {
+        $message = implode(' ', [
+            'Config SilverStripe\Dev\Tests\DeprecationTest\DeprecationTestObject.array_config is deprecated.',
+            'My array config message.'
+        ]);
+        $this->expectDeprecation();
+        $this->expectDeprecationMessage($message);
+        Deprecation::enable();
+        Config::modify()->merge(DeprecationTestObject::class, 'array_config', ['abc']);
+    }
 }

tests/php/Dev/DeprecationTest/DeprecationTestObject.php

@@ -0,0 +1,35 @@
+<?php
+
+namespace SilverStripe\Dev\Tests\DeprecationTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+
+class DeprecationTestObject extends DataObject implements TestOnly
+{
+    private static $db = [
+        "Name" => "Varchar"
+    ];
+
+    private static $table_name = 'DeprecatedTestObject';
+
+    /**
+     * @deprecated 1.2.3 My first config message
+     */
+    private static $first_config = 'ABC';
+
+    /**
+     * @deprecated My second config message
+     */
+    private static $second_config = 'DEF';
+
+    /**
+     * @deprecated
+     */
+    private static $third_config = 'XYZ';
+
+    /**
+     * @deprecated 1.2.3 My array config message
+     */
+    private static $array_config = ['lorem', 'ipsum'];
+}