[THRIFT-5757] Unit tests for php lib

sveneld · Mar 9, 2024 · 8a07e61 · 8a07e61
1 parent da2ef34
commit 8a07e61
Show file tree

Hide file tree

Showing 36 changed files with 3,366 additions and 144 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -90,7 +90,7 @@ jobs:
         uses: shivammathur/setup-php@v2
         with:
           php-version: ${{ matrix.php-version }}
-          extensions: mbstring, intl, xml
+          extensions: mbstring, intl, xml, curl
           ini-values: "error_reporting=E_ALL"
 
       - name: Install Dependencies

diff --git a/composer.json b/composer.json
@@ -23,8 +23,10 @@
     "require-dev": {
         "phpunit/phpunit": "^7.5 || ^8.5 || ^9.5",
         "squizlabs/php_codesniffer": "3.*",
+        "php-mock/php-mock-phpunit": "^2.10",
         "ext-json": "*",
-        "ext-xml": "*"
+        "ext-xml": "*",
+        "ext-curl": "*"
     },
     "autoload": {
         "psr-4": {"Thrift\\": "lib/php/lib/"}

diff --git a/lib/php/lib/Transport/TBufferedTransport.php b/lib/php/lib/Transport/TBufferedTransport.php
@@ -1,4 +1,5 @@
 <?php
+
 /*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements. See the NOTICE file

diff --git a/lib/php/lib/Transport/TCurlClient.php b/lib/php/lib/Transport/TCurlClient.php
@@ -1,4 +1,5 @@
 <?php
+
 /*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements. See the NOTICE file
@@ -227,7 +228,6 @@ public function flush()
             register_shutdown_function(array('Thrift\\Transport\\TCurlClient', 'closeCurlHandle'));
             self::$curlHandle = curl_init();
             curl_setopt(self::$curlHandle, CURLOPT_RETURNTRANSFER, true);
-            curl_setopt(self::$curlHandle, CURLOPT_BINARYTRANSFER, true);
             curl_setopt(self::$curlHandle, CURLOPT_USERAGENT, 'PHP/TCurlClient');
             curl_setopt(self::$curlHandle, CURLOPT_CUSTOMREQUEST, 'POST');
             curl_setopt(self::$curlHandle, CURLOPT_FOLLOWLOCATION, true);
@@ -238,9 +238,11 @@ public function flush()
         $fullUrl = $this->scheme_ . "://" . $host . $this->uri_;
 
         $headers = array();
-        $defaultHeaders = array('Accept' => 'application/x-thrift',
+        $defaultHeaders = array(
+            'Accept' => 'application/x-thrift',
             'Content-Type' => 'application/x-thrift',
-            'Content-Length' => TStringFuncFactory::create()->strlen($this->request_));
+            'Content-Length' => TStringFuncFactory::create()->strlen($this->request_)
+        );
         foreach (array_merge($defaultHeaders, $this->headers_) as $key => $value) {
             $headers[] = "$key: $value";
         }
@@ -292,10 +294,11 @@ public static function closeCurlHandle()
     {
         try {
             if (self::$curlHandle) {
-                curl_close(self::$curlHandle);
+                curl_close(self::$curlHandle); #This function has no effect. Prior to PHP 8.0.0, this function was used to close the resource.
                 self::$curlHandle = null;
             }
         } catch (\Exception $x) {
+            #it's not possible to throw an exception by calling a function that has no effect
             error_log('There was an error closing the curl handle: ' . $x->getMessage());
         }
     }

diff --git a/lib/php/lib/Transport/THttpClient.php b/lib/php/lib/Transport/THttpClient.php
@@ -1,4 +1,5 @@
 <?php
+
 /*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements. See the NOTICE file
@@ -212,11 +213,14 @@ public function flush()
         $host = $this->host_ . ($this->port_ != 80 ? ':' . $this->port_ : '');
 
         $headers = array();
-        $defaultHeaders = array('Host' => $host,
+        $defaultHeaders = array(
+            'Host' => $host,
             'Accept' => 'application/x-thrift',
             'User-Agent' => 'PHP/THttpClient',
             'Content-Type' => 'application/x-thrift',
-            'Content-Length' => TStringFuncFactory::create()->strlen($this->buf_));
+            'Content-Length' => TStringFuncFactory::create()->strlen($this->buf_)
+        );
+
         foreach (array_merge($defaultHeaders, $this->headers_) as $key => $value) {
             $headers[] = "$key: $value";
         }
@@ -225,10 +229,12 @@ public function flush()
 
         $baseHttpOptions = isset($options["http"]) ? $options["http"] : array();
 
-        $httpOptions = $baseHttpOptions + array('method' => 'POST',
+        $httpOptions = $baseHttpOptions + array(
+            'method' => 'POST',
             'header' => implode("\r\n", $headers),
             'max_redirects' => 1,
-            'content' => $this->buf_);
+            'content' => $this->buf_
+        );
         if ($this->timeout_ > 0) {
             $httpOptions['timeout'] = $this->timeout_;
         }

diff --git a/lib/php/lib/Transport/TMemoryBuffer.php b/lib/php/lib/Transport/TMemoryBuffer.php
@@ -1,4 +1,5 @@
 <?php
+
 /*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements. See the NOTICE file
@@ -35,6 +36,8 @@
  */
 class TMemoryBuffer extends TTransport
 {
+    protected $buf_ = '';
+
     /**
      * Constructor. Optionally pass an initial value
      * for the buffer.
@@ -44,8 +47,6 @@ public function __construct($buf = '')
         $this->buf_ = $buf;
     }
 
-    protected $buf_ = '';
-
     public function isOpen()
     {
         return true;

diff --git a/lib/php/lib/Transport/TPhpStream.php b/lib/php/lib/Transport/TPhpStream.php
@@ -1,4 +1,5 @@
 <?php
+
 /*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements. See the NOTICE file
@@ -53,7 +54,7 @@ public function __construct($mode)
     public function open()
     {
         if ($this->read_) {
-            $this->inStream_ = @fopen(self::inStreamName(), 'r');
+            $this->inStream_ = @fopen($this->inStreamName(), 'r');
             if (!is_resource($this->inStream_)) {
                 throw new TException('TPhpStream: Could not open php://input');
             }
@@ -113,7 +114,7 @@ public function flush()
         @fflush($this->outStream_);
     }
 
-    private static function inStreamName()
+    private function inStreamName()
     {
         if (php_sapi_name() == 'cli') {
             return 'php://stdin';

diff --git a/lib/php/lib/Transport/TSSLSocket.php b/lib/php/lib/Transport/TSSLSocket.php
@@ -36,7 +36,7 @@ class TSSLSocket extends TSocket
     /**
      * Remote port
      *
-     * @var resource
+     * @var null|resource
      */
     protected $context_ = null;
 
@@ -57,6 +57,10 @@ public function __construct(
     ) {
         $this->host_ = $this->getSSLHost($host);
         $this->port_ = $port;
+        // Initialize a stream context if not provided
+        if ($context === null) {
+            $context = stream_context_create();
+        }
         $this->context_ = $context;
         $this->debugHandler_ = $debugHandler ? $debugHandler : 'error_log';
     }
@@ -87,7 +91,8 @@ public function open()
             throw new TTransportException('Socket already connected', TTransportException::ALREADY_OPEN);
         }
 
-        if (empty($this->host_)) {
+        $host = parse_url($this->host_, PHP_URL_HOST);
+        if (empty($host)) {
             throw new TTransportException('Cannot open null host', TTransportException::NOT_OPEN);
         }
 

diff --git a/lib/php/lib/Transport/TSocket.php b/lib/php/lib/Transport/TSocket.php
@@ -252,8 +252,10 @@ public function open()
 
         if (function_exists('socket_import_stream') && function_exists('socket_set_option')) {
             // warnings silenced due to bug https://bugs.php.net/bug.php?id=70939
-            $socket = @socket_import_stream($this->handle_);
-            @socket_set_option($socket, SOL_TCP, TCP_NODELAY, 1);
+            $socket = socket_import_stream($this->handle_);
+            if ($socket !== false) {
+                @socket_set_option($socket, SOL_TCP, TCP_NODELAY, 1);
+            }
         }
     }
 

diff --git a/lib/php/lib/Transport/TSocketPool.php b/lib/php/lib/Transport/TSocketPool.php
@@ -1,4 +1,5 @@
 <?php
+
 /*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements. See the NOTICE file
@@ -24,24 +25,6 @@
 
 use Thrift\Exception\TException;
 
-/**
- * This library makes use of APCu cache to make hosts as down in a web
- * environment. If you are running from the CLI or on a system without APCu
- * installed, then these null functions will step in and act like cache
- * misses.
- */
-if (!function_exists('apcu_fetch')) {
-    function apcu_fetch($key)
-    {
-        return false;
-    }
-
-    function apcu_store($key, $var, $ttl = 0)
-    {
-        return false;
-    }
-}
-
 /**
  * Sockets implementation of the TTransport interface that allows connection
  * to a pool of servers.
@@ -91,6 +74,12 @@ class TSocketPool extends TSocket
      */
     private $alwaysTryLast_ = true;
 
+    /**
+     * Use apcu cache
+     * @var bool
+     */
+    private $useApcuCache;
+
     /**
      * Socket pool constructor
      *
@@ -116,9 +105,13 @@ public function __construct(
         }
 
         foreach ($hosts as $key => $host) {
-            $this->servers_ [] = array('host' => $host,
-                'port' => $ports[$key]);
+            $this->servers_ [] = array(
+                'host' => $host,
+                'port' => $ports[$key]
+            );
         }
+
+        $this->useApcuCache = function_exists('apcu_fetch');
     }
 
     /**
@@ -206,7 +199,7 @@ public function open()
             $failtimeKey = 'thrift_failtime:' . $host . ':' . $port . '~';
 
             // Cache miss? Assume it's OK
-            $lastFailtime = apcu_fetch($failtimeKey);
+            $lastFailtime = $this->apcuFetch($failtimeKey);
             if ($lastFailtime === false) {
                 $lastFailtime = 0;
             }
@@ -251,7 +244,7 @@ public function open()
 
                         // Only clear the failure counts if required to do so
                         if ($lastFailtime > 0) {
-                            apcu_store($failtimeKey, 0);
+                            $this->apcuStore($failtimeKey, 0);
                         }
 
                         // Successful connection, return now
@@ -265,7 +258,7 @@ public function open()
                 $consecfailsKey = 'thrift_consecfails:' . $host . ':' . $port . '~';
 
                 // Ignore cache misses
-                $consecfails = apcu_fetch($consecfailsKey);
+                $consecfails = $this->apcuFetch($consecfailsKey);
                 if ($consecfails === false) {
                     $consecfails = 0;
                 }
@@ -284,12 +277,12 @@ public function open()
                         );
                     }
                     // Store the failure time
-                    apcu_store($failtimeKey, time());
+                    $this->apcuStore($failtimeKey, time());
 
                     // Clear the count of consecutive failures
-                    apcu_store($consecfailsKey, 0);
+                    $this->apcuStore($consecfailsKey, 0);
                 } else {
-                    apcu_store($consecfailsKey, $consecfails);
+                    $this->apcuStore($consecfailsKey, $consecfails);
                 }
             }
         }
@@ -307,4 +300,20 @@ public function open()
         }
         throw new TException($error);
     }
+
+    /**
+     * This library makes use of APCu cache to make hosts as down in a web
+     * environment. If you are running from the CLI or on a system without APCu
+     * installed, then these null functions will step in and act like cache
+     * misses.
+     */
+    private function apcuFetch($key, &$success = null)
+    {
+        return $this->useApcuCache ? apcu_fetch($key, $success) : false;
+    }
+
+    private function apcuStore($key, $var, $ttl = 0)
+    {
+        return $this->useApcuCache ? apcu_store($key, $var, $ttl) : false;
+    }
 }
