Merge pull request #24 from doctrine/SimplifyModes

Simplify modes, allow stacking them, no tracking by default. Improve README

Author: beberlei

README.md

@@ -1,59 +1,76 @@
 # Doctrine Deprecations
 
-A small layer on top of `trigger_error(E_USER_DEPRECATED)` or PSR-3 logging
-with options to disable all deprecations or selectively for packages.
+A small (side-effect free by default) layer on top of
+`trigger_error(E_USER_DEPRECATED)` or PSR-3 logging.
 
-By default it does not log deprecations at runtime and needs to be configured
-to log through either trigger_error or with a PSR-3 logger. This is done to
-avoid side effects by deprecations on user error handlers that Doctrine has no
-control over.
+- no side-effects by default, making it a perfect fit for libraries that don't know how the error handler works they operate under
+- options to avoid having to rely on error handlers global state by using PSR-3 logging
+- deduplicate deprecation messages to avoid excessive triggering and reduce overhead
+
+We recommend to collect Deprecations using a PSR logger instead of relying on
+the global error handler.
 
 ## Usage from consumer perspective:
 
-Enable or Disable Doctrine deprecations to be sent as `trigger_error(E_USER_DEPRECATED)`
+Enable Doctrine deprecations to be sent to a PSR3 logger:
+
+```php
+\Doctrine\Deprecations\Deprecation::enableWithPsrLogger($logger);
+```
+
+Enable Doctrine deprecations to be sent as `@trigger_error($message, E_USER_DEPRECATED)`
 messages.
 
 ```php
 \Doctrine\Deprecations\Deprecation::enableWithTriggerError();
-\Doctrine\Deprecations\Deprecation::enableWithSuppressedTriggerError();
-\Doctrine\Deprecations\Deprecation::disable();
 ```
 
-Enable Doctrine deprecations to be sent to a PSR3 logger:
+If you only want to enable deprecation tracking, without logging or calling `trigger_error` then call:
 
 ```php
-\Doctrine\Deprecations\Deprecation::enableWithPsrLogger($logger);
+\Doctrine\Deprecations\Deprecation::enableTrackingDeprecations();
 ```
 
-Disable deprecations from a package
+Tracking is enabled with all three modes and provides access to all triggered
+deprecations and their individual count:
 
 ```php
-\Doctrine\Deprecations\Deprecation::ignorePackage("doctrine/orm");
+$deprecations = \Doctrine\Deprecations\Deprecation::getTriggeredDeprecations();
+
+foreach ($deprecations as $identifier => $count) {
+    echo $identifier . " was triggered " . $count . " times\n";
+}
 ```
 
+### Suppressing Specific Deprecations
+
 Disable triggering about specific deprecations:
 
 ```php
 \Doctrine\Deprecations\Deprecation::ignoreDeprecations("https://link/to/deprecations-description-identifier");
 ```
 
-Access is provided to all triggered deprecations and their individual count:
+Disable all deprecations from a package
 
 ```php
-$deprecations = \Doctrine\Deprecations\Deprecation::getTriggeredDeprecations();
-
-foreach ($deprecations as $identifier => $count) {
-    echo $identifier . " was triggered " . $count . " times\n";
-}
+\Doctrine\Deprecations\Deprecation::ignorePackage("doctrine/orm");
 ```
 
+### Other Operations
+
 When used within PHPUnit or other tools that could collect multiple instances of the same deprecations
 the deduplication can be disabled:
 
 ```php
 \Doctrine\Deprecations\Deprecation::withoutDeduplication();
 ```
 
+Disable deprecation tracking again:
+
+```php
+\Doctrine\Deprecations\Deprecation::disable();
+```
+
 ## Usage from a library/producer perspective:
 
 When you want to unconditionally trigger a deprecation even when called
