From 3a589e5d4aa14939649e7399cd35fe9c412d9773 Mon Sep 17 00:00:00 2001 From: Quentin de Longraye Date: Sat, 29 Oct 2016 12:32:01 +0200 Subject: [PATCH 1/2] fix logger and improve code readability --- AbstractAdapter.php | 90 ++++++++++++++++--------------- Adapter/LoggerHelper.php | 40 ++++++++------ Adapter/ParameterBagInterface.php | 4 +- AdapterInterface.php | 2 + ParserInterface.php | 3 +- Tests/AbstractAdapterTest.php | 6 ++- Tests/AbstractTestCase.php | 2 +- 7 files changed, 84 insertions(+), 63 deletions(-) diff --git a/AbstractAdapter.php b/AbstractAdapter.php index 8b60fc3..c4382c6 100644 --- a/AbstractAdapter.php +++ b/AbstractAdapter.php @@ -17,75 +17,49 @@ abstract class AbstractAdapter implements AdapterInterface /** * @var null|CacheHelperInterface */ - protected $cache; + private $cache; /** * @var ParameterBagInterface */ - protected $parameters; + private $parameters; /** * @var LoggerHelper */ - protected $log; + private $log; - public function __construct(LoggerInterface $logger) + public function __construct() { - $this->cache = null; $this->parameters = new ParameterBag(); - $this->log = new LoggerHelper($logger); } abstract protected function handleRequest(Request $request); - public function getParameters() - { - return $this->parameters; - } - - public function setCache(CacheHelperInterface $cache = null) - { - $this->cache = $cache; - } - - public function hasCache() - { - return null !== $this->cache; - } - - public function getCache() - { - return $this->cache; - } - public function sendRequest(Request $request) { if (!$this->isConnected() || $this->getHost() === null) { - throw new ConnectionException( - (new \ReflectionClass($this))->getShortName().' must be connected before sending data.' - ); + throw new ConnectionException($this->currentAdapter().' must be connected before sending data.'); } if (!$this->supportsMethod($request->getMethod())) { - throw new RequestException( - 'Method '.$request->getMethod().' is not supported by '.(new \ReflectionClass($this))->getShortName().'.' - ); + throw new RequestException('Method '.$request->getMethod().' is not supported by '.$this->currentAdapter().'.'); } - if ($this->hasCache() && $this->cache->getPool()->hasItem($request)) { + if ($this->hasCache() && $this->getCache()->getPool()->hasItem($request)) { return $this->getFromCache($request); } try { - $this->log->request($this->getHost(), $request, (new \ReflectionClass($this))->getShortName()); + $this->log->request($this, $request); $response = $this->handleRequest($request); - $this->log->response($this->getHost(), $response, $request); + $this->log->response($this, $response, $request); if ($this->hasCache()) { $this->saveToCache($request, $response); } } catch (WebServiceException $e) { - $this->log->requestFailure($this->getHost(), $request, (new \ReflectionClass($this))->getShortName(), $e->getMessage()); + $this->log->requestFailure($this, $request, $e->getMessage()); throw new WebServiceException('Unable to contact WebService : '.$e->getMessage()); } @@ -95,27 +69,57 @@ public function sendRequest(Request $request) private function getFromCache(Request $request) { - $this->log->cacheGet($this->getHost(), $request, (new \ReflectionClass($this->cache))->getShortName()); + $this->log->cacheGet($this->getHost(), $request, (new \ReflectionClass($this->getCache()))->getShortName()); - $response = $this->cache->getPool()->getItem($request); + $response = $this->getCache()->getPool()->getItem($request); - $this->log->response($this->getHost(), $response, $request); + $this->log->response($this, $response, $request); return $response; } private function saveToCache(Request $request, $response) { - $duration = $this->cache->getConfig()->get($request, CacheHelperInterface::DEFAULT_DURATION); + $duration = $this->getCache()->getConfig()->get($request, CacheHelperInterface::DEFAULT_DURATION); - $item = $this->cache->getPool() + $item = $this->getCache()->getPool() ->getItem($request) ->set($response) ->expiresAfter($duration) ; - $this->cache->getPool()->save($item); + $this->getCache()->getPool()->save($item); + + $this->log->cacheAdd($this, $request, (new \ReflectionClass($this->getCache()))->getShortName(), $duration); + } + + private function currentAdapter() + { + return (new \ReflectionClass($this))->getShortName(); + } + + public function getParameters() + { + return $this->parameters; + } + + public function setCache(CacheHelperInterface $cache = null) + { + $this->cache = $cache; + } - $this->log->cacheAdd($this->getHost(), $request, (new \ReflectionClass($this->cache))->getShortName(), $duration); + public function hasCache() + { + return null !== $this->cache; + } + + public function getCache() + { + return $this->cache; + } + + public function setLogger(LoggerInterface $logger) + { + $this->log = new LoggerHelper($logger); } } diff --git a/Adapter/LoggerHelper.php b/Adapter/LoggerHelper.php index a06d203..9d56dc5 100644 --- a/Adapter/LoggerHelper.php +++ b/Adapter/LoggerHelper.php @@ -2,6 +2,7 @@ namespace dLdL\WebService\Adapter; +use dLdL\WebService\AdapterInterface; use dLdL\WebService\Http\Request; use Psr\Log\LoggerInterface; @@ -14,56 +15,65 @@ public function __construct(LoggerInterface $logger) $this->logger = $logger; } - public function request($host, Request $request, $class) + public function request(AdapterInterface $adapter, Request $request) { $this->logger->info( - sprintf('Sending request to %s using adapter %s.', $host.'/'.$request->getUrl(), $class), + sprintf('Sending request to %s%s using %s.', + $adapter->getHost(), $request->getUrl(), $this->className($adapter) + ), $request->getParameters() ); } - public function response($host, $response, Request $request) + public function response(AdapterInterface $adapter, $response, Request $request) { $this->logger->debug( - sprintf('Response trace for request to %s.', $host.'/'.$request->getUrl()), + sprintf('Response trace for %s%s.', $adapter->getHost(), $request->getUrl()), [$response] ); } - public function cacheGet($host, Request $request, $class) + public function cacheGet($host, Request $request, $cacheClass) { $this->logger->info( - sprintf('Retrieving data for url %s from cache %s.', $host.'/'.$request->getUrl(), $class), + sprintf('Retrieving data for %s%s from cache %s.', $host, $request->getUrl(), $cacheClass), $request->getParameters() ); } - public function cacheAdd($host, Request $request, $class, $time) + public function cacheAdd($host, Request $request, $cacheClass, $time) { $this->logger->info( - sprintf('Adding data to cache %s for url %s (will expire in %s seconds).', $class, $host.'/'.$request->getUrl(), $time), + sprintf('Adding response for %s%s to cache %s (will expire in %s seconds).', + $host, $request->getUrl(), $cacheClass, $time + ), $request->getParameters() ); } - public function connectionFailure($host, Request $request, $class) + public function connectionFailure(AdapterInterface $adapter, Request $request) { $this->logger->error( - sprintf('Failed to connect to %s using the adapter %s.', $host.'/'.$request->getUrl(), $class), + sprintf('Failed to connect to %s%s using %s.', + $adapter->getHost(), $request->getUrl(), $this->className($adapter) + ), $request->getParameters() ); } - public function requestFailure($host, Request $request, $class, $exceptionMessage) + public function requestFailure(AdapterInterface $adapter, Request $request, $exceptionMessage) { $this->logger->error( sprintf( - 'Failed to send request to %s using the adapter %s. Exception message : %s', - $host.'/'.$request->getUrl(), - $class, - $exceptionMessage + 'Failed to send request to %s%s using %s. Exception message : %s', + $adapter->getHost(), $request->getUrl(), $this->className($adapter), $exceptionMessage ), $request->getParameters() ); } + + private function className(AdapterInterface $adapter) + { + return (new \ReflectionClass($adapter))->getShortName(); + } } diff --git a/Adapter/ParameterBagInterface.php b/Adapter/ParameterBagInterface.php index 590ee27..3b53b1b 100644 --- a/Adapter/ParameterBagInterface.php +++ b/Adapter/ParameterBagInterface.php @@ -31,8 +31,8 @@ public function get($name, $defaultValue = null); /** * Add a value to the bag. * - * @param string $name Key of the added value - * @param mixed $value Value added to the bag + * @param string $name Key of the added value + * @param mixed $value Value added to the bag */ public function add($name, $value); diff --git a/AdapterInterface.php b/AdapterInterface.php index 2f958a3..b05db61 100644 --- a/AdapterInterface.php +++ b/AdapterInterface.php @@ -28,6 +28,8 @@ public function connect($host); * @throws ConnectionException if adapter is not connected * * @see AdapterInterface::isConnected() to check if adapter is connected + * + * @return string The current hostname */ public function getHost(); diff --git a/ParserInterface.php b/ParserInterface.php index f54c3d8..17cd299 100644 --- a/ParserInterface.php +++ b/ParserInterface.php @@ -11,7 +11,8 @@ interface ParserInterface * This method can be used to parse a raw response from an adapter and convert * it to a business object. * - * @param mixed $response Response to parse. + * @param mixed $response Response to parse + * * @return mixed */ public function parse($response); diff --git a/Tests/AbstractAdapterTest.php b/Tests/AbstractAdapterTest.php index c2eca7d..b68a820 100644 --- a/Tests/AbstractAdapterTest.php +++ b/Tests/AbstractAdapterTest.php @@ -3,6 +3,7 @@ namespace dLdL\WebService\Tests; use dLdL\WebService\AbstractAdapter; +use dLdL\WebService\Adapter\LoggerHelper; use dLdL\WebService\Exception\ConnectionException; use dLdL\WebService\Exception\RequestException; use Psr\Log\LoggerInterface; @@ -11,7 +12,9 @@ class AbstractAdapterTest extends AbstractTestCase { protected function getTestedClass() { - return $this->getMockForAbstractClass(AbstractAdapter::class, [$this->createMock(LoggerInterface::class)]); + $abstractAdapter = $this->getMockForAbstractClass(AbstractAdapter::class); + + return $abstractAdapter; } public function testInitialization() @@ -63,6 +66,7 @@ public function testUnsupportedRequestMethod() public function testRequest() { $abstractAdapter = $this->getTestedClass(); + $abstractAdapter->setLogger($this->createMock(LoggerInterface::class)); $abstractAdapter->expects($this->exactly(3)) ->method('getHost') diff --git a/Tests/AbstractTestCase.php b/Tests/AbstractTestCase.php index 1b4ae15..6f7128c 100644 --- a/Tests/AbstractTestCase.php +++ b/Tests/AbstractTestCase.php @@ -16,5 +16,5 @@ protected function getFakeGetRequest() return new Request('/fake/get/url', 'GET', ['test' => true]); } - protected abstract function getTestedClass(); + abstract protected function getTestedClass(); } From 9c69cacdea39de374d40c9bbff40e65d67fb7bbb Mon Sep 17 00:00:00 2001 From: Quentin de Longraye Date: Sat, 29 Oct 2016 13:14:39 +0200 Subject: [PATCH 2/2] add tests for logger --- Tests/AbstractAdapterTest.php | 1 - Tests/AbstractTestCase.php | 2 - Tests/Adapter/LoggerHelperTest.php | 145 +++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 3 deletions(-) create mode 100644 Tests/Adapter/LoggerHelperTest.php diff --git a/Tests/AbstractAdapterTest.php b/Tests/AbstractAdapterTest.php index b68a820..d7d46bf 100644 --- a/Tests/AbstractAdapterTest.php +++ b/Tests/AbstractAdapterTest.php @@ -3,7 +3,6 @@ namespace dLdL\WebService\Tests; use dLdL\WebService\AbstractAdapter; -use dLdL\WebService\Adapter\LoggerHelper; use dLdL\WebService\Exception\ConnectionException; use dLdL\WebService\Exception\RequestException; use Psr\Log\LoggerInterface; diff --git a/Tests/AbstractTestCase.php b/Tests/AbstractTestCase.php index 6f7128c..1a2c28b 100644 --- a/Tests/AbstractTestCase.php +++ b/Tests/AbstractTestCase.php @@ -15,6 +15,4 @@ protected function getFakeGetRequest() { return new Request('/fake/get/url', 'GET', ['test' => true]); } - - abstract protected function getTestedClass(); } diff --git a/Tests/Adapter/LoggerHelperTest.php b/Tests/Adapter/LoggerHelperTest.php new file mode 100644 index 0000000..a42da9f --- /dev/null +++ b/Tests/Adapter/LoggerHelperTest.php @@ -0,0 +1,145 @@ +adapter = $this->getMockBuilder(AdapterInterface::class) + ->setMockClassName('RandomAdapter') + ->getMock() + ; + + $this->request = $this->createMock(Request::class); + $this->request->expects($this->once()) + ->method('getUrl') + ->willReturn('/test/url') + ; + + $this->logger = $this->createMock(LoggerInterface::class); + } + + protected function getTestedClass($logger) + { + return new LoggerHelper($logger); + } + + public function testRequest() + { + $this->request->expects($this->once()) + ->method('getParameters') + ->willReturn(['number' => 42]) + ; + + $this->logger->expects($this->once()) + ->method('info') + ->with('Sending request to http://example.com/test/url using RandomAdapter.', ['number' => 42]) + ; + + $this->adapter->expects($this->once()) + ->method('getHost') + ->willReturn('http://example.com') + ; + + $this->getTestedClass($this->logger)->request($this->adapter, $this->request); + } + + public function testResponse() + { + $this->logger->expects($this->once()) + ->method('debug') + ->with('Response trace for http://example.com/test/url.', [['response' => 'ok']]) + ; + + $this->adapter->expects($this->once()) + ->method('getHost') + ->willReturn('http://example.com') + ; + + $this->getTestedClass($this->logger)->response($this->adapter, ['response' => 'ok'], $this->request); + } + + public function testCacheGet() + { + $this->request->expects($this->once()) + ->method('getParameters') + ->willReturn(['number' => 42]) + ; + + $this->logger->expects($this->once()) + ->method('info') + ->with('Retrieving data for http://example.com/test/url from cache RandomCache.', ['number' => 42]) + ; + + $this->getTestedClass($this->logger)->cacheGet('http://example.com', $this->request, 'RandomCache'); + } + + public function testCacheAdd() + { + $this->request->expects($this->once()) + ->method('getParameters') + ->willReturn(['number' => 42]) + ; + + $this->logger->expects($this->once()) + ->method('info') + ->with('Adding response for http://example.com/test/url to cache RandomCache (will expire in 3600 seconds).', ['number' => 42]) + ; + + $this->getTestedClass($this->logger)->cacheAdd('http://example.com', $this->request, 'RandomCache', 3600); + } + + public function testConnectionFailure() + { + $this->request->expects($this->once()) + ->method('getParameters') + ->willReturn(['number' => 42]) + ; + + $this->logger->expects($this->once()) + ->method('error') + ->with('Failed to connect to http://example.com/test/url using RandomAdapter.', ['number' => 42]) + ; + + $this->adapter->expects($this->once()) + ->method('getHost') + ->willReturn('http://example.com') + ; + + $this->getTestedClass($this->logger)->connectionFailure($this->adapter, $this->request); + } + + public function testRequestFailure() + { + $this->request->expects($this->once()) + ->method('getParameters') + ->willReturn(['number' => 42]) + ; + + $this->logger->expects($this->once()) + ->method('error') + ->with('Failed to send request to http://example.com/test/url using RandomAdapter. Exception message : No Internet connection.', ['number' => 42]) + ; + + $this->adapter->expects($this->once()) + ->method('getHost') + ->willReturn('http://example.com') + ; + + $this->getTestedClass($this->logger)->requestFailure($this->adapter, $this->request, 'No Internet connection.'); + } +}