refactor: differentiate between plugin locator and header locator usage

- Reverts `(set|get)PluginClassLoader()` method to original implementations, but with the addition of triggering deprecation notices. `trigger_error()` is now silenced, per recommendation from @glensc. - Adds `$headerLocator` property. - Renames `(get|set)HeaderLoader()` methods to `get|setHeaderLocator()`, and adds typehints. These now operate on the `$headerLocator` property. - Modifies `resolveHeaderClass()` to check for a non-null `$pluginClassLoader` property; if found, it uses that to resolve the header class, defaulting to `GenericHeader` if not loaded. - Updates PHPUnit dependency to 7.5.20, so we can use the various error types it provides. This is possible since we now depend on 7.1+. However, I did not update to later versions of PHPUnit as they would require refactoring tests to add typehints. - Refactors the unit tests to mirror the changes made. Signed-off-by: Matthew Weier O'Phinney <[email protected]>
laminas · Jul 29, 2020 · e85123e · e85123e
1 parent 5c5e243
commit e85123e
Show file tree

Hide file tree

Showing 3 changed files with 90 additions and 73 deletions.
diff --git a/composer.json b/composer.json
@@ -43,7 +43,7 @@
         "laminas/laminas-config": "^2.6",
         "laminas/laminas-crypt": "^2.6 || ^3.0",
         "laminas/laminas-servicemanager": "^2.7.10 || ^3.3.1",
