[BUGFIX] t3lib_lock: Clean up constructor and add unit tests
authorChristian Kuhn <lolli@schwarzbu.ch>
Sun, 21 Aug 2011 09:28:13 +0000 (11:28 +0200)
committerSusanne Moog <typo3@susannemoog.de>
Sun, 21 Aug 2011 15:48:13 +0000 (17:48 +0200)
t3lib_lock can be cleaned up a bit: The constructor currently returns values
which is impossible, one error case is turned into an exception.
The constructor is additionally covered with unit tests.

Change-Id: I86b2d7e2c1329cefd6a00952c9a024fd926d5abf
Resolves: #29132
Reviewed-on: http://review.typo3.org/4444
Reviewed-by: Philipp Gampe
Tested-by: Philipp Gampe
Reviewed-by: Susanne Moog
Tested-by: Susanne Moog
t3lib/class.t3lib_lock.php
tests/t3lib/class.t3lib_lockTest.php

index ae7a8f6..07a66d1 100644 (file)
  * @see        class.t3lib_tstemplate.php, class.tslib_fe.php
  */
 class t3lib_lock {
+
+       /**
+        * @var string Locking method: One of 'simple', 'flock', 'semaphore' or 'disable'
+        */
        protected $method;
-       protected $id; // Identifier used for this lock
-       protected $resource; // Resource used for this lock (can be a file or a semaphore resource)
+
+       /**
+        * @var mixed Identifier used for this lock
+        */
+       protected $id;
+
+       /**
+        * @var mixed Resource used for this lock (can be a file or a semaphore resource)
+        */
+       protected $resource;
+
+       /**
+        * @var resource File pointer if using flock method
+        */
        protected $filepointer;
+
+       /**
+        * @var boolean True if lock is acquired
+        */
        protected $isAcquired = FALSE;
 
-       protected $loops = 150; // Number of times a locked resource is tried to be acquired. This is only used by manual locks like the "simple" method.
-       protected $step = 200; // Milliseconds after lock acquire is retried. $loops * $step results in the maximum delay of a lock. Only used by manual locks like the "simple" method.
+       /**
+        * @var integer Number of times a locked resource is tried to be acquired. Only used in manual locks method "simple".
+        */
+       protected $loops = 150;
+
+       /**
+        * @var integer Milliseconds after lock acquire is retried. $loops * $step results in the maximum delay of a lock. Only used in manual lock method "simple".
+        */
+       protected $step = 200;
+
+       /**
+        * @var string Logging facility
+        */
        protected $syslogFacility = 'cms';
+
+       /**
+        * @var boolean True if locking should be logged
+        */
        protected $isLoggingEnabled = TRUE;
 
 
@@ -60,16 +95,15 @@ class t3lib_lock {
         * Constructor:
         * initializes locking, check input parameters and set variables accordingly.
         *
-        * @param       string          ID to identify this lock in the system
-        * @param       string          Define which locking method to use. Defaults to "simple".
-        * @param       integer         Number of times a locked resource is tried to be acquired. This is only used by manual locks like the "simple" method.
-        * @param       integer         Milliseconds after lock acquire is retried. $loops * $step results in the maximum delay of a lock. Only used by manual locks like the "simple" method.
-        * @return      boolean         Returns TRUE unless something went wrong
+        * @param string $id ID to identify this lock in the system
+        * @param string $method Define which locking method to use. Defaults to "simple".
+        * @param integer $loops Number of times a locked resource is tried to be acquired. Only used in manual locks method "simple".
+        * @param integer step Milliseconds after lock acquire is retried. $loops * $step results in the maximum delay of a lock. Only used in manual lock method "simple".
         */
-       public function __construct($id, $method = '', $loops = 0, $step = 0) {
+       public function __construct($id, $method = 'simple', $loops = 0, $step = 0) {
+                       // Force ID to be string
+               $id = (string) $id;
 
-                       // Input checks
-               $id = (string) $id; // Force ID to be string
                if (intval($loops)) {
                        $this->loops = intval($loops);
                }
@@ -77,14 +111,8 @@ class t3lib_lock {
                        $this->step = intval($step);
                }
 
-                       // Detect locking method
-               if (in_array($method, array('disable', 'simple', 'flock', 'semaphore'))) {
-                       $this->method = $method;
-               } else {
-                       throw new InvalidArgumentException('No such method "' . $method . '"', 1294586097);
-               }
+               $this->method = $method;
 
-               $success = FALSE;
                switch ($this->method) {
                        case 'simple':
                        case 'flock':
@@ -94,20 +122,24 @@ class t3lib_lock {
                                }
                                $this->id = md5($id);
                                $this->resource = $path . $this->id;
-                               $success = TRUE;
                        break;
                        case 'semaphore':
                                $this->id = abs(crc32($id));
-                               if (($this->resource = sem_get($this->id, 1)) == TRUE) {
-                                       $success = TRUE;
+                               if (($this->resource = sem_get($this->id, 1)) === FALSE) {
+                                       throw new Exception(
+                                               'Unable to get semaphore',
+                                               1313828196
+                                       );
                                }
                        break;
                        case 'disable':
-                               return FALSE;
                        break;
+                       default:
+                               throw new InvalidArgumentException(
+                                       'No such method "' . $method . '"',
+                                       1294586097
+                               );
                }
-
-               return $success;
        }
 
        /**
index 96f6d44..7305a97 100644 (file)
@@ -49,8 +49,138 @@ class t3lib_lockTest extends tx_phpunit_testcase {
        protected $backupGlobalsBlacklist = array('TYPO3_DB');
 
        ///////////////////////////////
+       // tests concerning __construct
+       ///////////////////////////////
+
+       /**
+        * @test
+        */
+       public function constructorUsesDefaultLockingMethodSimple() {
+               $instance = new t3lib_lock('999999999');
+               $this->assertSame('simple', $instance->getMethod());
+       }
+
+       /**
+        * @test
+        */
+       public function constructorSetsMethodToGivenParameter() {
+               $instance = new t3lib_lock('999999999', 'flock');
+               $this->assertSame('flock', $instance->getMethod());
+       }
+
+       /**
+        * @test
+        */
+       public function constructorDoesNotThrowExceptionIfUsingDisableMethod() {
+               $instance = new t3lib_lock('999999999', 'disable');
+       }
+
+       /**
+        * @test
+        * @expectedException InvalidArgumentException
+        */
+       public function constructorThrowsExceptionForNotExistingLockingMethod() {
+               $instance = new t3lib_lock('999999999', 'foo');
+       }
+
+       /**
+        * @test
+        */
+       public function constructorUsesDefaultValueForLoops() {
+               $instance = new t3lib_lock('999999999');
+               $instance->setEnableLogging(FALSE);
+               $t3libLockReflection = new ReflectionClass('t3lib_lock');
+               $t3libLockReflectionResourceProperty = $t3libLockReflection->getProperty('loops');
+               $t3libLockReflectionResourceProperty->setAccessible(TRUE);
+               $this->assertSame(150, $t3libLockReflectionResourceProperty->getValue($instance));
+       }
+
+       /**
+        * @test
+        */
+       public function constructorSetsLoopsToGivenNumberOfLoops() {
+               $instance = new t3lib_lock('999999999', 'simple', 10);
+               $instance->setEnableLogging(FALSE);
+               $t3libLockReflection = new ReflectionClass('t3lib_lock');
+               $t3libLockReflectionResourceProperty = $t3libLockReflection->getProperty('loops');
+               $t3libLockReflectionResourceProperty->setAccessible(TRUE);
+               $this->assertSame(10, $t3libLockReflectionResourceProperty->getValue($instance));
+       }
+
+       /**
+        * @test
+        */
+       public function constructorUsesDefaultValueForSteps() {
+               $instance = new t3lib_lock('999999999');
+               $instance->setEnableLogging(FALSE);
+               $t3libLockReflection = new ReflectionClass('t3lib_lock');
+               $t3libLockReflectionResourceProperty = $t3libLockReflection->getProperty('step');
+               $t3libLockReflectionResourceProperty->setAccessible(TRUE);
+               $this->assertSame(200, $t3libLockReflectionResourceProperty->getValue($instance));
+       }
+
+       /**
+        * @test
+        */
+       public function constructorSetsStepToGivenNumberOfStep() {
+               $instance = new t3lib_lock('999999999', 'simple', 0, 10);
+               $instance->setEnableLogging(FALSE);
+               $t3libLockReflection = new ReflectionClass('t3lib_lock');
+               $t3libLockReflectionResourceProperty = $t3libLockReflection->getProperty('step');
+               $t3libLockReflectionResourceProperty->setAccessible(TRUE);
+               $this->assertSame(10, $t3libLockReflectionResourceProperty->getValue($instance));
+       }
+
+       /**
+        * @test
+        */
+       public function constructorCreatesLockDirectoryIfNotExisting() {
+               t3lib_div::rmdir(PATH_site . 'typo3temp/locks/', TRUE);
+               $instance = new t3lib_lock('999999999', 'simple');
+               $this->assertTrue(is_dir(PATH_site . 'typo3temp/locks/'));
+       }
+
+       /**
+        * @test
+        */
+       public function constructorSetsIdToMd5OfStringIfUsingSimleLocking() {
+               $instance = new t3lib_lock('999999999', 'simple');
+               $this->assertSame(md5('999999999'), $instance->getId());
+       }
+
+       /**
+        * @test
+        */
+       public function constructorSetsResourceToPathWithIdIfUsingSimpleLocking() {
+               $instance = new t3lib_lock('999999999', 'simple');
+               $this->assertSame(PATH_site . 'typo3temp/locks/' . md5('999999999'), $instance->getResource());
+       }
+
+       /**
+        * @test
+        */
+       public function constructorSetsIdToAbsCrc32OfIdStringIfUsingSemaphoreLocking() {
+               if (!function_exists('sem_get')) {
+                       $this->markTestSkipped('The system does not support semaphore base locking.');
+               }
+               $instance = new t3lib_lock('999999999', 'semaphore');
+               $this->assertSame(abs(crc32('999999999')), $instance->getId());
+       }
+
+       /**
+        * @test
+        */
+       public function constructorSetsResourceToSemaphoreResourceIfUsingSemaphoreLocking() {
+               if (!function_exists('sem_get')) {
+                       $this->markTestSkipped('The system does not support semaphore base locking.');
+               }
+               $instance = new t3lib_lock('999999999', 'semaphore');
+               $this->assertTrue(is_resource($instance->getResource()));
+       }
+
+       ///////////////////////////////
        // tests concerning acquire
-       ///////////////////////////////a
+       ///////////////////////////////
 
        /**
         * @test
@@ -74,6 +204,7 @@ class t3lib_lockTest extends tx_phpunit_testcase {
                $this->assertEquals($resultFilePermissions, '0777');
        }
 
+
        ///////////////////////////////
        // tests concerning release
        ///////////////////////////////