diff --git a/core/lib/Drupal/Core/Logger/LogMessageParser.php b/core/lib/Drupal/Core/Logger/LogMessageParser.php index 8a67df1..be10b0f 100644 --- a/core/lib/Drupal/Core/Logger/LogMessageParser.php +++ b/core/lib/Drupal/Core/Logger/LogMessageParser.php @@ -2,6 +2,8 @@ namespace Drupal\Core\Logger; +use Psr\Log\InvalidArgumentException; + /** * Parses log messages and their placeholders. */ @@ -29,13 +31,72 @@ public function parseMessagePlaceholders(&$message, array &$context) { $key = '@' . $key; } } - if (!empty($key) && ($key[0] === '@' || $key[0] === '%' || $key[0] === '!')) { + + if (!empty($key) && ($key[0] === '@' || $key[0] === '%' || $key[0] === ':')) { + if (!$this->placeholderIsStringable($variable)) { + $this->thrownBadApiUseException($key); + } + // The key is now in \Drupal\Component\Utility\SafeMarkup::format() style. - $variables[$key] = $variable; + $variables[$key] = (string) $variable; } } return $variables; } + /** + * Check if a placeholder value can be converted into a string. + * + * @param $placeholder + * The placeholder value to analyze. + * + * @return bool + * Returns TRUE if the placeholder can be converted into a string. + */ + protected function placeholderIsStringable($placeholder) { + if (is_resource($placeholder)) { + return FALSE; + } + + if (is_object($placeholder) && method_exists($placeholder, '__toString')) { + return TRUE; + } + + return is_null($placeholder) || is_scalar($placeholder); + } + + /** + * Throwns an exception indicating the logger API is being used incorrectly. + * + * @param string $key + * A log context key. + * + * @throws Psr\Log\InvalidArgumentException + */ + protected function thrownBadApiUseException($key) { + $debug_backtrace = debug_backtrace(); + $relevant_backtrace = NULL; + foreach ($debug_backtrace as $index => $backtrace) { + if (!empty($backtrace['function']) && + !empty($backtrace['class']) && + $backtrace['function'] == 'log' && + $backtrace['class'] == 'Drupal\\Core\\Logger\\LoggerChannel') { + + // Drupal\Core\Logger\LoggerChannel::log is called by the function that + // is passing non stringable arguments. + $relevant_backtrace = $debug_backtrace[$index + 1]; + break; + } + } + + $exception_msg = ''; + if (!empty($relevant_backtrace) && !empty($relevant_backtrace['file']) && $relevant_backtrace['line']) { + $exception_msg = 'There is a missuse of the logging API. Please check line ' . $relevant_backtrace['line'] . ' of ' . $relevant_backtrace['file'] . '. '; + } + $exception_msg .= 'Log context contains non-string value for key "' . $key . '"'; + + throw new InvalidArgumentException($exception_msg); + } + } diff --git a/core/modules/comment/tests/src/Kernel/CommentIntegrationTest.php b/core/modules/comment/tests/src/Kernel/CommentIntegrationTest.php index af73034..9029b7e 100644 --- a/core/modules/comment/tests/src/Kernel/CommentIntegrationTest.php +++ b/core/modules/comment/tests/src/Kernel/CommentIntegrationTest.php @@ -51,6 +51,7 @@ public function testViewMode() { // Create a new comment view mode and a view display entity. EntityViewMode::create([ 'id' => "comment.$mode", + 'label' => "comment.$mode", 'targetEntityType' => 'comment', 'settings' => ['comment_type' => 'comment'], ])->save(); diff --git a/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php b/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php index 395517f..381b251 100644 --- a/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php +++ b/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php @@ -4,6 +4,23 @@ use Drupal\Core\Logger\LogMessageParser; use Drupal\Tests\UnitTestCase; +use Psr\Log\InvalidArgumentException; + +/** + * A class that isn't string convertible. + */ +class NotStringConvertible { } + +/** + * A class that is string convertible. + */ +class StringConvertible { + + public function __toString() { + return 'convertible'; + } + +} /** * @coversDefaultClass \Drupal\Core\Logger\LogMessageParser @@ -34,9 +51,36 @@ public function testParseMessagePlaceholders(array $value, array $expected) { } /** + * Test for LogMessageParserTrait::parseMessagePlaceholders() + * + * @param array $value + * An array containing: + * - message: A string that contains a message with placeholders. + * - context: An array with placeholder values. + * @param array $expected + * An array with the expected values after the test has run. + * - message: The expected parsed message. + * - context: The expected values of the placeholders. + * + * @dataProvider providerTestParseMessageNonStringablePlaceholders + * @covers ::parseMessagePlaceholders + */ + public function testParseMessageInvalidPlaceholders(array $value, array $expected) { + $parser = new LogMessageParser(); + try { + $message_placeholders = $parser->parseMessagePlaceholders($value['message'], $value['context']); + $this->fail("Never executed."); + } + catch (InvalidArgumentException $e) { + $this->assertStringStartsWith('Log context contains non-string value for key', $e->getMessage()); + } + } + + /** * Data provider for testParseMessagePlaceholders(). */ public function providerTestParseMessagePlaceholders() { + // @codingStandardsIgnoreStart return [ // PSR3 only message. [ @@ -53,6 +97,11 @@ public function providerTestParseMessagePlaceholders() { ['message' => 'User @username created', 'context' => ['@username' => 'Dries']], ['message' => 'User @username created', 'context' => ['@username' => 'Dries']], ], + // format_string style message only. + [ + ['message' => 'Example :url', 'context' => [':url' => 'http://example.com']], + ['message' => 'Example :url', 'context' => [':url' => 'http://example.com']], + ], // Message without placeholders but wildcard characters. [ ['message' => 'User W-\\};~{&! created @', 'context' => ['' => '']], @@ -63,7 +112,53 @@ public function providerTestParseMessagePlaceholders() { ['message' => 'Test {with} two {encapsuled} strings', 'context' => ['with' => 'together', 'encapsuled' => 'awesome']], ['message' => 'Test @with two @encapsuled strings', 'context' => ['@with' => 'together', '@encapsuled' => 'awesome']], ], + // Placeholders convertible into a string. + [ + ['message' => 'object @b', 'context' => ['@b' => new StringConvertible()]], + ['message' => 'object @b', 'context' => ['@b' => 'convertible']], + ], + // Message without placeholders, but with other context values. + [ + ['message' => 'message', 'context' => ['not_a_placeholder' => new NotStringConvertible()]], + ['message' => 'message', 'context' => []], // All non placeholders are filtered out. + ], + ]; + // @codingStandardsIgnoreEnd + } + + /** + * Data provider for testParseMessageInvalidPlaceholders(). + */ + public function providerTestParseMessageNonStringablePlaceholders() { + // @codingStandardsIgnoreStart + return [ + // Placeholders not convertible into a string. + [ + ['message' => 'array @a', 'context' => ['@a' => []]], + ['message' => 'array @a', 'context' => ['@a' => []]], + ], + // Placeholders not convertible into a string. + [ + ['message' => 'object @b', 'context' => ['@b' => new NotStringConvertible()]], + ['message' => 'object @b', 'context' => ['@b' => new NotStringConvertible()]], + ], + // Closures are not convertible into a string. + [ + ['message' => 'closure @c', 'context' => ['@c' => function() {} ]], + ['message' => 'closure @c', 'context' => ['@c' => function() {} ]], + ], + // Resources cannot be converted into a string. + [ + ['message' => 'resource @r', 'context' => ['@r' => fopen('php://memory', 'r+') ]], + ['message' => 'resource @r', 'context' => ['@r' => fopen('php://memory', 'r+') ]], + ], + // Placeholders not converible into strings that are not the first placeholder. + [ + ['message' => 'mixed @a @b @c', 'context' => ['@a' => 123, '@b' => [1], '@c' => TRUE]], + ['message' => 'mixed @a @b @c', 'context' => ['@a' => 123, '@b' => [1], '@c' => TRUE]], + ], ]; + // @codingStandardsIgnoreEnd } }