SocketHandler: Coding standard fixes. Remove redundant test.

src/Monolog/Handler/SocketHandler.php

@@ -17,7 +17,7 @@ use Monolog\Logger;
  * Stores to any socket - uses fsockopen() or pfsockopen().
  * 
  * @author Pablo de Leon Belloc <pablolb@gmail.com>
- * @see http://php.net/manual/en/function.fsockopen.php
+ * @see    http://php.net/manual/en/function.fsockopen.php
  */
 class SocketHandler extends AbstractProcessingHandler
 {
@@ -31,9 +31,9 @@ class SocketHandler extends AbstractProcessingHandler
     private $errstr;
 
     /**
-     * @param string $connectionString
+     * @param string  $connectionString Socket connection string
-     * @param integer $level The minimum logging level at which this handler will be triggered
+     * @param integer $level            The minimum logging level at which this handler will be triggered
-     * @param Boolean $bubble Whether the messages that are handled can bubble up the stack or not
+     * @param Boolean $bubble           Whether the messages that are handled can bubble up the stack or not
      */
     public function __construct($connectionString, $level = Logger::DEBUG, $bubble = true)
     {
@@ -45,9 +45,10 @@ class SocketHandler extends AbstractProcessingHandler
     /**
      * Connect (if necessary) and write to the socket
      * 
+     * @param array $record
+     * 
      * @throws \UnexpectedValueException
      * @throws \RuntimeException
-     * @param string $string
      */
     public function write(array $record)
     {
@@ -66,6 +67,9 @@ class SocketHandler extends AbstractProcessingHandler
         $this->closeSocket();
     }
 
+    /**
+     * Close socket, if open
+     */
     public function closeSocket()
     {
         if (is_resource($this->resource)) {
@@ -74,6 +78,11 @@ class SocketHandler extends AbstractProcessingHandler
         }
     }
 
+    /**
+     * Set socket connection to nbe persistent. It only has effect before the connection is initiated.
+     * 
+     * @param type $boolean 
+     */
     public function setPersistent($boolean)
     {
         $this->persistent = (boolean) $boolean;
@@ -82,8 +91,9 @@ class SocketHandler extends AbstractProcessingHandler
     /**
      * Set connection timeout.  Only has effect before we connect.
      * 
-     * @see http://php.net/manual/en/function.fsockopen.php
      * @param integer $seconds 
+     * 
+     * @see http://php.net/manual/en/function.fsockopen.php
      */
     public function setConnectionTimeout($seconds)
     {
@@ -94,8 +104,9 @@ class SocketHandler extends AbstractProcessingHandler
     /**
      * Set write timeout. Only has effect before we connect.
      * 
-     * @see http://php.net/manual/en/function.stream-set-timeout.php
      * @param type $seconds 
+     * 
+     * @see http://php.net/manual/en/function.stream-set-timeout.php
      */
     public function setTimeout($seconds)
     {
@@ -103,44 +114,46 @@ class SocketHandler extends AbstractProcessingHandler
         $this->timeout = (int) $seconds;
     }
 
-    private function validateTimeout($value)
+    /**
-    {
+     * Get current connection string
-        $ok = filter_var($value, FILTER_VALIDATE_INT, array('options' => array(
+     * 
-                'min_range' => 0,
+     * @return string
-                )));
+     */
-        if ($ok === false) {
-            throw new \InvalidArgumentException("Timeout must be 0 or a positive integer (got $value)");
-        }
-    }
-
     public function getConnectionString()
     {
         return $this->connectionString;
     }
 
+    /**
+     * Get persistent setting
+     * 
+     * @return boolean
+     */
     public function isPersistent()
     {
         return $this->persistent;
     }
 
+    /**
+     * Get current connection timeout setting
+     * 
+     * @return float
+     */
     public function getConnectionTimeout()
     {
         return $this->connectionTimeout;
     }
 
+    /**
+     * Get current in-transfer timeout
+     * 
+     * @return float
+     */
     public function getTimeout()
     {
         return $this->timeout;
     }
 
-    private function connectIfNotConnected()
-    {
-        if ($this->isConnected()) {
-            return;
-        }
-        $this->connect();
-    }
-
     /**
      * Check to see if the socket is currently available.
      * 
@@ -151,55 +164,9 @@ class SocketHandler extends AbstractProcessingHandler
     public function isConnected()
     {
         return is_resource($this->resource)
-                && !feof($this->resource);  // on TCP - other party can close connection.
+                && !feof($this->resource);  // on TCP - other party can close connection. 
     }
-
+    
-    private function connect()
-    {
-        $this->createSocketResource();
-        $this->setSocketTimeout();
-    }
-
-    private function createSocketResource()
-    {
-        if ($this->isPersistent()) {
-            $resource = $this->pfsockopen();
-        } else {
-            $resource = $this->fsockopen();
-        }
-        if (!$resource) {
-            throw new \UnexpectedValueException("Failed connecting to $this->connectionString ($this->errno: $this->errstr)");
-        }
-        return $this->resource = $resource;
-    }
-
-    private function setSocketTimeout()
-    {
-        if (!$this->stream_set_timeout()) {
-            throw new \UnexpectedValueException("Failed setting timeout with stream_set_timeout()");
-        }
-    }
-
-    private function writeToSocket($data)
-    {
-        $length = strlen($data);
-        $sent = 0;
-        while ($this->isConnected() && $sent < $length) {
-            $chunk = $this->fwrite(substr($data, $sent));
-            if ($chunk === false) {
-                throw new \RuntimeException("Could not write to socket");
-            }
-            $sent += $chunk;
-            $socketInfo = $this->stream_get_meta_data();
-            if ($socketInfo['timed_out']) {
-                throw new \RuntimeException("Write timed-out");
-            }
-        }
-        if (!$this->isConnected() && $sent < $length) {
-            throw new \RuntimeException("End-of-file reached, probably we got disconnected (sent $sent of $length)");
-        }
-    }
-
     /**
      * Allow mock
      */
@@ -240,4 +207,68 @@ class SocketHandler extends AbstractProcessingHandler
         return stream_get_meta_data($this->resource);
     }
 
+    private function validateTimeout($value)
+    {
+        $ok = filter_var($value, FILTER_VALIDATE_INT, array('options' => array(
+                'min_range' => 0,
+                )));
+        if ($ok === false) {
+            throw new \InvalidArgumentException("Timeout must be 0 or a positive integer (got $value)");
+        }
+    }
+
+    private function connectIfNotConnected()
+    {
+        if ($this->isConnected()) {
+            return;
+        }
+        $this->connect();
+    }
+
+    private function connect()
+    {
+        $this->createSocketResource();
+        $this->setSocketTimeout();
+    }
+
+    private function createSocketResource()
+    {
+        if ($this->isPersistent()) {
+            $resource = $this->pfsockopen();
+        } else {
+            $resource = $this->fsockopen();
+        }
+        if (!$resource) {
+            throw new \UnexpectedValueException("Failed connecting to $this->connectionString ($this->errno: $this->errstr)");
+        }
+        $this->resource = $resource;
+    }
+
+    private function setSocketTimeout()
+    {
+        if (!$this->stream_set_timeout()) {
+            throw new \UnexpectedValueException("Failed setting timeout with stream_set_timeout()");
+        }
+    }
+
+    private function writeToSocket($data)
+    {
+        $length = strlen($data);
+        $sent = 0;
+        while ($this->isConnected() && $sent < $length) {
+            $chunk = $this->fwrite(substr($data, $sent));
+            if ($chunk === false) {
+                throw new \RuntimeException("Could not write to socket");
+            }
+            $sent += $chunk;
+            $socketInfo = $this->stream_get_meta_data();
+            if ($socketInfo['timed_out']) {
+                throw new \RuntimeException("Write timed-out");
+            }
+        }
+        if (!$this->isConnected() && $sent < $length) {
+            throw new \RuntimeException("End-of-file reached, probably we got disconnected (sent $sent of $length)");
+        }
+    }
+
 }

tests/Monolog/Handler/SocketHandlerTest.php

@@ -77,18 +77,6 @@ class SocketHandlerTest extends TestCase
         $this->assertEquals('tcp://localhost:9090', $this->handler->getConnectionString());
     }
 
