From 90b034e0cb63421834a5e50c9d26f8be81be46f6 Mon Sep 17 00:00:00 2001 From: Marcos Sader Date: Wed, 7 Jan 2015 04:15:51 -0300 Subject: [PATCH 1/5] Improve send() method --- src/Client.php | 74 ++++++++++++++++++++++++++++++--------- tests/unit/ClientTest.php | 68 +++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 16 deletions(-) diff --git a/src/Client.php b/src/Client.php index 6411580..6c808cc 100644 --- a/src/Client.php +++ b/src/Client.php @@ -1,17 +1,20 @@ stream = $stream; + $this->throwExceptions(false); + } + + /** + * Controls whether failed calls to Carbon will throw an Exception. + * + * @see send() + * + * @param boolean $throw + * + * @return self + */ + public function throwExceptions($throw = true) + { + $this->throw_exceptions = (bool) $throw; + + return $this; } /** @@ -55,28 +75,50 @@ public function getNamespace() * @param int|float $value Metric Value * @param int|null $timestamp Metric Timestamp * + * @throws ErrorException If $this->throw_exceptions is true * @return bool */ public function send($path, $value, $timestamp = null) { - if (!is_resource($this->stream) - || !is_string($path) - || empty($path) - || !is_numeric($value) - ) { - return false; - } + $result = false; + $exception = null; - $value = (float) $value; - $timestamp = is_numeric($timestamp) ? (int) $timestamp : time(); - $full_path = $this->sanitizePath( - sprintf('%s.%s', $this->getNamespace(), $path) - ); + set_error_handler(function ($code, $message, $file = null, $line = 0) { + throw new ErrorException($message, $code, null, $file, $line); + }); + + try { + if (!is_string($path) || empty($path)) { + throw new InvalidArgumentException('$path must be a non-empty string'); + } + + if (!is_numeric($value)) { + throw new InvalidArgumentException( + sprintf('$value must be of type int|float, %s given.', gettype($value)) + ); + } - $data = sprintf("%s %f %d\n", $full_path, $value, $timestamp); - $sent = fwrite($this->stream, $data); + $value = (float) $value; + $timestamp = is_numeric($timestamp) ? (int) $timestamp : time(); + $full_path = $this->sanitizePath( + sprintf('%s.%s', $this->getNamespace(), $path) + ); + + $data = sprintf("%s %f %d\n", $full_path, $value, $timestamp); + $sent = fwrite($this->stream, $data); + $result = is_int($sent) && $sent === strlen($data); + } catch (Exception $e) { + if ($this->throw_exceptions) { + $exception = $e; + } + } + restore_error_handler(); + + if (!empty($exception)) { + throw $exception; + } - return is_int($sent) && $sent === strlen($data); + return $result; } /** diff --git a/tests/unit/ClientTest.php b/tests/unit/ClientTest.php index 7133c83..18ec0b9 100644 --- a/tests/unit/ClientTest.php +++ b/tests/unit/ClientTest.php @@ -1,6 +1,8 @@ assertEquals($expected, $carbon->getNamespace()); } + public function testThrowExceptionsSetting() + { + $carbon = new Client($this->stream); + + $throw_exceptions = (new ReflectionClass($carbon))->getProperty('throw_exceptions'); + $throw_exceptions->setAccessible(true); + $this->assertFalse($throw_exceptions->getValue($carbon)); + + $carbon->throwExceptions(true); + $this->assertTrue($throw_exceptions->getValue($carbon)); + } + public function providerNamespaces() { return [ @@ -89,6 +103,60 @@ public function testSend( } } + /** + * @expectedException InvalidArgumentException + */ + public function testSendErrorOnInvalidPath() + { + $carbon = new Client($this->stream); + + $sent = $carbon->send(['x'], 1); + $this->assertFalse($sent); + + $carbon->throwExceptions(true); + $carbon->send(['x'], 1); + } + + /** + * @expectedException InvalidArgumentException + */ + public function testSendErrorOnInvalidValue() + { + $carbon = new Client($this->stream); + + $sent = $carbon->send('x', 'string'); + $this->assertFalse($sent); + + $carbon->throwExceptions(true); + $carbon->send('x', []); + } + + /** + * @expectedException ErrorException + */ + public function testSendErrorOnClosedStream() + { + $carbon = new Client($this->stream); + + // simulate network failure + fclose($this->stream); + + $sent = $carbon->send('metric', 1); + $this->assertFalse($sent); + + $carbon->throwExceptions(true); + $sent = $carbon->send('metric', 1); + } + + public function testSendErrorOnReadOnlyStream() + { + $read_only_stream = fopen('php://memory', 'x'); + $carbon = new Client($read_only_stream); + + $sent = $carbon->send('metric', 1); + $this->assertFalse($sent); + } + public function providerMetrics() { return [ From f385dc330ac11695c66eb0927d7298106eb97e95 Mon Sep 17 00:00:00 2001 From: Marcos Sader Date: Wed, 7 Jan 2015 04:30:10 -0300 Subject: [PATCH 2/5] improve readability --- src/Client.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Client.php b/src/Client.php index 6c808cc..fd6f885 100644 --- a/src/Client.php +++ b/src/Client.php @@ -108,13 +108,11 @@ public function send($path, $value, $timestamp = null) $sent = fwrite($this->stream, $data); $result = is_int($sent) && $sent === strlen($data); } catch (Exception $e) { - if ($this->throw_exceptions) { - $exception = $e; - } + $exception = $e; } restore_error_handler(); - if (!empty($exception)) { + if (!empty($exception) && $this->throw_exceptions) { throw $exception; } From 3b97f484309c8ac18571ef58292f42099e67629b Mon Sep 17 00:00:00 2001 From: Marcos Sader Date: Wed, 7 Jan 2015 04:31:46 -0300 Subject: [PATCH 3/5] use r flag to open read only stream because hhvm allows writes otherwise --- tests/unit/ClientTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/ClientTest.php b/tests/unit/ClientTest.php index 18ec0b9..7482789 100644 --- a/tests/unit/ClientTest.php +++ b/tests/unit/ClientTest.php @@ -150,7 +150,7 @@ public function testSendErrorOnClosedStream() public function testSendErrorOnReadOnlyStream() { - $read_only_stream = fopen('php://memory', 'x'); + $read_only_stream = fopen('php://memory', 'r'); $carbon = new Client($read_only_stream); $sent = $carbon->send('metric', 1); From e3ac3b8e7555b8c5e5b558483f91cca3c1ff0dc2 Mon Sep 17 00:00:00 2001 From: Marcos Sader Date: Wed, 7 Jan 2015 04:36:58 -0300 Subject: [PATCH 4/5] use php://input instead of php://memory to fake a read-only stream because hhvm allows writes to memory always --- tests/unit/ClientTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/ClientTest.php b/tests/unit/ClientTest.php index 7482789..8409b39 100644 --- a/tests/unit/ClientTest.php +++ b/tests/unit/ClientTest.php @@ -150,7 +150,7 @@ public function testSendErrorOnClosedStream() public function testSendErrorOnReadOnlyStream() { - $read_only_stream = fopen('php://memory', 'r'); + $read_only_stream = fopen('php://input', 'r'); $carbon = new Client($read_only_stream); $sent = $carbon->send('metric', 1); From a5438254d5c454726b654ef8d47c0cb821892621 Mon Sep 17 00:00:00 2001 From: Marcos Sader Date: Wed, 7 Jan 2015 04:47:19 -0300 Subject: [PATCH 5/5] skip minor test that is broken in HHVM --- tests/unit/ClientTest.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/unit/ClientTest.php b/tests/unit/ClientTest.php index 8409b39..4b72428 100644 --- a/tests/unit/ClientTest.php +++ b/tests/unit/ClientTest.php @@ -150,7 +150,13 @@ public function testSendErrorOnClosedStream() public function testSendErrorOnReadOnlyStream() { - $read_only_stream = fopen('php://input', 'r'); + if (defined('HHVM_VERSION')) { + $this->markTestSkipped( + "Skipping because HHVM won't open a php://memory stream as read-only" + ); + } + + $read_only_stream = fopen('php://memory', 'r'); $carbon = new Client($read_only_stream); $sent = $carbon->send('metric', 1);