-        "phpunit/phpunit": "^5.7.25 || ^6.4.4 || ^7.1.4"
+        "phpunit/phpunit": "^7.5.20"
     },
     "suggest": {
         "laminas/laminas-crypt": "Crammd5 support in SMTP Auth",

diff --git a/src/Headers.php b/src/Headers.php
@@ -6,6 +6,8 @@
  * @license   https://github.com/laminas/laminas-mail/blob/master/LICENSE.md New BSD License
  */
 
+declare(strict_types=1);
+
 namespace Laminas\Mail;
 
 use ArrayIterator;
@@ -31,11 +33,15 @@ class Headers implements Countable, Iterator
     const FOLDING = "\r\n ";
 
     /**
-     * @todo Rename to $headerLoader for 3.0.0
-     * @todo Remove ability to use PluginClassLoader for 3.0.0
-     * @var null|Header\HeaderLoader|PluginClassLoader
+     * @var null|Header\HeaderLocatorInterface
+     */
+    private $headerLocator;
+
+    /**
+     * @todo Remove for 3.0.0.
+     * @var null|PluginClassLocator
      */
-    protected $pluginClassLoader = null;
+    protected $pluginClassLoader;
 
     /**
      * @var array key names for $headers array
@@ -122,70 +128,70 @@ public static function fromString($string, $EOL = self::EOL)
     }
 
     /**
-     * Set an alternate implementation for loading header classes.
+     * Set an alternate PluginClassLocator implementation for loading header classes.
      *
      * @deprecated since 2.12.0
      * @todo Remove for version 3.0.0
-     * @param  Header\HeaderLoader|\Laminas\Loader\PluginClassLocator $pluginClassLoader
-     * @return Headers
+     * @return $this
      */
-    public function setPluginClassLoader($pluginClassLoader)
+    public function setPluginClassLoader(PluginClassLocator $pluginClassLoader)
     {
-        if ($pluginClassLoader instanceof PluginClassLocator) {
-            trigger_error(sprintf(
-                'Usage of %s instances for loading headers has been deprecated; please use %s',
-                PluginClassLoader::class,
-                Header\HeaderLoader::class
-            ), E_USER_DEPRECATED);
-        }
+        // Silenced; can be caught in custom error handlers.
+        @trigger_error(sprintf(
+            'Since laminas/laminas-mail 2.12.0: Usage of %s is deprecated; use %s::setHeaderLocator() instead',
+            __METHOD__,
+            __CLASS__
+        ), E_USER_DEPRECATED);
 
         $this->pluginClassLoader = $pluginClassLoader;
         return $this;
     }
 
     /**
-     * Return an instance of a Header\HeaderLoader or PluginClassLocator
+     * Return a PluginClassLocator instance for customizing headers.
      *
      * Lazyloads a Header\HeaderLoader if necessary.
      *
      * @deprecated since 2.12.0
      * @todo Remove for version 3.0.0
-     * @return Header\HeaderLoader|PluginClassLocator
+     * @return PluginClassLocator
      */
     public function getPluginClassLoader()
     {
-        trigger_error(sprintf(
-            'The method %s has been deprecated; please use getHeaderLoader() or resolveHeaderClass() instead',
-            __METHOD__
+        // Silenced; can be caught in custom error handlers.
+        @trigger_error(sprintf(
+            'Since laminas/laminas-mail 2.12.0: Usage of %s is deprecated; use %s::getHeaderLocator() instead',
+            __METHOD__,
+            __CLASS__
         ), E_USER_DEPRECATED);
 
-        if ($this->pluginClassLoader === null) {
+        if (! $this->pluginClassLoader) {
             $this->pluginClassLoader = new Header\HeaderLoader();
         }
 
         return $this->pluginClassLoader;
     }
 
     /**
-     * Retrieve the header class loader.
+     * Retrieve the header class locator for customizing headers.
      *
-     * @todo remove ability to return PluginClassLoader for 3.0.0
-     * @return Header\HeaderLoader|PluginClassLoader
+     * Lazyloads a Header\HeaderLocator instance if necessary.
      */
-    public function getHeaderLoader()
+    public function getHeaderLocator(): Header\HeaderLocatorInterface
     {
-        if (! $this->pluginClassLoader) {
-            $this->setHeaderLoader(new Header\HeaderLoader());
+        if (! $this->headerLocator) {
+            $this->setHeaderLocator(new Header\HeaderLocator());
         }
-        return $this->pluginClassLoader;
+        return $this->headerLocator;
     }
 
     /**
+     * @todo Return self when we update to 7.4 or later as minimum PHP version.
      * @return $this
      */
-    public function setHeaderLoader(Header\HeaderLoader $headerLoader)
+    public function setHeaderLocator(Header\HeaderLocatorInterface $headerLocator)
     {
-        $this->pluginClassLoader = $headerLoader;
+        $this->headerLocator = $headerLocator;
         return $this;
     }
 
@@ -581,6 +587,9 @@ protected function normalizeFieldName($fieldName)
      */
     private function resolveHeaderClass($key)
     {
-        return $this->getHeaderLoader()->get($key, Header\GenericHeader::class);
+        if ($this->pluginClassLoader) {
+            return $this->pluginClassLoader->load($key) ?: Header\GenericHeader::class;
+        }
+        return $this->getHeaderLocator()->get($key, Header\GenericHeader::class);
     }
 }
diff --git a/test/HeadersTest.php b/test/HeadersTest.php
@@ -18,11 +18,47 @@
 use PHPUnit\Framework\TestCase;
 
 /**
- * @group      Laminas_Mail
  * @covers \Laminas\Mail\Headers<extended>
  */
 class HeadersTest extends TestCase
 {
+    /** @var null|callable */
+    private $originalErrorHandler;
+
+    public function tearDown(): void
+    {
+        $this->restoreErrorHandler();
+    }
+
+    /**
+     * Handle deprecation errors and throw them.
+     *
+     * This is necessary as we are silencing the trigger_error call. This is
+     * done so that there is no impact on users, but, if they are logging errors
+     * using an error handler, they will see them in their logs. As such, we
+     * cannot rely on PHPUnit to catch them, and need to instead handle them
+     * ourselves in a similar fashion.
+     */
+    public function setDeprecationErrorHandler(): void
+    {
+        $this->originalErrorHandler = set_error_handler(
+            function (int $errno, string $errstr, string $errfile, int $errline) {
+                throw new Deprecated($errstr, $errno, $errfile, $errline);
+            },
+            E_USER_DEPRECATED
+        );
+    }
+
+    public function restoreErrorHandler(): void
+    {
+        if (null !== $this->originalErrorHandler) {
+            return;
+        }
+
+        restore_error_handler();
+        $this->originalErrorHandler = null;
+    }
+
     public function testHeadersImplementsProperClasses()
     {
         $headers = new Mail\Headers();
@@ -110,7 +146,7 @@ public function testHeadersFromStringFactoryCreatesMultipleObjects()
     public function testHeadersFromStringMultiHeaderWillAggregateLazyLoadedHeaders()
     {
         $headers = new Mail\Headers();
-        $loader  = $headers->getHeaderLoader();
+        $loader  = $headers->getHeaderLocator();
         $loader->add('foo', GenericMultiHeader::class);
         $headers->addHeaderLine('foo: bar1,bar2,bar3');
         $headers->forceLoading();
@@ -567,26 +603,20 @@ public function testAddHeaderCallsSetEncoding()
      */
     public function testGetPluginClassLoaderEmitsDeprecationNotice()
     {
-        if (! class_exists(Deprecated::class)) {
-            $this->markTestSkipped('Requires a PHPUnit version that contains Error exceptions');
-        }
-
+        $this->setDeprecationErrorHandler();
         $headers = new Mail\Headers();
 
         $this->expectException(Deprecated::class);
-        $this->expectExceptionMessage('getPluginClassLoader has been deprecated');
+        $this->expectExceptionMessage('getPluginClassLoader is deprecated');
         $headers->getPluginClassLoader();
     }
 
     /**
      * @todo Remove for 3.0.0
      */
-    public function testSetPluginClassLoaderEmitsDeprecationNoticeWhenPluginClassLocatorUsed()
+    public function testSetPluginClassLoaderEmitsDeprecationNotice()
     {
-        if (! class_exists(Deprecated::class)) {
-            $this->markTestSkipped('Requires a PHPUnit version that contains Error exceptions');
-        }
-
+        $this->setDeprecationErrorHandler();
         $headers = new Mail\Headers();
         $loader  = $this->prophesize(PluginClassLocator::class)->reveal();
 
@@ -595,41 +625,19 @@ public function testSetPluginClassLoaderEmitsDeprecationNoticeWhenPluginClassLoc
         $headers->setPluginClassLoader($loader);
     }
 
-    /**
-     * @todo Remove for 3.0.0
-     */
-    public function testGetPluginClassLoaderReturnsHeaderLoaderInstanceByDefault()
-    {
-        $headers = new Mail\Headers();
-        $loader  = @$headers->getPluginClassLoader();
-        $this->assertInstanceOf(Mail\Header\HeaderLoader::class, $loader);
-    }
-
-    /**
-     * @todo Remove for 3.0.0
-     */
-    public function testSetPluginClassLoaderAcceptsHeaderLoaderInstance()
-    {
-        $headers = new Mail\Headers();
-        $loader  = new Mail\Header\HeaderLoader();
-
-        @$headers->setPluginClassLoader($loader);
-        $this->assertSame($loader, $headers->getHeaderLoader());
-    }
-
-    public function testGetHeaderLoaderReturnsHeaderLoaderInstanceByDefault()
+    public function testGetHeaderLocatorReturnsHeaderLocatorInstanceByDefault()
     {
         $headers = new Mail\Headers();
-        $loader  = $headers->getHeaderLoader();
-        $this->assertInstanceOf(Mail\Header\HeaderLoader::class, $loader);
+        $locator = $headers->getHeaderLocator();
+        $this->assertInstanceOf(Mail\Header\HeaderLocator::class, $locator);
     }
 
-    public function testCanInjectAlternateHeaderLoaderInstance()
+    public function testCanInjectAlternateHeaderLocatorInstance()
     {
         $headers = new Mail\Headers();
-        $loader  = new Mail\Header\HeaderLoader();
+        $locator = $this->prophesize(Mail\Header\HeaderLocatorInterface::class)->reveal();
 
-        $headers->setHeaderLoader($loader);
-        $this->assertSame($loader, $headers->getHeaderLoader());
+        $headers->setHeaderLocator($locator);
+        $this->assertSame($locator, $headers->getHeaderLocator());
     }
 }