diff --git a/core/lib/Drupal/Core/Image/ImageInterface.php b/core/lib/Drupal/Core/Image/ImageInterface.php index 1e40e90..b8ca6a8 100644 --- a/core/lib/Drupal/Core/Image/ImageInterface.php +++ b/core/lib/Drupal/Core/Image/ImageInterface.php @@ -152,7 +152,10 @@ public function scaleAndCrop($width, $height); * Converts an image to a specified format. * * @param string $format - * The format to convert to (e.g. 'jpeg', 'png'). + * The format to convert to (e.g. 'jpeg', 'png'). Allowed values depend on + * the implementation of the convert operation of the image toolkit. For a + * working example, see + * \Drupal\system\Plugin\ImageToolkit\Operation\gd\Convert. */ public function convert($format); diff --git a/core/modules/image/src/Controller/ImageStyleDownloadController.php b/core/modules/image/src/Controller/ImageStyleDownloadController.php index 7d154b5..173114e 100644 --- a/core/modules/image/src/Controller/ImageStyleDownloadController.php +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php @@ -130,7 +130,10 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st // Don't try to generate file if source is missing. if (!file_exists($image_uri)) { - // Remove the extension to check if the image style added one. + // If the image style converted the extension, it has been added to the + // original file, resulting in filenames like image.png.jpeg. So to find + // the actual source image, we remove the extension and check if that + // image exists. $path_info = pathinfo($image_uri); $image_uri = $path_info['dirname'] . DIRECTORY_SEPARATOR . $path_info['filename']; if (!file_exists($image_uri)) { diff --git a/core/modules/image/src/Entity/ImageStyle.php b/core/modules/image/src/Entity/ImageStyle.php index 26411cb..2dabf9b 100644 --- a/core/modules/image/src/Entity/ImageStyle.php +++ b/core/modules/image/src/Entity/ImageStyle.php @@ -172,24 +172,15 @@ protected static function replaceImageStyle(ImageStyleInterface $style) { * {@inheritdoc} */ public function buildUri($uri) { - $scheme = file_uri_scheme($uri); + $scheme = $this->fileUriScheme($uri); if ($scheme) { - $path = file_uri_target($uri); + $path = $this->fileUriTarget($uri); } else { $path = $uri; - $scheme = file_default_scheme(); + $scheme = $this->fileDefaultScheme(); } - - // Check the extension of the derivative. - $extension = pathinfo($path, PATHINFO_EXTENSION); - $original_extension = $extension; - $this->getDerivativeExtension($extension); - if ($original_extension != $extension) { - $path .= '.' . $extension; - } - - return $scheme . '://styles/' . $this->id() . '/' . $scheme . '/' . $path; + return $scheme . '://styles/' . $this->id() . '/' . $scheme . '/' . $this->addExtension($path); } /** @@ -338,16 +329,8 @@ public function getDerivativeExtension(&$extension) { * {@inheritdoc} */ public function getPathToken($uri) { - // Check the extension of the derivative. - $extension = pathinfo($uri, PATHINFO_EXTENSION); - $original_extension = $extension; - $this->getDerivativeExtension($extension); - if ($original_extension != $extension) { - $uri .= '.' . $extension; - } - // Return the first 8 characters. - return substr(Crypt::hmacBase64($this->id() . ':' . $uri, $this->getPrivateKey() . $this->getHashSalt()), 0, 8); + return substr(Crypt::hmacBase64($this->id() . ':' . $this->addExtension($uri), $this->getPrivateKey() . $this->getHashSalt()), 0, 8); } /** @@ -416,24 +399,114 @@ public function setName($name) { } /** - * {@inheritdoc} + * Returns the image effect plugin manager. + * + * @return \Drupal\Component\Plugin\PluginManagerInterface + * The image effect plugin manager. */ - public function getImageEffectPluginManager() { + protected function getImageEffectPluginManager() { return \Drupal::service('plugin.manager.image.effect'); } /** - * {@inheritdoc} + * Gets the Drupal private key. + * + * @return string + * The Drupal private key. */ - public function getPrivateKey() { + protected function getPrivateKey() { return \Drupal::service('private_key')->get(); } /** - * {@inheritdoc} + * Gets a salt useful for hardening against SQL injection. + * + * @return string + * A salt based on information in settings.php, not in the database. + * + * @throws \RuntimeException */ - public function getHashSalt() { + protected function getHashSalt() { return Settings::getHashSalt(); } + /** + * Adds an extension to a path. + * + * If this image style changes the extension of the derivative, this method + * adds the new extension to the given path. This way we avoid filename + * clashes and make it easy to find the source image. + * + * @param string $path + * The path to add the extension to. + * + * @return string + * The given path if this image style doesn't change its extension, or the + * path with the added extension if it does. + */ + protected function addExtension($path) { + $extension = pathinfo($path, PATHINFO_EXTENSION); + $original_extension = $extension; + $this->getDerivativeExtension($extension); + if ($original_extension !== $extension) { + $path .= '.' . $extension; + } + return $path; + } + + /** + * Provides a wrapper for file_uri_scheme() to allow unit testing. + * + * Returns the scheme of a URI (e.g. a stream). + * + * @param string $uri + * A stream, referenced as "scheme://target" or "data:target". + * + * @see file_uri_target() + * + * @todo: Remove when https://www.drupal.org/node/2050759 is in. + * + * @return string + * A string containing the name of the scheme, or FALSE if none. For example, + * the URI "public://example.txt" would return "public". + */ + protected function fileUriScheme($uri) { + return file_uri_scheme($uri); + } + + /** + * Provides a wrapper for file_uri_target() to allow unit testing. + * + * Returns the part of a URI after the schema. + * + * @param string $uri + * A stream, referenced as "scheme://target" or "data:target". + * + * @see file_uri_scheme() + * + * @todo: Convert file_uri_target() into a proper injectable service. + * + * @return string|bool + * A string containing the target (path), or FALSE if none. + * For example, the URI "public://sample/test.txt" would return + * "sample/test.txt". + */ + protected function fileUriTarget($uri) { + return file_uri_target($uri); + } + + /** + * Provides a wrapper for file_default_scheme() to allow unit testing. + * + * Gets the default file stream implementation. + * + * @todo: Convert file_default_scheme() into a proper injectable service. + * + * @return + * 'public', 'private' or any other file scheme defined as the default. + */ + protected function fileDefaultScheme() { + return file_default_scheme(); + } + } diff --git a/core/modules/image/src/ImageEffectInterface.php b/core/modules/image/src/ImageEffectInterface.php index eddcf62..5b272f9 100644 --- a/core/modules/image/src/ImageEffectInterface.php +++ b/core/modules/image/src/ImageEffectInterface.php @@ -47,7 +47,7 @@ public function transformDimensions(array &$dimensions); * Determines the MIME type of the derivative without generating it. * * @param string $mime_type - * The MIME type to be modified. + * The MIME type to be set to the derivative's MIME type. */ public function getDerivativeMimeType(&$mime_type); @@ -55,7 +55,7 @@ public function getDerivativeMimeType(&$mime_type); * Determines the extension of the derivative without generating it. * * @param string $extension - * Extension to be modified. + * The file extension to be set to the derivative's file extension. */ public function getDerivativeExtension(&$extension); diff --git a/core/modules/image/src/ImageStyleInterface.php b/core/modules/image/src/ImageStyleInterface.php index 18f373b..91a2e9c 100644 --- a/core/modules/image/src/ImageStyleInterface.php +++ b/core/modules/image/src/ImageStyleInterface.php @@ -135,18 +135,15 @@ public function transformDimensions(array &$dimensions); * Determines the MIME type of the derivative without generating it. * * @param string $mime_type - * The MIME type to be modified. + * The MIME type to be set to the derivative's MIME type. */ public function getDerivativeMimeType(&$mime_type); /** * Determines the extension of the derivative without generating it. * - * Stores the extension of this image style into $extension. - * * @param string $extension - * String passed by reference. Implementations have to store the - * resulting extension, without the leading dot. + * The file extension to be set to the derivative's file extension. */ public function getDerivativeExtension(&$extension); @@ -190,30 +187,4 @@ public function addImageEffect(array $configuration); */ public function deleteImageEffect(ImageEffectInterface $effect); - /** - * Returns the image effect plugin manager. - * - * @return \Drupal\Component\Plugin\PluginManagerInterface - * The image effect plugin manager. - */ - public function getImageEffectPluginManager(); - - /** - * Gets the Drupal private key. - * - * @return string - * The Drupal private key. - */ - public function getPrivateKey(); - - /** - * Gets a salt useful for hardening against SQL injection. - * - * @return string - * A salt based on information in settings.php, not in the database. - * - * @throws \RuntimeException - */ - public function getHashSalt(); - } diff --git a/core/modules/image/src/Plugin/ImageEffect/ConvertImageEffect.php b/core/modules/image/src/Plugin/ImageEffect/ConvertImageEffect.php index 232ac2f..5091e51 100644 --- a/core/modules/image/src/Plugin/ImageEffect/ConvertImageEffect.php +++ b/core/modules/image/src/Plugin/ImageEffect/ConvertImageEffect.php @@ -44,7 +44,8 @@ public function applyEffect(ImageInterface $image) { * {@inheritdoc} */ public function getDerivativeMimeType(&$mime_type) { - // Guess needs a filename. + // The guess method on the MIME type guesser needs a full filename. It does + // not work with just an extension. $mime_type = \Drupal::service('file.mime_type.guesser.extension')->guess('test.' . $this->configuration['format']); } diff --git a/core/modules/image/src/Tests/ImageStyleTest.php b/core/modules/image/src/Tests/ImageStyleTest.php index 98e45a1..0a005cb 100644 --- a/core/modules/image/src/Tests/ImageStyleTest.php +++ b/core/modules/image/src/Tests/ImageStyleTest.php @@ -13,7 +13,6 @@ /** * @coversDefaultClass \Drupal\image\Entity\ImageStyle * - * @group Drupal * @group Image */ class ImageStyleTest extends UnitTestCase { @@ -39,12 +38,11 @@ class ImageStyleTest extends UnitTestCase { */ protected $entityTypeId; - /** * Gets a mocked image style for testing. * * @param string $image_effect_id - * The image effect id. + * The image effect ID. * @param \Drupal\image\ImageEffectInterface|\PHPUnit_Framework_MockObject_MockObject $image_effect * The image effect used for testing. * @@ -59,18 +57,32 @@ protected function getImageStyleMock($image_effect_id, $image_effect, $stubs = a ->method('createInstance') ->with($image_effect_id) ->will($this->returnValue($image_effect)); - + $default_stubs = array( + 'getImageEffectPluginManager', + 'fileUriScheme', + 'fileUriTarget', + 'fileDefaultScheme', + ); $image_style = $this->getMockBuilder('\Drupal\image\Entity\ImageStyle') ->setConstructorArgs(array( array('effects' => array($image_effect_id => array('id' => $image_effect_id))), $this->entityTypeId, )) - ->setMethods(array_merge(array('getImageEffectPluginManager'), $stubs)) + ->setMethods(array_merge($default_stubs, $stubs)) ->getMock(); $image_style->expects($this->any()) ->method('getImageEffectPluginManager') ->will($this->returnValue($effectManager)); + $image_style->expects($this->any()) + ->method('fileUriScheme') + ->will($this->returnCallback(array($this, 'fileUriScheme'))); + $image_style->expects($this->any()) + ->method('fileUriTarget') + ->will($this->returnCallback(array($this, 'fileUriTarget'))); + $image_style->expects($this->any()) + ->method('fileDefaultScheme') + ->will($this->returnCallback(array($this, 'fileDefaultScheme'))); return $image_style; } @@ -140,7 +152,6 @@ public function testGetDerivativeExtension() { * @covers ::buildUri */ public function testBuildUri() { - include_once DRUPAL_ROOT . '/core/includes/file.inc'; // Image style that changes the extension. $image_effect_id = $this->randomMachineName(); $logger = $this->getMockBuilder('\Psr\Log\LoggerInterface')->getMock(); @@ -165,7 +176,6 @@ public function testBuildUri() { $image_style = $this->getImageStyleMock($image_effect_id, $image_effect); $this->assertEquals($image_style->buildUri('public://test.jpeg'), 'public://styles/' . $image_style->id() . '/public/test.jpeg'); - } /** @@ -193,7 +203,7 @@ public function testGetPathToken() { ->method('getHashSalt') ->will($this->returnValue($hash_salt)); - // Assert the extension has been added to the uri before creating the token. + // Assert the extension has been added to the URI before creating the token. $this->assertEquals($image_style->getPathToken('public://test.jpeg.png'), $image_style->getPathToken('public://test.jpeg')); $this->assertEquals(substr(Crypt::hmacBase64($image_style->id() . ':' . 'public://test.jpeg.png', $private_key . $hash_salt), 0, 8), $image_style->getPathToken('public://test.jpeg')); $this->assertNotEquals(substr(Crypt::hmacBase64($image_style->id() . ':' . 'public://test.jpeg', $private_key . $hash_salt), 0, 8), $image_style->getPathToken('public://test.jpeg')); @@ -218,7 +228,37 @@ public function testGetPathToken() { $this->assertNotEquals($image_style->getPathToken('public://test.jpeg.png'), $image_style->getPathToken('public://test.jpeg')); $this->assertNotEquals(substr(Crypt::hmacBase64($image_style->id() . ':' . 'public://test.jpeg.png', $private_key . $hash_salt), 0, 8), $image_style->getPathToken('public://test.jpeg')); $this->assertEquals(substr(Crypt::hmacBase64($image_style->id() . ':' . 'public://test.jpeg', $private_key . $hash_salt), 0, 8), $image_style->getPathToken('public://test.jpeg')); + } + + /** + * Mock function for ImageStyle::fileUriScheme(). + */ + public function fileUriScheme($uri) { + if (preg_match('/^([\w\-]+):\/\/|^(data):/', $uri, $matches)) { + // The scheme will always be the last element in the matches array. + return array_pop($matches); + } + + return FALSE; + } + + /** + * Mock function for ImageStyle::fileUriTarget(). + */ + public function fileUriTarget($uri) { + // Remove the scheme from the URI and remove erroneous leading or trailing, + // forward-slashes and backslashes. + $target = trim(preg_replace('/^[\w\-]+:\/\/|^data:/', '', $uri), '\/'); + + // If nothing was replaced, the URI doesn't have a valid scheme. + return $target !== $uri ? $target : FALSE; + } + /** + * Mock function for ImageStyle::fileDefaultScheme(). + */ + public function fileDefaultScheme() { + return 'public'; } }