-    public function testConnectionRefuesed()
-    {
-        try {
-            $this->createHandler('127.0.0.1:7894');
-            $string = 'Hello world';
-            $this->writeRecord($string);
-            $this->fail("Shoul not connect - are you running a server on 127.0.0.1:7894 ?");
-        } catch (\UnexpectedValueException $e) {
-            
-        }
-    }
-
     /**
      * @expectedException UnexpectedValueException
      */
@@ -96,8 +84,8 @@ class SocketHandlerTest extends TestCase
     {
         $this->setMockHandler(array('fsockopen'));
         $this->handler->expects($this->once())
-                ->method('fsockopen')
+            ->method('fsockopen')
-                ->will($this->returnValue(false));
+            ->will($this->returnValue(false));
         $this->writeRecord('Hello world');
     }
 
@@ -108,8 +96,8 @@ class SocketHandlerTest extends TestCase
     {
         $this->setMockHandler(array('pfsockopen'));
         $this->handler->expects($this->once())
-                ->method('pfsockopen')
+            ->method('pfsockopen')
-                ->will($this->returnValue(false));
+            ->will($this->returnValue(false));
         $this->handler->setPersistent(true);
         $this->writeRecord('Hello world');
     }
@@ -121,8 +109,8 @@ class SocketHandlerTest extends TestCase
     {
         $this->setMockHandler(array('stream_set_timeout'));
         $this->handler->expects($this->once())
-                ->method('stream_set_timeout')
+            ->method('stream_set_timeout')
-                ->will($this->returnValue(false));
+            ->will($this->returnValue(false));
         $this->writeRecord('Hello world');
     }
 