diff --git a/lib/php/phpunit.xml b/lib/php/phpunit.xml
@@ -28,12 +28,11 @@
          stopOnFailure="true"
          processIsolation="true"
          xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd">
-    <coverage includeUncoveredFiles="true">
-        <include>
-            <directory suffix=".php">./src</directory>
+    <filter>
+        <whitelist processUncoveredFilesFromWhitelist="true">
             <directory suffix=".php">./lib</directory>
-        </include>
-    </coverage>
+        </whitelist>
+    </filter>
     <testsuites>
         <testsuite name="Thrift PHP Test Suite">
             <directory>./test/Unit</directory>

diff --git a/lib/php/test/Fixtures/Fixtures.php b/lib/php/test/Fixtures/Fixtures.php
@@ -17,8 +17,6 @@
  * KIND, either express or implied. See the License for the
  * specific language governing permissions and limitations
  * under the License.
- *
- * @package thrift.test
  */
 
 namespace Test\Thrift\Fixtures;

diff --git a/lib/php/test/Fixtures/TJSONProtocolFixtures.php b/lib/php/test/Fixtures/TJSONProtocolFixtures.php
@@ -17,8 +17,6 @@
  * KIND, either express or implied. See the License for the
  * specific language governing permissions and limitations
  * under the License.
