Fix issue found by Scrutinizer

Author: mathroc

src/FileLock.php

@@ -20,6 +20,14 @@ class FileLock implements Lock
     private $fh;
     private $remove_on_release;
 
+    private $logger;
+
+    /**
+     * @param string       $lock_file         path to file
+     * @param string|null  $identifier        resource identifier (default to $lock_file) for logging
+     * @param string|null  $owner             owner name for logging
+     * @param boolean      $remove_on_release remove file on release if no other lock remains
+     */
     public function __construct($lock_file, $identifier = null, $owner = null, $remove_on_release = false)
     {
         $this->lock_file         = $lock_file;
@@ -30,33 +38,53 @@ public function __construct($lock_file, $identifier = null, $owner = null, $remo
         $this->logger = new NullLogger;
     }
 
+    /**
+     * @param  boolean $exclusive true for an exclusive lock, false for shared one
+     * @param  boolean $blocking  true to wait for lock to be available, false to throw exception instead of waiting
+     * @inherit
+     */
     public function acquire($exclusive = FileLock::EXCLUSIVE, $blocking = FileLock::NON_BLOCKING)
     {
-        $operation  = ($exclusive ? LOCK_EX : LOCK_SH) | ($blocking ? 0 : LOCK_NB);
-
-        if (!flock($this->fh(), $operation)) {
-            $this->logger->debug(($exclusive ?
-                '{owner} could not acquire exclusive lock on {identifier}':
-                '{owner} could not acquire shared lock on {identifier}'
-            ), [
-                'identifier' => $this->identifier,
-                'owner' => $this->owner
-            ]);
-
-            throw new Exception(($exclusive ?
-                'Could not acquire exclusive lock on '.$this->identifier:
-                'Could not acquire shared lock on '.$this->identifier
-            ));
+        if ($exclusive === FileLock::EXCLUSIVE) {
+            $lock_type = 'exclusive';
+            $operation = LOCK_EX;
+        } else {
+            $lock_type = 'shared';
+            $operation = LOCK_SH;
+        }
 
+        if ($blocking === FileLock::NON_BLOCKING) {
+            $operation |= LOCK_NB;
         }
 
-        $this->logger->debug(($exclusive ?
-            '{owner} exclusive lock acquired on {identifier}':
-            '{owner} shared lock acquired on {identifier}'
-        ), [
+        $this->tryAcquire($operation, $lock_type);
+    }
+
+    /**
+     * try to acquire lock on file, throw in case of faillure
+     * @param  int    $operation
+     * @param  string $lock_type lock type description
+     * @return void
+     * @see https://php.net/flock
+     */
+    private function tryAcquire($operation, $lock_type)
+    {
+        $log_data = [
             'identifier' => $this->identifier,
-            'owner' => $this->owner
-        ]);
+            'owner' => $this->owner,
+            'lock_type' => $lock_type
+        ];
+
+        if (!$this->flock($operation)) {
+            $this->logger->debug('{owner} could not acquire {lock_type} lock on {identifier}', $log_data);
+
+            throw new Exception(
+                'Could not acquire '.$lock_type.' lock on '.$this->identifier
+            );
+
+        }
+
+        $this->logger->debug('{owner} {lock_type} lock acquired on {identifier}', $log_data);
     }
 
     public function release()
@@ -65,22 +93,18 @@ public function release()
             return;
         }
 
-        if($this->remove_on_release && flock($this->fh, LOCK_EX)) {
-            fclose($this->fh);
-            $this->fh = null;
+        if($this->remove_on_release && $this->flock(LOCK_EX)) {
             unlink($this->lock_file);
-        } else {
-            flock($this->fh, LOCK_UN);
         }
 
+        $this->flock(LOCK_UN);
+        fclose($this->fh);
+        $this->fh = null;
 
-        $this->logger->debug(
-            '{owner} lock released on {identifier}',
-            [
-                'identifier' => $this->identifier,
-                'owner' => $this->owner
-            ]
-        );
+        $this->logger->debug('{owner} lock released on {identifier}', [
+            'identifier' => $this->identifier,
+            'owner' => $this->owner
+        ]);
     }
 
     public function setLogger(LoggerInterface $logger)
@@ -93,12 +117,19 @@ public function __destruct()
         $this->release();
     }
 
-    private function fh()
+    /**
+     * @return boolean
+     */
+    private function flock($operation)
     {
         if ($this->fh === null) {
             $this->fh = fopen($this->lock_file, 'c');
         }
 
-        return $this->fh;
+        if (!is_resource($this->fh)) {
+            throw new Exception('Could not open lock file '.$this->lock_file);
+        }
+
+        return flock($this->fh, $operation);
     }
 }

src/Lock.php

@@ -6,7 +6,22 @@
 
 interface Lock
 {
+    /**
+     * Acquire a lock on the resource
+     * @return void
+     */
     public function acquire();
+
+    /**
+     * Release lock on the resource
+     * @return void
+     */
     public function release();
+
+    /**
+     * Replace lock logger
+     * @param LoggerInterface $logger
+     * @return void
+     */
     public function setLogger(LoggerInterface $logger);
 }

src/LockFactory.php

@@ -6,6 +6,17 @@
 
 interface LockFactory
 {
+    /**
+     * Create a lock handle on $resource
+     * @param  string      $resource resource identifier
+     * @param  string|null $owner    owner name (for logging)
+     * @return Lock
+     */
     public function create($resource, $owner = null);
+
+    /**
+     * @param LoggerInterface $logger
+     * @return void
+     */
     public function setLogger(LoggerInterface $logger);
 }