From 83a24937ba50ab39f450faf8c448d6c68a293ea0 Mon Sep 17 00:00:00 2001 From: Remon van de Kamp Date: Fri, 20 May 2016 20:13:19 +0200 Subject: [PATCH] Add deprecation errors on RotatingFileHandler (#774) * Add deprecation errors when attempting to set dateFormats of fileFormats that break the possibility of rotating easily in RotatingFileHandler. Version 2.x of Monolog will throw `\InvalidArgumentException`s in these cases. --- src/Monolog/Handler/RotatingFileHandler.php | 18 +++ .../Handler/RotatingFileHandlerTest.php | 132 ++++++++++++++++-- 2 files changed, 140 insertions(+), 10 deletions(-) diff --git a/src/Monolog/Handler/RotatingFileHandler.php b/src/Monolog/Handler/RotatingFileHandler.php index 22e6c3dc..cd938057 100644 --- a/src/Monolog/Handler/RotatingFileHandler.php +++ b/src/Monolog/Handler/RotatingFileHandler.php @@ -24,6 +24,10 @@ use Monolog\Logger; */ class RotatingFileHandler extends StreamHandler { + const FILE_PER_DAY = 'Y-m-d'; + const FILE_PER_MONTH = 'Y-m'; + const FILE_PER_YEAR = 'Y'; + protected $filename; protected $maxFiles; protected $mustRotate; @@ -64,6 +68,20 @@ class RotatingFileHandler extends StreamHandler public function setFilenameFormat($filenameFormat, $dateFormat) { + if (!in_array($dateFormat, array(self::FILE_PER_DAY, self::FILE_PER_MONTH, self::FILE_PER_YEAR))) { + trigger_error( + 'Invalid date format - format should be one of '. + 'RotatingFileHandler::FILE_PER_DAY, RotatingFileHandler::FILE_PER_MONTH '. + 'or RotatingFileHandler::FILE_PER_YEAR.', + E_USER_DEPRECATED + ); + } + if (substr_count($filenameFormat, '{date}') === 0) { + trigger_error( + 'Invalid filename format - format should contain at least `{date}`, because otherwise rotating is impossible.', + E_USER_DEPRECATED + ); + } $this->filenameFormat = $filenameFormat; $this->dateFormat = $dateFormat; $this->url = $this->getTimedFilename(); diff --git a/tests/Monolog/Handler/RotatingFileHandlerTest.php b/tests/Monolog/Handler/RotatingFileHandlerTest.php index f4cefda1..96e6dff4 100644 --- a/tests/Monolog/Handler/RotatingFileHandlerTest.php +++ b/tests/Monolog/Handler/RotatingFileHandlerTest.php @@ -12,19 +12,52 @@ namespace Monolog\Handler; use Monolog\TestCase; +use PHPUnit_Framework_Error_Deprecated; /** * @covers Monolog\Handler\RotatingFileHandler */ class RotatingFileHandlerTest extends TestCase { + /** + * This var should be private but then the anonymous function + * in the `setUp` method won't be able to set it. `$this` cant't + * be used in the anonymous function in `setUp` because PHP 5.3 + * does not support it. + */ + public $lastError; + public function setUp() { $dir = __DIR__.'/Fixtures'; chmod($dir, 0777); if (!is_writable($dir)) { - $this->markTestSkipped($dir.' must be writeable to test the RotatingFileHandler.'); + $this->markTestSkipped($dir.' must be writable to test the RotatingFileHandler.'); } + $this->lastError = null; + $self = $this; + // workaround with &$self used for PHP 5.3 + set_error_handler(function($code, $message) use (&$self) { + $self->lastError = array( + 'code' => $code, + 'message' => $message, + ); + }); + } + + private function assertErrorWasTriggered($code, $message) + { + if (empty($this->lastError)) { + $this->fail( + sprintf( + 'Failed asserting that error with code `%d` and message `%s` was triggered', + $code, + $message + ) + ); + } + $this->assertEquals($code, $this->lastError['code'], sprintf('Expected an error with code %d to be triggered, got `%s` instead', $code, $this->lastError['code'])); + $this->assertEquals($message, $this->lastError['message'], sprintf('Expected an error with message `%d` to be triggered, got `%s` instead', $message, $this->lastError['message'])); } public function testRotationCreatesNewFile() @@ -43,14 +76,14 @@ class RotatingFileHandlerTest extends TestCase /** * @dataProvider rotationTests */ - public function testRotation($createFile) + public function testRotation($createFile, $dateFormat, $timeCallback) { - touch($old1 = __DIR__.'/Fixtures/foo-'.date('Y-m-d', time() - 86400).'.rot'); - touch($old2 = __DIR__.'/Fixtures/foo-'.date('Y-m-d', time() - 86400 * 2).'.rot'); - touch($old3 = __DIR__.'/Fixtures/foo-'.date('Y-m-d', time() - 86400 * 3).'.rot'); - touch($old4 = __DIR__.'/Fixtures/foo-'.date('Y-m-d', time() - 86400 * 4).'.rot'); + touch($old1 = __DIR__.'/Fixtures/foo-'.date($dateFormat, $timeCallback(-1)).'.rot'); + touch($old2 = __DIR__.'/Fixtures/foo-'.date($dateFormat, $timeCallback(-2)).'.rot'); + touch($old3 = __DIR__.'/Fixtures/foo-'.date($dateFormat, $timeCallback(-3)).'.rot'); + touch($old4 = __DIR__.'/Fixtures/foo-'.date($dateFormat, $timeCallback(-4)).'.rot'); - $log = __DIR__.'/Fixtures/foo-'.date('Y-m-d').'.rot'; + $log = __DIR__.'/Fixtures/foo-'.date($dateFormat).'.rot'; if ($createFile) { touch($log); @@ -58,6 +91,7 @@ class RotatingFileHandlerTest extends TestCase $handler = new RotatingFileHandler(__DIR__.'/Fixtures/foo.rot', 2); $handler->setFormatter($this->getIdentityFormatter()); + $handler->setFilenameFormat('{filename}-{date}', $dateFormat); $handler->handle($this->getRecord()); $handler->close(); @@ -72,11 +106,88 @@ class RotatingFileHandlerTest extends TestCase public function rotationTests() { + $now = time(); + $dayCallback = function($ago) use ($now) { + return $now + 86400 * $ago; + }; + $monthCallback = function($ago) { + return gmmktime(0, 0, 0, date('n') + $ago, date('d'), date('Y')); + }; + $yearCallback = function($ago) { + return gmmktime(0, 0, 0, date('n'), date('d'), date('Y') + $ago); + }; + return array( 'Rotation is triggered when the file of the current day is not present' - => array(true), - 'Rotation is not triggered when the file is already present' - => array(false), + => array(true, RotatingFileHandler::FILE_PER_DAY, $dayCallback), + 'Rotation is not triggered when the file of the current day is already present' + => array(false, RotatingFileHandler::FILE_PER_DAY, $dayCallback), + + 'Rotation is triggered when the file of the current month is not present' + => array(true, RotatingFileHandler::FILE_PER_MONTH, $monthCallback), + 'Rotation is not triggered when the file of the current month is already present' + => array(false, RotatingFileHandler::FILE_PER_MONTH, $monthCallback), + + 'Rotation is triggered when the file of the current year is not present' + => array(true, RotatingFileHandler::FILE_PER_YEAR, $yearCallback), + 'Rotation is not triggered when the file of the current year is already present' + => array(false, RotatingFileHandler::FILE_PER_YEAR, $yearCallback), + ); + } + + /** + * @dataProvider dateFormatProvider + */ + public function testAllowOnlyFixedDefinedDateFormats($dateFormat, $valid) + { + $handler = new RotatingFileHandler(__DIR__.'/Fixtures/foo.rot', 2); + $handler->setFilenameFormat('{filename}-{date}', $dateFormat); + if (!$valid) { + $this->assertErrorWasTriggered( + E_USER_DEPRECATED, + 'Invalid date format - format should be one of '. + 'RotatingFileHandler::FILE_PER_DAY, RotatingFileHandler::FILE_PER_MONTH '. + 'or RotatingFileHandler::FILE_PER_YEAR.' + ); + } + } + + public function dateFormatProvider() + { + return array( + array(RotatingFileHandler::FILE_PER_DAY, true), + array(RotatingFileHandler::FILE_PER_MONTH, true), + array(RotatingFileHandler::FILE_PER_YEAR, true), + array('m-d-Y', false), + array('Y-m-d-h-i', false) + ); + } + + /** + * @dataProvider filenameFormatProvider + */ + public function testDisallowFilenameFormatsWithoutDate($filenameFormat, $valid) + { + $handler = new RotatingFileHandler(__DIR__.'/Fixtures/foo.rot', 2); + $handler->setFilenameFormat($filenameFormat, RotatingFileHandler::FILE_PER_DAY); + if (!$valid) { + $this->assertErrorWasTriggered( + E_USER_DEPRECATED, + 'Invalid filename format - format should contain at least `{date}`, because otherwise rotating is impossible.' + ); + } + } + + public function filenameFormatProvider() + { + return array( + array('{filename}', false), + array('{filename}-{date}', true), + array('{date}', true), + array('foobar-{date}', true), + array('foo-{date}-bar', true), + array('{date}-foobar', true), + array('foobar', false), ); } @@ -95,5 +206,6 @@ class RotatingFileHandlerTest extends TestCase foreach (glob(__DIR__.'/Fixtures/*.rot') as $file) { unlink($file); } + restore_error_handler(); } }