- *
- * @package thrift.test
  */
 
 namespace Test\Thrift\Fixtures;

diff --git a/lib/php/test/Fixtures/TSimpleJSONProtocolFixtures.php b/lib/php/test/Fixtures/TSimpleJSONProtocolFixtures.php
@@ -17,8 +17,6 @@
  * KIND, either express or implied. See the License for the
  * specific language governing permissions and limitations
  * under the License.
- *
- * @package thrift.test
  */
 
 namespace Test\Thrift\Fixtures;

diff --git a/lib/php/test/Unit/Lib/ClassLoader/Fixtures/A/TestClass.php b/lib/php/test/Unit/Lib/ClassLoader/Fixtures/A/TestClass.php
@@ -17,11 +17,6 @@
  * KIND, either express or implied. See the License for the
  * specific language governing permissions and limitations
  * under the License.
- *
- * ClassLoader to load Thrift library and definitions
- * Inspired from UniversalClassLoader from Symfony 2
- *
- * @package thrift.classloader
  */
 
 namespace A;

diff --git a/lib/php/test/Unit/Lib/ClassLoader/Fixtures/B/TestClass.php b/lib/php/test/Unit/Lib/ClassLoader/Fixtures/B/TestClass.php
@@ -17,11 +17,6 @@
  * KIND, either express or implied. See the License for the
  * specific language governing permissions and limitations
  * under the License.
- *
- * ClassLoader to load Thrift library and definitions
- * Inspired from UniversalClassLoader from Symfony 2
- *
- * @package thrift.classloader
  */
 
 namespace B;