From 259f957ce89be071ddd7ba850c8c941fe51ef59b Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 11 May 2017 17:38:29 +1200 Subject: [PATCH] API Rename services to match FQN of interface / classes --- .upgrade.yml | 4 ++ _config/logging.yml | 41 +++++++++---- composer.json | 1 + .../01_Templates/03_Requirements.md | 26 ++++---- .../14_Files/01_File_Management.md | 6 +- .../14_Files/03_File_Security.md | 4 +- src/Control/HTTPResponse.php | 13 ++-- src/Control/Session.php | 2 +- src/Core/Core.php | 3 +- src/Dev/CliDebugView.php | 5 ++ src/Dev/Debug.php | 4 +- src/Logging/ErrorHandler.php | 14 +++++ src/Logging/HTTPOutputHandler.php | 22 +++++++ src/Logging/MonologErrorHandler.php | 9 +-- src/View/Requirements_Backend.php | 1 + tests/php/Core/Cache/CacheTest.php | 38 +++++------- .../DebugViewFriendlyErrorFormatterTest.php | 37 ++++++++++++ .../Logging/DetailedErrorFormatterTest.php | 30 ++++++++++ .../ErrorGenerator.php | 37 ++++++++++++ tests/php/Logging/HTTPOutputHandlerTest.php | 59 +++++++++++++++++++ 20 files changed, 286 insertions(+), 70 deletions(-) create mode 100644 src/Logging/ErrorHandler.php create mode 100644 tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php create mode 100644 tests/php/Logging/DetailedErrorFormatterTest.php create mode 100644 tests/php/Logging/DetailedErrorFormatterTest/ErrorGenerator.php create mode 100644 tests/php/Logging/HTTPOutputHandlerTest.php diff --git a/.upgrade.yml b/.upgrade.yml index c48c0cdc5..c793add3c 100644 --- a/.upgrade.yml +++ b/.upgrade.yml @@ -409,6 +409,10 @@ mappings: SilverStripe\Framework\Logging\DetailedErrorFormatter: SilverStripe\Logging\DetailedErrorFormatter SilverStripe\Framework\Logging\HTTPOutputHandler: SilverStripe\Logging\HTTPOutputHandler SilverStripe\Framework\Logging\MonologErrorHandler: SilverStripe\Logging\MonologErrorHandler + DebugViewFriendlyErrorFormatter: Monolog\Formatter\FormatterInterface.friendly + DetailedErrorFormatter: Monolog\Formatter\FormatterInterface.detailed + DisplayErrorHandler: Monolog\Handler\HandlerInterface + ErrorHandler: SilverStripe\Logging\ErrorHandler; SS_Log: SilverStripe\Logging\Log SilverStripe\Logging\SS_Log: SilverStripe\Logging\Log ComparisonFilter: SilverStripe\ORM\Filters\ComparisonFilter diff --git a/_config/logging.yml b/_config/logging.yml index 6bfe98007..f2f0de2ed 100644 --- a/_config/logging.yml +++ b/_config/logging.yml @@ -1,8 +1,13 @@ --- Name: logging --- +# Logging is built up of a chain containing: +# - A top level \ErrorHandler which registers the error service +# - A \Logger which acts as the error service +# - A \HandlerInterface which handles errors for the logger +# - One or more \FormatterInterface which format errors for the handler SilverStripe\Core\Injector\Injector: - ErrorHandler: + SilverStripe\Logging\ErrorHandler: class: SilverStripe\Logging\MonologErrorHandler properties: Logger: %$Psr\Log\LoggerInterface @@ -12,35 +17,45 @@ SilverStripe\Core\Injector\Injector: constructor: - "error-log" calls: - DisplayErrorHandler: [ pushHandler, [ %$DisplayErrorHandler ] ] - + DisplayErrorHandler: [ pushHandler, [ %$Monolog\Handler\HandlerInterface ] ] +--- +Name: loggingformatters +--- +SilverStripe\Core\Injector\Injector: + # Display detailed information on an error + Monolog\Formatter\FormatterInterface.detailed: + class: SilverStripe\Logging\DetailedErrorFormatter + # Display friendly error messages and suppresses possible disclosure of dev configuration + Monolog\Formatter\FormatterInterface.friendly: + class: SilverStripe\Logging\DebugViewFriendlyErrorFormatter + properties: + Title: "There has been an error" + Body: "The website server has not been able to respond to your request" --- Name: dev-logging Only: environment: dev --- +# Dev handler outputs detailed information including notices SilverStripe\Core\Injector\Injector: - DisplayErrorHandler: + Monolog\Handler\HandlerInterface: class: SilverStripe\Logging\HTTPOutputHandler constructor: - "notice" properties: - Formatter: %$SilverStripe\Logging\DetailedErrorFormatter + DefaultFormatter: %$Monolog\Formatter\FormatterInterface.detailed --- Name: live-logging Except: environment: dev --- +# Live handler outputs user-friendly error details, and ignores notices +# CLI errors still show full details SilverStripe\Core\Injector\Injector: - DisplayErrorHandler: + Monolog\Handler\HandlerInterface: class: SilverStripe\Logging\HTTPOutputHandler constructor: - "error" properties: - Formatter: %$SilverStripe\Logging\DebugViewFriendlyErrorFormatter - CLIFormatter: %$SilverStripe\Logging\DetailedErrorFormatter - SilverStripe\Logging\DebugViewFriendlyErrorFormatter: - class: SilverStripe\Logging\DebugViewFriendlyErrorFormatter - properties: - Title: "There has been an error" - Body: "The website server has not been able to respond to your request" + DefaultFormatter: %$Monolog\Formatter\FormatterInterface.friendly + CLIFormatter: %$Monolog\Formatter\FormatterInterface.detailed diff --git a/composer.json b/composer.json index 120e0a50e..f51c805ea 100644 --- a/composer.json +++ b/composer.json @@ -66,6 +66,7 @@ "SilverStripe\\i18n\\": "src/i18n/", "SilverStripe\\i18n\\Tests\\": "tests/php/i18n/", "SilverStripe\\Logging\\": "src/Logging/", + "SilverStripe\\Logging\\Tests\\": "tests/php/Logging/", "SilverStripe\\ORM\\": "src/ORM/", "SilverStripe\\ORM\\Tests\\": "tests/php/ORM/", "SilverStripe\\Security\\": "src/Security/", diff --git a/docs/en/02_Developer_Guides/01_Templates/03_Requirements.md b/docs/en/02_Developer_Guides/01_Templates/03_Requirements.md index 8cbeb9767..e59d380a2 100644 --- a/docs/en/02_Developer_Guides/01_Templates/03_Requirements.md +++ b/docs/en/02_Developer_Guides/01_Templates/03_Requirements.md @@ -176,25 +176,25 @@ replaced. For instance, the below will set a new set of dependencies to write to combine_hash_querystring: true default_combined_files_folder: 'combined' SilverStripe\Core\Injector\Injector: - MySiteAdapter: - class: 'SilverStripe\Filesystem\Flysystem\AssetAdapter' + # Create adapter that points to the custom directory root + SilverStripe\Assets\Flysystem\PublicAdapter.custom-adapter: + class: SilverStripe\Assets\Flysystem\PublicAssetAdapter constructor: - Root: ./mysite/javascript - # Define the default filesystem - MySiteBackend: + Root: ./mysite/javascript + # Set flysystem filesystem that uses this adapter + League\Flysystem\Filesystem.custom-filesystem: class: 'League\Flysystem\Filesystem' constructor: - Adapter: '%$MySiteAdapter' - calls: - PublicURLPlugin: [ addPlugin, [ %$FlysystemUrlPlugin ] ] - # Requirements config - MySiteAssetHandler: - class: SilverStripe\Filesystem\Storage\FlysystemGeneratedAssetHandler + Adapter: '%$SilverStripe\Assets\Flysystem\PublicAdapter.custom-adapter' + # Create handler to generate assets using this filesystem + SilverStripe\Assets\Storage\GeneratedAssetHandler.custom-generated-assets: + class: SilverStripe\Assets\Flysystem\GeneratedAssets properties: - Filesystem: '%$MySiteBackend' + Filesystem: %$League\Flysystem\Filesystem.custom-filesystem + # Assign this generator to the requirements builder SilverStripe\View\Requirements_Backend: properties: - AssetHandler: '%$MySiteAssetHandler' + AssetHandler: '%$SilverStripe\Assets\Storage\GeneratedAssetHandler.custom-generated-assets' In the above configuration, automatic expiry of generated files has been disabled, and it is necessary for the developer to maintain these files manually. This may be useful in environments where assets must diff --git a/docs/en/02_Developer_Guides/14_Files/01_File_Management.md b/docs/en/02_Developer_Guides/14_Files/01_File_Management.md index cf940d3cc..fc7bd5635 100644 --- a/docs/en/02_Developer_Guides/14_Files/01_File_Management.md +++ b/docs/en/02_Developer_Guides/14_Files/01_File_Management.md @@ -35,12 +35,12 @@ Because the filesystem now uses the sha1 of file contents in order to version mu filename, the default storage paths in 4.0 will not be the same as in 3. In order to retain existing file paths in line with framework version 3 you should set the -`\SilverStripe\Filesystem\Flysystem\FlysystemAssetStore.legacy_paths` config to true. +`\SilverStripe\Assets\Flysystem\FlysystemAssetStore.legacy_filenames` config to true. Note that this will not allow you to utilise certain file versioning features in 4.0. :::yaml - \SilverStripe\Filesystem\Flysystem\FlysystemAssetStore: - legacy_paths: true + \SilverStripe\Assets\Flysystem\FlysystemAssetStore: + legacy_filenames: true ## Loading content into `DBFile` diff --git a/docs/en/02_Developer_Guides/14_Files/03_File_Security.md b/docs/en/02_Developer_Guides/14_Files/03_File_Security.md index 19d6d4579..9c85181e1 100644 --- a/docs/en/02_Developer_Guides/14_Files/03_File_Security.md +++ b/docs/en/02_Developer_Guides/14_Files/03_File_Security.md @@ -19,7 +19,7 @@ config option: :::php - $store = singleton('AssetStore'); + $store = singleton(AssetStore::class); $store->setFromString('My protected content', 'Documents/Mydocument.txt', null, null, array( 'visibility' => AssetStore::VISIBILITY_PROTECTED )); @@ -182,7 +182,7 @@ the web server, bypassing the need for additional PHP requests. :::php - $store = singleton('AssetStore'); + $store = singleton(AssetStore::class); $store->publish('NewCompanyLogo.gif', 'a870de278b475cb75f5d9f451439b2d378e13af1'); diff --git a/src/Control/HTTPResponse.php b/src/Control/HTTPResponse.php index 6e35aa9a0..4060509c8 100644 --- a/src/Control/HTTPResponse.php +++ b/src/Control/HTTPResponse.php @@ -3,6 +3,8 @@ namespace SilverStripe\Control; use InvalidArgumentException; +use Monolog\Formatter\FormatterInterface; +use Monolog\Handler\HandlerInterface; use SilverStripe\Core\Convert; use SilverStripe\Core\Injector\Injectable; use SilverStripe\Core\Injector\Injector; @@ -306,11 +308,12 @@ EOT } } - // Only show error pages or generic "friendly" errors if the status code signifies - // an error, and the response doesn't have any body yet that might contain - // a more specific error description. - if (Director::isLive() && $this->isError() && !$this->body) { - $formatter = Injector::inst()->get('FriendlyErrorFormatter'); + // If this is an error but no error body has yet been generated, + // delegate formatting to current error handler. + if ($this->isError() && !$this->body) { + /** @var HandlerInterface $handler */ + $handler = Injector::inst()->get(HandlerInterface::class); + $formatter = $handler->getFormatter(); echo $formatter->format(array( 'code' => $this->statusCode )); diff --git a/src/Control/Session.php b/src/Control/Session.php index e502516f6..4b9edeb7b 100644 --- a/src/Control/Session.php +++ b/src/Control/Session.php @@ -183,7 +183,7 @@ class Session * Set a key/value pair in the session * * @param string $name Key - * @param string $val Value + * @param string|array $val Value */ public static function set($name, $val) { diff --git a/src/Core/Core.php b/src/Core/Core.php index 8cfe5a3ce..fa9a889ad 100644 --- a/src/Core/Core.php +++ b/src/Core/Core.php @@ -12,6 +12,7 @@ use SilverStripe\Control\Director; use SilverStripe\Core\Manifest\ModuleLoader; use SilverStripe\Core\Manifest\ModuleManifest; use SilverStripe\i18n\i18n; +use SilverStripe\Logging\ErrorHandler; /** * This file is the Framework bootstrap. It will get your environment ready to call Director::direct(). @@ -116,7 +117,7 @@ if (Director::isLive()) { /** * Load error handlers */ -$errorHandler = Injector::inst()->get('ErrorHandler'); +$errorHandler = Injector::inst()->get(ErrorHandler::class); $errorHandler->start(); /////////////////////////////////////////////////////////////////////////////// diff --git a/src/Dev/CliDebugView.php b/src/Dev/CliDebugView.php index 976177e37..3c13b1086 100644 --- a/src/Dev/CliDebugView.php +++ b/src/Dev/CliDebugView.php @@ -88,6 +88,11 @@ class CliDebugView extends DebugView return $output; } + public function renderParagraph($text) + { + return wordwrap($text, self::config()->columns) . "\n\n"; + } + /** * Render the information header for the view * diff --git a/src/Dev/Debug.php b/src/Dev/Debug.php index 06d7ec90d..669f8f59a 100644 --- a/src/Dev/Debug.php +++ b/src/Dev/Debug.php @@ -186,8 +186,8 @@ class Debug public static function create_debug_view() { $service = Director::is_cli() || Director::is_ajax() - ? 'SilverStripe\\Dev\\CliDebugView' - : 'SilverStripe\\Dev\\DebugView'; + ? CliDebugView::class + : DebugView::class; return Injector::inst()->get($service); } diff --git a/src/Logging/ErrorHandler.php b/src/Logging/ErrorHandler.php new file mode 100644 index 000000000..d562fa028 --- /dev/null +++ b/src/Logging/ErrorHandler.php @@ -0,0 +1,14 @@ +getDefaultFormatter(); + } + + /** + * Check default formatter to use + * + * @return FormatterInterface + */ + public function getDefaultFormatter() + { return parent::getFormatter(); } + /** + * Set default formatter + * + * @param FormatterInterface $formatter + * @return $this + */ + public function setDefaultFormatter(FormatterInterface $formatter) + { + parent::setFormatter($formatter); + return $this; + } + /** * @param array $record * @return bool diff --git a/src/Logging/MonologErrorHandler.php b/src/Logging/MonologErrorHandler.php index 25ce3fe8f..8d2ccdb80 100644 --- a/src/Logging/MonologErrorHandler.php +++ b/src/Logging/MonologErrorHandler.php @@ -3,12 +3,9 @@ namespace SilverStripe\Logging; use Psr\Log\LoggerInterface; -use Monolog\ErrorHandler; +use Monolog\ErrorHandler as MonologHandler; -/** - * Simple adaptor to start Monolog\ErrorHandler - */ -class MonologErrorHandler +class MonologErrorHandler implements ErrorHandler { /** * @var LoggerInterface @@ -32,6 +29,6 @@ class MonologErrorHandler . "Is your Injector config correct?"); } - ErrorHandler::register($this->logger); + MonologHandler::register($this->logger); } } diff --git a/src/View/Requirements_Backend.php b/src/View/Requirements_Backend.php index 0f55ef690..d3a8853c6 100644 --- a/src/View/Requirements_Backend.php +++ b/src/View/Requirements_Backend.php @@ -1288,6 +1288,7 @@ class Requirements_Backend * @param array $fileList List of files to combine * @param string $type Either 'js' or 'css' * @return string|null URL to this resource, if there are files to combine + * @throws Exception */ protected function getCombinedFileURL($combinedFile, $fileList, $type) { diff --git a/tests/php/Core/Cache/CacheTest.php b/tests/php/Core/Cache/CacheTest.php index 393870297..934a3e033 100644 --- a/tests/php/Core/Cache/CacheTest.php +++ b/tests/php/Core/Cache/CacheTest.php @@ -5,7 +5,6 @@ namespace SilverStripe\Core\Tests\Cache; use Psr\SimpleCache\CacheInterface; use SilverStripe\Core\Cache\ApcuCacheFactory; use SilverStripe\Core\Cache\MemcachedCacheFactory; -use SilverStripe\Core\Config\Config; use SilverStripe\Core\Injector\Injector; use SilverStripe\Core\Test\Cache\CacheTest\MockCache; use SilverStripe\Dev\SapphireTest; @@ -18,38 +17,29 @@ class CacheTest extends SapphireTest { parent::setUp(); - Config::modify() - ->set( - Injector::class, - ApcuCacheFactory::class, - [ - 'constructor' => [ 'version' => 4400 ] - ] - ) - ->set( - Injector::class, - CacheInterface::class . '.TestApcuCache', - [ + Injector::inst() + ->load([ + ApcuCacheFactory::class => [ + 'constructor' => [ 'version' => 'ss40test' ] + ], + MemcachedCacheFactory::class => MemcachedCacheFactory::class, + CacheInterface::class . '.TestApcuCache' => [ 'factory' => ApcuCacheFactory::class, 'constructor' => [ 'namespace' => 'TestApcuCache', 'defaultLifetime' => 2600, ], - ] - ) - ->set( - Injector::class, - CacheInterface::class . '.TestMemcache', - [ + ], + CacheInterface::class . '.TestMemcache' => [ 'factory' => MemcachedCacheFactory::class, 'constructor' => [ 'namespace' => 'TestMemCache', 'defaultLifetime' => 5600, ], - ] - ) - ->set(Injector::class, ApcuCache::class, MockCache::class) - ->set(Injector::class, MemcachedCache::class, MockCache::class); + ], + ApcuCache::class => MockCache::class, + MemcachedCache::class => MockCache::class, + ]); } public function testApcuCacheFactory() @@ -63,7 +53,7 @@ class CacheTest extends SapphireTest [ 'TestApcuCache_'.md5(BASE_PATH), 2600, - 4400 + 'ss40test' ], $cache->getArgs() ); diff --git a/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php b/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php new file mode 100644 index 000000000..49d834457 --- /dev/null +++ b/tests/php/Logging/DebugViewFriendlyErrorFormatterTest.php @@ -0,0 +1,37 @@ +set('admin_email', 'testy@mctest.face'); + } + + public function testOutput() + { + $formatter = new DebugViewFriendlyErrorFormatter(); + $formatter->setTitle("There has been an error"); + $formatter->setBody("The website server has not been able to respond to your request"); + + $expected = <<assertEquals($expected, $formatter->output(404)); + } +} diff --git a/tests/php/Logging/DetailedErrorFormatterTest.php b/tests/php/Logging/DetailedErrorFormatterTest.php new file mode 100644 index 000000000..3e6e979bd --- /dev/null +++ b/tests/php/Logging/DetailedErrorFormatterTest.php @@ -0,0 +1,30 @@ +mockException(); + + $output = ''.$formatter->format(['context' => [ + 'exception' => $exception, + ]]); + + $base = __DIR__; + $this->assertContains('ERROR [Emergency]: Uncaught Exception: Error', $output); + $this->assertContains("Line 32 in $base/DetailedErrorFormatterTest/ErrorGenerator.php", $output); + $this->assertContains('* 32: throw new Exception(\'Error\');', $output); + $this->assertContains( + 'SilverStripe\\Logging\\Tests\\DetailedErrorFormatterTest\\ErrorGenerator->mockException(4)', + $output + ); + } +} diff --git a/tests/php/Logging/DetailedErrorFormatterTest/ErrorGenerator.php b/tests/php/Logging/DetailedErrorFormatterTest/ErrorGenerator.php new file mode 100644 index 000000000..cfd0a5751 --- /dev/null +++ b/tests/php/Logging/DetailedErrorFormatterTest/ErrorGenerator.php @@ -0,0 +1,37 @@ +mockException(1); + } catch (\Exception $ex) { + return $ex; + } + return null; + break; + case 4: + throw new Exception('Error'); + default: + return $this->mockException($depth + 1); + } + } +} diff --git a/tests/php/Logging/HTTPOutputHandlerTest.php b/tests/php/Logging/HTTPOutputHandlerTest.php new file mode 100644 index 000000000..352fdf13d --- /dev/null +++ b/tests/php/Logging/HTTPOutputHandlerTest.php @@ -0,0 +1,59 @@ +markTestSkipped("This test only runs in CLI mode"); + } + if (!Director::isDev()) { + $this->markTestSkipped("This test only runs in dev mode"); + } + } + + public function testGetFormatter() + { + $handler = new HTTPOutputHandler(); + + $detailedFormatter = new DetailedErrorFormatter(); + $friendlyFormatter = new DebugViewFriendlyErrorFormatter(); + + // Handler without CLIFormatter chooses correct formatter + $handler->setDefaultFormatter($detailedFormatter); + $this->assertInstanceOf(DetailedErrorFormatter::class, $handler->getFormatter()); + $this->assertInstanceOf(DetailedErrorFormatter::class, $handler->getDefaultFormatter()); + + // Handler with CLIFormatter should return that, although default handler is still accessible + $handler->setCLIFormatter($friendlyFormatter); + $this->assertInstanceOf(DebugViewFriendlyErrorFormatter::class, $handler->getFormatter()); + $this->assertInstanceOf(DetailedErrorFormatter::class, $handler->getDefaultFormatter()); + } + + /** + * Covers `#dev-logging` section in logging.yml + */ + public function testDevConfig() + { + /** @var HTTPOutputHandler $handler */ + $handler = Injector::inst()->get(HandlerInterface::class); + $this->assertInstanceOf(HTTPOutputHandler::class, $handler); + + // Test only default formatter is set, but CLI specific formatter is left out + $this->assertNull($handler->getCLIFormatter()); + $this->assertInstanceOf(DetailedErrorFormatter::class, $handler->getDefaultFormatter()); + $this->assertInstanceOf(DetailedErrorFormatter::class, $handler->getFormatter()); + } +}