@@ -93,7 +110,7 @@ then use:
 ```
 
 Based on the issue link each deprecation message is only triggered once per
-request, so it must be unique for each deprecation.
+request.
 
 A limited stacktrace is included in the deprecation message to find the
 offending location.

lib/Doctrine/Deprecations/Deprecation.php

@@ -8,41 +8,43 @@
 
 use function array_key_exists;
 use function array_reduce;
-use function basename;
 use function debug_backtrace;
 use function sprintf;
 use function strpos;
+use function strrpos;
+use function substr;
 use function trigger_error;
 
 use const DEBUG_BACKTRACE_IGNORE_ARGS;
+use const DIRECTORY_SEPARATOR;
 use const E_USER_DEPRECATED;
 
 /**
  * Manages Deprecation logging in different ways.
  *
- * By default triggered exceptions are not logged, only the amount of
- * deprecations triggered can be queried with `Deprecation::getUniqueTriggeredDeprecationsCount()`.
+ * By default triggered exceptions are not logged.
  *
  * To enable different deprecation logging mechanisms you can call the
  * following methods:
  *
- *  - Uses trigger_error with E_USER_DEPRECATED
- *    \Doctrine\Deprecations\Deprecation::enableWithTriggerError();
+ *  - Minimal collection of deprecations via getTriggeredDeprecations()
+ *    \Doctrine\Deprecations\Deprecation::enableTrackingDeprecations();
  *
  *  - Uses @trigger_error with E_USER_DEPRECATED
- *    \Doctrine\Deprecations\Deprecation::enableWithSuppressedTriggerError();
+ *    \Doctrine\Deprecations\Deprecation::enableWithTriggerError();
  *
  *  - Sends deprecation messages via a PSR-3 logger
  *    \Doctrine\Deprecations\Deprecation::enableWithPsrLogger($logger);
  *
- * Packages that trigger deprecations should use the `trigger()` method.
+ * Packages that trigger deprecations should use the `trigger()` or
+ * `triggerIfCalledFromOutside()` methods.
  */
 class Deprecation
 {
-    private const TYPE_NONE                     = 0;
-    private const TYPE_TRIGGER_ERROR            = 1;
-    private const TYPE_TRIGGER_SUPPRESSED_ERROR = 2;
-    private const TYPE_PSR_LOGGER               = 3;
+    private const TYPE_NONE               = 0;
+    private const TYPE_TRACK_DEPRECATIONS = 1;
+    private const TYPE_TRIGGER_ERROR      = 2;
+    private const TYPE_PSR_LOGGER         = 4;
 
     /** @var int */
     private static $type = self::TYPE_NONE;
@@ -60,7 +62,7 @@ class Deprecation
     private static $deduplication = true;
 
     /**
-     * Trigger a deprecation for the given package, starting with given version.
+     * Trigger a deprecation for the given package and identfier.
      *
      * The link should point to a Github issue or Wiki entry detailing the
      * deprecation. It is additionally used to de-duplicate the trigger of the
@@ -70,6 +72,10 @@ class Deprecation
      */
     public static function trigger(string $package, string $link, string $message, ...$args): void
     {
+        if (self::$type === self::TYPE_NONE) {
+            return;
+        }
+
         if (array_key_exists($link, self::$ignoredLinks)) {
             self::$ignoredLinks[$link]++;
         } else {
@@ -80,12 +86,6 @@ public static function trigger(string $package, string $link, string $message, .
             return;
         }
 
-        // do not move this condition to the top, because we still want to
-        // count occcurences of deprecations even when we are not logging them.
-        if (self::$type === self::TYPE_NONE) {
-            return;
-        }
-
         if (isset(self::$ignoredPackages[$package])) {
             return;
         }
@@ -98,16 +98,32 @@ public static function trigger(string $package, string $link, string $message, .
     }
 
     /**
+     * Trigger a deprecation for the given package and identifier when called from outside.
+     *
+     * "Outside" means we assume that $package is currently installed as a
+     * dependency and the caller is not a file in that package. When $package
+     * is installed as a root package then deprecations triggered from the
+     * tests folder are also considered "outside".
+     *
+     * This deprecation method assumes that you are using Composer to install
+     * the dependency and are using the default /vendor/ folder and not a
+     * Composer plugin to change the install location. The assumption is also
+     * that $package is the exact composer packge name.
+     *
+     * Compared to {@link trigger()} this method causes some overhead when
+     * deprecation tracking is enabled even during deduplication, because it
+     * needs to call {@link debug_backtrace()}
+     *
      * @param mixed $args
      */
     public static function triggerIfCalledFromOutside(string $package, string $link, string $message, ...$args): void
     {
+        if (self::$type === self::TYPE_NONE) {
+            return;
+        }
+
         $backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2);
 
-        // "outside" means we assume that $package is currently installed as a
-        // dependency and the caller is not a file in that package.
-        // When $package is installed as a root package, then this deprecation
-        // is always ignored
         // first check that the caller is not from a tests folder, in which case we always let deprecations pass
         if (strpos($backtrace[1]['file'], '/tests/') === false) {
             if (strpos($backtrace[0]['file'], '/vendor/' . $package . '/') === false) {
@@ -129,12 +145,6 @@ public static function triggerIfCalledFromOutside(string $package, string $link,
             return;
         }
 
-        // do not move this condition to the top, because we still want to
-        // count occcurences of deprecations even when we are not logging them.
-        if (self::$type === self::TYPE_NONE) {
-            return;
-        }
-
         if (isset(self::$ignoredPackages[$package])) {
             return;
         }
@@ -149,56 +159,61 @@ public static function triggerIfCalledFromOutside(string $package, string $link,
      */
     private static function delegateTriggerToBackend(string $message, array $backtrace, string $link, string $package): void
     {
-        if (self::$type === self::TYPE_TRIGGER_ERROR) {
-            $message .= sprintf(
-                ' (%s:%d called by %s:%d, %s, package %s)',
-                basename($backtrace[0]['file']),
-                $backtrace[0]['line'],
-                basename($backtrace[1]['file']),
-                $backtrace[1]['line'],
-                $link,
-                $package
-            );
-
-            trigger_error($message, E_USER_DEPRECATED);
-        } elseif (self::$type === self::TYPE_TRIGGER_SUPPRESSED_ERROR) {
-            $message .= sprintf(
-                ' (%s:%d called by %s:%d, %s, package %s)',
-                basename($backtrace[0]['file']),
-                $backtrace[0]['line'],
-                basename($backtrace[1]['file']),
-                $backtrace[1]['line'],
-                $link,
-                $package
-            );
-
-            @trigger_error($message, E_USER_DEPRECATED);
-        } elseif (self::$type === self::TYPE_PSR_LOGGER) {
+        if ((self::$type & self::TYPE_PSR_LOGGER) > 0) {
             $context = [
                 'file' => $backtrace[0]['file'],
                 'line' => $backtrace[0]['line'],
+                'package' => $package,
+                'link' => $link,
             ];
 
-            $context['package'] = $package;
-            $context['link']    = $link;
-
             self::$logger->notice($message, $context);
         }
+
+        if (! ((self::$type & self::TYPE_TRIGGER_ERROR) > 0)) {
+            return;
+        }
+
+        $message .= sprintf(
+            ' (%s:%d called by %s:%d, %s, package %s)',
+            self::basename($backtrace[0]['file']),
+            $backtrace[0]['line'],
+            self::basename($backtrace[1]['file']),
+            $backtrace[1]['line'],
+            $link,
+            $package
+        );
+
+        @trigger_error($message, E_USER_DEPRECATED);
     }
 
-    public static function enableWithTriggerError(): void
+    /**
+     * A non-local-aware version of PHPs basename function.
+     */
+    private static function basename(string $filename): string
     {
-        self::$type = self::TYPE_TRIGGER_ERROR;
+        $pos = strrpos($filename, DIRECTORY_SEPARATOR);
+
+        if ($pos === false) {
+            return $filename;
+        }
+
+        return substr($filename, $pos + 1);
     }
 
-    public static function enableWithSuppressedTriggerError(): void
+    public static function enableTrackingDeprecations(): void
+    {
+        self::$type |= self::TYPE_TRACK_DEPRECATIONS;
+    }
+
+    public static function enableWithTriggerError(): void
     {
-        self::$type = self::TYPE_TRIGGER_SUPPRESSED_ERROR;
+        self::$type |= self::TYPE_TRIGGER_ERROR;
     }
 
     public static function enableWithPsrLogger(LoggerInterface $logger): void
     {
-        self::$type   = self::TYPE_PSR_LOGGER;
+        self::$type  |= self::TYPE_PSR_LOGGER;
         self::$logger = $logger;
     }
 

lib/Doctrine/Deprecations/PHPUnit/VerifyDeprecations.php

@@ -26,6 +26,14 @@ public function expectNoDeprecationWithIdentifier(string $identifier): void
         $this->doctrineNoDeprecationsExpectations[$identifier] = Deprecation::getTriggeredDeprecations()[$identifier] ?? 0;
     }
 
+    /**
+     * @before
+     */
+    public function enableDeprecationTracking(): void
+    {
+        Deprecation::enableTrackingDeprecations();
+    }
+
     /**
      * @after
      */

test_fixtures/src/Foo.php

@@ -8,13 +8,13 @@
 
 class Foo
 {
-    public function triggerDependencyWithDeprecation(): void
+    public static function triggerDependencyWithDeprecation(): void
     {
         $bar = new Bar();
         $bar->oldFunc();
     }
 
-    public function triggerDependencyWithDeprecationFromInside(): void
+    public static function triggerDependencyWithDeprecationFromInside(): void
     {
         $bar = new Bar();
         $bar->newFunc();