@@ -132,18 +120,18 @@ class SocketHandlerTest extends TestCase
     public function testWriteFailsOnIfFwriteReturnsFalse()
     {
         $this->setMockHandler(array('fwrite'));
-        
+
         $callback = function($arg) {
-            $map = array(
+                $map = array(
-                'Hello world' =>  6,
+                    'Hello world' => 6,
-                'world' => false,
+                    'world' => false,
-            );
+                );
-            return $map[$arg];
+                return $map[$arg];
-        };
+            };
 
         $this->handler->expects($this->exactly(2))
-                ->method('fwrite')
+            ->method('fwrite')
-                ->will($this->returnCallback($callback));
+            ->will($this->returnCallback($callback));
 
         $this->writeRecord('Hello world');
     }
@@ -154,21 +142,21 @@ class SocketHandlerTest extends TestCase
     public function testWriteFailsIfStreamTimesOut()
     {
         $this->setMockHandler(array('fwrite', 'stream_get_meta_data'));
-        
+
         $callback = function($arg) {
-            $map = array(
+                $map = array(
-                'Hello world' => 6,
+                    'Hello world' => 6,
-                'world' => 5,
+                    'world' => 5,
-            );
+                );
-            return $map[$arg];
+                return $map[$arg];
-        };
+            };
 
         $this->handler->expects($this->exactly(1))
-                ->method('fwrite')
+            ->method('fwrite')
-                ->will($this->returnCallback($callback));
+            ->will($this->returnCallback($callback));
         $this->handler->expects($this->exactly(1))
-                ->method('stream_get_meta_data')
+            ->method('stream_get_meta_data')
-                ->will($this->returnValue(array('timed_out' => true)));
+            ->will($this->returnValue(array('timed_out' => true)));
 
 
         $this->writeRecord('Hello world');
@@ -183,16 +171,16 @@ class SocketHandlerTest extends TestCase
 
         $res = $this->res;
         $callback = function($string) use ($res) {
-                    fclose($res);
+                fclose($res);
-                    return strlen('Hello');
+                return strlen('Hello');
-                };
+            };
 
         $this->handler->expects($this->exactly(1))
-                ->method('fwrite')
+            ->method('fwrite')
-                ->will($this->returnCallback($callback));
+            ->will($this->returnCallback($callback));
         $this->handler->expects($this->exactly(1))
-                ->method('stream_get_meta_data')
+            ->method('stream_get_meta_data')
-                ->will($this->returnValue(array('timed_out' => false)));
+            ->will($this->returnValue(array('timed_out' => false)));
 
         $this->writeRecord('Hello world');
     }
@@ -210,18 +198,18 @@ class SocketHandlerTest extends TestCase
     public function testWriteWithMock()
     {
         $this->setMockHandler(array('fwrite'));
-        
+
         $callback = function($arg) {
-            $map = array(
+                $map = array(
-                'Hello world' => 6,
+                    'Hello world' => 6,
-                'world' => 5,
+                    'world' => 5,
-            );
+                );
-            return $map[$arg];
+                return $map[$arg];
-        };
+            };
 
         $this->handler->expects($this->exactly(2))
-                ->method('fwrite')
+            ->method('fwrite')
-                ->will($this->returnCallback($callback));
+            ->will($this->returnCallback($callback));
 
         $this->writeRecord('Hello world');
     }
@@ -230,9 +218,9 @@ class SocketHandlerTest extends TestCase
     {
         $this->setMockHandler();
         $this->writeRecord('Hello world');
-        $this->assertTrue(is_resource($this->res));
+        $this->assertInternalType('resource', $this->res);
         $this->handler->close();
-        $this->assertFalse(is_resource($this->res));
+        $this->assertFalse(is_resource($this->res), "Expected resource to be closed after closing handler");
     }
 
     public function testCloseDoesNotClosePersistentSocket()
@@ -266,28 +254,28 @@ class SocketHandlerTest extends TestCase
         $finalMethods = array_merge($defaultMethods, $newMethods);
 
         $this->handler = $this->getMock(
-                '\Monolog\Handler\SocketHandler', $finalMethods, array('localhost:1234')
+            '\Monolog\Handler\SocketHandler', $finalMethods, array('localhost:1234')
         );
 
         if (!in_array('fsockopen', $methods)) {
             $this->handler->expects($this->any())
-                    ->method('fsockopen')
+                ->method('fsockopen')
-                    ->will($this->returnValue($this->res));
+                ->will($this->returnValue($this->res));
         }
 
         if (!in_array('pfsockopen', $methods)) {
             $this->handler->expects($this->any())
-                    ->method('pfsockopen')
+                ->method('pfsockopen')
-                    ->will($this->returnValue($this->res));
+                ->will($this->returnValue($this->res));
         }
 
         if (!in_array('stream_set_timeout', $methods)) {
             $this->handler->expects($this->any())
-                    ->method('stream_set_timeout')
+                ->method('stream_set_timeout')
-                    ->will($this->returnValue(true));
+                ->will($this->returnValue(true));
         }
 
         $this->handler->setFormatter($this->getIdentityFormatter());
     }
-    
+
 }