diff --git a/lib/php/lib/Factory/TBinaryProtocolAcceleratedFactory.php b/lib/php/lib/Factory/TBinaryProtocolAcceleratedFactory.php new file mode 100644 index 0000000000..011e56bd6c --- /dev/null +++ b/lib/php/lib/Factory/TBinaryProtocolAcceleratedFactory.php @@ -0,0 +1,50 @@ +strictRead, $this->strictWrite); + } +} diff --git a/lib/php/lib/Factory/TBinaryProtocolFactory.php b/lib/php/lib/Factory/TBinaryProtocolFactory.php index c6e69df4d8..76ef2738c2 100644 --- a/lib/php/lib/Factory/TBinaryProtocolFactory.php +++ b/lib/php/lib/Factory/TBinaryProtocolFactory.php @@ -39,11 +39,7 @@ public function __construct( ) { } - /** - * @param TTransport $trans - * @return TBinaryProtocol - */ - public function getProtocol($trans) + public function getProtocol(TTransport $trans): TBinaryProtocol { return new TBinaryProtocol($trans, $this->strictRead, $this->strictWrite); } diff --git a/lib/php/lib/Factory/TCompactProtocolFactory.php b/lib/php/lib/Factory/TCompactProtocolFactory.php index 2a49212de4..8050ddcff7 100644 --- a/lib/php/lib/Factory/TCompactProtocolFactory.php +++ b/lib/php/lib/Factory/TCompactProtocolFactory.php @@ -33,11 +33,7 @@ */ class TCompactProtocolFactory implements TProtocolFactory { - /** - * @param TTransport $trans - * @return TCompactProtocol - */ - public function getProtocol($trans) + public function getProtocol(TTransport $trans): TCompactProtocol { return new TCompactProtocol($trans); } diff --git a/lib/php/lib/Factory/TFramedTransportFactory.php b/lib/php/lib/Factory/TFramedTransportFactory.php index 90726850ea..f82b3b730e 100644 --- a/lib/php/lib/Factory/TFramedTransportFactory.php +++ b/lib/php/lib/Factory/TFramedTransportFactory.php @@ -28,7 +28,7 @@ class TFramedTransportFactory implements TTransportFactoryInterface { - public function getTransport(TTransport $transport) + public function getTransport(TTransport $transport): TFramedTransport { return new TFramedTransport($transport); } diff --git a/lib/php/lib/Factory/TJSONProtocolFactory.php b/lib/php/lib/Factory/TJSONProtocolFactory.php index 7613b34efb..f0f6008a1d 100644 --- a/lib/php/lib/Factory/TJSONProtocolFactory.php +++ b/lib/php/lib/Factory/TJSONProtocolFactory.php @@ -33,11 +33,7 @@ */ class TJSONProtocolFactory implements TProtocolFactory { - /** - * @param TTransport $trans - * @return TJSONProtocol - */ - public function getProtocol($trans) + public function getProtocol(TTransport $trans): TJSONProtocol { return new TJSONProtocol($trans); } diff --git a/lib/php/lib/Factory/TProtocolFactory.php b/lib/php/lib/Factory/TProtocolFactory.php index 22ac0abe62..3021acfcfd 100644 --- a/lib/php/lib/Factory/TProtocolFactory.php +++ b/lib/php/lib/Factory/TProtocolFactory.php @@ -26,6 +26,7 @@ namespace Thrift\Factory; use Thrift\Protocol\TProtocol; +use Thrift\Transport\TTransport; /** * Protocol factory creates protocol objects from transports @@ -34,8 +35,6 @@ interface TProtocolFactory { /** * Build a protocol from the base transport - * - * @return TProtocol protocol */ - public function getProtocol($trans); + public function getProtocol(TTransport $trans): TProtocol; } diff --git a/lib/php/lib/Factory/TTransportFactory.php b/lib/php/lib/Factory/TTransportFactory.php index f1acc42ddf..8182d1fbef 100644 --- a/lib/php/lib/Factory/TTransportFactory.php +++ b/lib/php/lib/Factory/TTransportFactory.php @@ -27,11 +27,7 @@ class TTransportFactory implements TTransportFactoryInterface { - /** - * @param TTransport $transport - * @return TTransport - */ - public function getTransport(TTransport $transport) + public function getTransport(TTransport $transport): TTransport { return $transport; } diff --git a/lib/php/lib/Factory/TTransportFactoryInterface.php b/lib/php/lib/Factory/TTransportFactoryInterface.php index a831fdf487..d0d2981f00 100644 --- a/lib/php/lib/Factory/TTransportFactoryInterface.php +++ b/lib/php/lib/Factory/TTransportFactoryInterface.php @@ -27,9 +27,5 @@ interface TTransportFactoryInterface { - /** - * @param TTransport $transport - * @return TTransport - */ - public function getTransport(TTransport $transport); + public function getTransport(TTransport $transport): TTransport; } diff --git a/lib/php/lib/Server/TForkingServer.php b/lib/php/lib/Server/TForkingServer.php index fa8d67a6ef..7ac5a1cd0c 100644 --- a/lib/php/lib/Server/TForkingServer.php +++ b/lib/php/lib/Server/TForkingServer.php @@ -31,27 +31,22 @@ class TForkingServer extends TServer * Listens for new client using the supplied * transport. We fork when a new connection * arrives. - * - * @return void */ - public function serve() + public function serve(): void { $this->transport->listen(); while (!$this->stop) { try { $transport = $this->transport->accept(); + $pid = pcntl_fork(); - if ($transport != null) { - $pid = pcntl_fork(); - - if ($pid > 0) { - $this->handleParent($transport, $pid); - } elseif ($pid === 0) { - $this->handleChild($transport); - } else { - throw new TException('Failed to fork'); - } + if ($pid > 0) { + $this->handleParent($transport, $pid); + } elseif ($pid === 0) { + $this->handleChild($transport); + } else { + throw new TException('Failed to fork'); } } catch (TTransportException $e) { } @@ -62,23 +57,16 @@ public function serve() /** * Code run by the parent - * - * @param TTransport $transport - * @param int $pid - * @return void */ - private function handleParent(TTransport $transport, $pid) + private function handleParent(TTransport $transport, int $pid): void { $this->children[$pid] = $transport; } /** * Code run by the child. - * - * @param TTransport $transport - * @return void */ - private function handleChild(TTransport $transport) + private function handleChild(TTransport $transport): void { try { $inputTransport = $this->inputTransportFactory->getTransport($transport); @@ -94,12 +82,7 @@ private function handleChild(TTransport $transport) exit(0); } - /** - * Collects any children we may have - * - * @return void - */ - private function collectChildren() + private function collectChildren(): void { $status = null; foreach ($this->children as $pid => $transport) { @@ -115,10 +98,8 @@ private function collectChildren() /** * Stops the server running. Kills the transport * and then stops the main serving loop - * - * @return void */ - public function stop() + public function stop(): void { $this->transport->close(); $this->stop = true; diff --git a/lib/php/lib/Server/TSSLServerSocket.php b/lib/php/lib/Server/TSSLServerSocket.php index 0f401dffa0..ab8f18dd17 100644 --- a/lib/php/lib/Server/TSSLServerSocket.php +++ b/lib/php/lib/Server/TSSLServerSocket.php @@ -54,12 +54,7 @@ public function __construct( $this->context = $context ?? stream_context_create(); } - /** - * Opens a new socket server handle - * - * @return void - */ - public function listen() + public function listen(): void { $this->listener = @stream_socket_server( $this->host . ':' . $this->port, @@ -70,12 +65,7 @@ public function listen() ); } - /** - * Implementation of accept. If not client is accepted in the given time - * - * @return TSocket - */ - protected function acceptImpl() + protected function acceptImpl(): ?TSSLSocket { $handle = @stream_socket_accept($this->listener, $this->acceptTimeout / 1000.0); if (!$handle) { diff --git a/lib/php/lib/Server/TServer.php b/lib/php/lib/Server/TServer.php index 3e6df87bb2..219ef7f362 100644 --- a/lib/php/lib/Server/TServer.php +++ b/lib/php/lib/Server/TServer.php @@ -14,11 +14,6 @@ */ abstract class TServer { - /** - * Sets up all the factories, etc - * - * @param TProcessor $processor - */ public function __construct( protected $processor, protected TServerTransport $transport, @@ -33,17 +28,8 @@ public function __construct( * Serves the server. This should never return * unless a problem permits it to do so or it * is interrupted intentionally - * - * @abstract - * @return void */ - abstract public function serve(); + abstract public function serve(): void; - /** - * Stops the server serving - * - * @abstract - * @return void - */ - abstract public function stop(); + abstract public function stop(): void; } diff --git a/lib/php/lib/Server/TServerSocket.php b/lib/php/lib/Server/TServerSocket.php index 7487e796f0..c1c2aa5111 100644 --- a/lib/php/lib/Server/TServerSocket.php +++ b/lib/php/lib/Server/TServerSocket.php @@ -55,44 +55,23 @@ public function __construct( ) { } - /** - * Sets the accept timeout - * - * @param int $acceptTimeout - * @return void - */ - public function setAcceptTimeout($acceptTimeout) + public function setAcceptTimeout(int $acceptTimeout): void { $this->acceptTimeout = $acceptTimeout; } - /** - * Opens a new socket server handle - * - * @return void - */ - public function listen() + public function listen(): void { $this->listener = stream_socket_server('tcp://' . $this->host . ':' . $this->port); } - /** - * Closes the socket server handle - * - * @return void - */ - public function close() + public function close(): void { @fclose($this->listener); $this->listener = null; } - /** - * Implementation of accept. If not client is accepted in the given time - * - * @return TSocket - */ - protected function acceptImpl() + protected function acceptImpl(): ?TSocket { $handle = @stream_socket_accept($this->listener, $this->acceptTimeout / 1000.0); if (!$handle) { diff --git a/lib/php/lib/Server/TServerTransport.php b/lib/php/lib/Server/TServerTransport.php index f3e98b6447..499c5af2b0 100644 --- a/lib/php/lib/Server/TServerTransport.php +++ b/lib/php/lib/Server/TServerTransport.php @@ -14,43 +14,23 @@ */ abstract class TServerTransport { - /** - * List for new clients - * - * @abstract - * @return void - */ - abstract public function listen(); + abstract public function listen(): void; - /** - * Close the server - * - * @abstract - * @return void - */ - abstract public function close(); + abstract public function close(): void; /** - * Subclasses should use this to implement - * accept. - * - * @abstract - * @return TTransport + * Subclasses implement accept here. */ - abstract protected function acceptImpl(); + abstract protected function acceptImpl(): ?TTransport; /** - * Uses the accept implemtation. If null is returned, an - * exception is thrown. - * - * @throws TTransportException - * @return TTransport + * @throws TTransportException when no transport is available */ - public function accept() + public function accept(): TTransport { $transport = $this->acceptImpl(); - if ($transport == null) { + if ($transport === null) { throw new TTransportException("accept() may not return NULL"); } diff --git a/lib/php/lib/Server/TSimpleServer.php b/lib/php/lib/Server/TSimpleServer.php index 0038f5641f..a00f5e408a 100644 --- a/lib/php/lib/Server/TSimpleServer.php +++ b/lib/php/lib/Server/TSimpleServer.php @@ -22,24 +22,19 @@ class TSimpleServer extends TServer * Listens for new client using the supplied * transport. It handles TTransportExceptions * to avoid timeouts etc killing it - * - * @return void */ - public function serve() + public function serve(): void { $this->transport->listen(); while (!$this->stop) { try { $transport = $this->transport->accept(); - - if ($transport != null) { - $inputTransport = $this->inputTransportFactory->getTransport($transport); - $outputTransport = $this->outputTransportFactory->getTransport($transport); - $inputProtocol = $this->inputProtocolFactory->getProtocol($inputTransport); - $outputProtocol = $this->outputProtocolFactory->getProtocol($outputTransport); - while ($this->processor->process($inputProtocol, $outputProtocol)) { - } + $inputTransport = $this->inputTransportFactory->getTransport($transport); + $outputTransport = $this->outputTransportFactory->getTransport($transport); + $inputProtocol = $this->inputProtocolFactory->getProtocol($inputTransport); + $outputProtocol = $this->outputProtocolFactory->getProtocol($outputTransport); + while ($this->processor->process($inputProtocol, $outputProtocol)) { } } catch (TTransportException $e) { } @@ -49,10 +44,8 @@ public function serve() /** * Stops the server running. Kills the transport * and then stops the main serving loop - * - * @return void */ - public function stop() + public function stop(): void { $this->transport->close(); $this->stop = true; diff --git a/lib/php/test/Unit/Lib/Factory/TBinaryProtocolAcceleratedFactoryTest.php b/lib/php/test/Unit/Lib/Factory/TBinaryProtocolAcceleratedFactoryTest.php new file mode 100644 index 0000000000..ac77cc536a --- /dev/null +++ b/lib/php/test/Unit/Lib/Factory/TBinaryProtocolAcceleratedFactoryTest.php @@ -0,0 +1,84 @@ +createStub(TBufferedTransport::class); + $factory = new TBinaryProtocolAcceleratedFactory($strictRead, $strictWrite); + $protocol = $factory->getProtocol($transport); + + $this->assertInstanceOf(TBinaryProtocolAccelerated::class, $protocol); + $this->assertEquals($strictRead, $this->getPropertyValue($protocol, 'strictRead')); + $this->assertEquals($strictWrite, $this->getPropertyValue($protocol, 'strictWrite')); + } + + public static function getProtocolDataProvider(): \Generator + { + yield 'defaults' => ['strictRead' => false, 'strictWrite' => true]; + yield 'allTrue' => ['strictRead' => true, 'strictWrite' => true]; + yield 'allFalse' => ['strictRead' => false, 'strictWrite' => false]; + yield 'strictReadOnly' => ['strictRead' => true, 'strictWrite' => false]; + } + + public function testGetProtocolDefaultsMatchAcceleratedConstructor(): void + { + $transport = $this->createStub(TBufferedTransport::class); + $protocol = (new TBinaryProtocolAcceleratedFactory())->getProtocol($transport); + + // TBinaryProtocolAccelerated defaults: strictRead=false, strictWrite=true. + $this->assertFalse($this->getPropertyValue($protocol, 'strictRead')); + $this->assertTrue($this->getPropertyValue($protocol, 'strictWrite')); + } + + public function testNonBufferedTransportIsWrapped(): void + { + // TBinaryProtocolAccelerated wraps any transport without putBack() in TBufferedTransport. + $rawTransport = $this->createStub(TTransport::class); + $protocol = (new TBinaryProtocolAcceleratedFactory())->getProtocol($rawTransport); + + $this->assertInstanceOf(TBufferedTransport::class, $protocol->getTransport()); + } + + public function testGetProtocolCreatesNewInstancePerCall(): void + { + $transport = $this->createStub(TBufferedTransport::class); + $factory = new TBinaryProtocolAcceleratedFactory(); + + $this->assertNotSame($factory->getProtocol($transport), $factory->getProtocol($transport)); + } +} diff --git a/lib/php/test/Unit/Lib/Server/Fixture/ServerStub.php b/lib/php/test/Unit/Lib/Server/Fixture/ServerStub.php index a73e24606a..ef174e0f89 100644 --- a/lib/php/test/Unit/Lib/Server/Fixture/ServerStub.php +++ b/lib/php/test/Unit/Lib/Server/Fixture/ServerStub.php @@ -27,11 +27,11 @@ class ServerStub extends TServer { - public function serve() + public function serve(): void { } - public function stop() + public function stop(): void { } } diff --git a/lib/php/test/Unit/Lib/Server/Fixture/ServerTransportStub.php b/lib/php/test/Unit/Lib/Server/Fixture/ServerTransportStub.php index 9f3984c5a4..2b1dc9a211 100644 --- a/lib/php/test/Unit/Lib/Server/Fixture/ServerTransportStub.php +++ b/lib/php/test/Unit/Lib/Server/Fixture/ServerTransportStub.php @@ -24,25 +24,23 @@ namespace Test\Thrift\Unit\Lib\Server\Fixture; use Thrift\Server\TServerTransport; +use Thrift\Transport\TTransport; class ServerTransportStub extends TServerTransport { - private $acceptedTransport; - - public function __construct($acceptedTransport) + public function __construct(private ?TTransport $acceptedTransport) { - $this->acceptedTransport = $acceptedTransport; } - public function listen() + public function listen(): void { } - public function close() + public function close(): void { } - protected function acceptImpl() + protected function acceptImpl(): ?TTransport { return $this->acceptedTransport; } diff --git a/lib/php/test/Unit/Lib/Server/TForkingServerTest.php b/lib/php/test/Unit/Lib/Server/TForkingServerTest.php index abb67e377f..fa903e98d3 100644 --- a/lib/php/test/Unit/Lib/Server/TForkingServerTest.php +++ b/lib/php/test/Unit/Lib/Server/TForkingServerTest.php @@ -39,6 +39,8 @@ class TForkingServerTest extends TestCase use PHPMock; use ReflectionHelper; + private const NO_CONNECTION = 'no connection'; + private function createServer( $processor = null, $transport = null, @@ -107,7 +109,7 @@ public function testServeListensAndLoopsUntilStopped() $server = $this->createServer(null, $serverTransport); - // accept returns null (no connection), then on second call we stop + // accept throws TTransportException (no connection), then on second call we stop $callCount = 0; $serverTransport->method('accept')->willReturnCallback( function () use ($server, &$callCount) { @@ -115,7 +117,7 @@ function () use ($server, &$callCount) { if ($callCount >= 2) { $this->setPropertyValue($server, 'stop', true); } - return null; + throw new TTransportException(self::NO_CONNECTION); } ); @@ -144,7 +146,7 @@ function () use ($server, $clientTransport, &$callCount) { return $clientTransport; } $this->setPropertyValue($server, 'stop', true); - return null; + throw new TTransportException(self::NO_CONNECTION); } ); @@ -201,7 +203,7 @@ function () use ($server, &$callCount) { throw new TTransportException('Connection reset'); } $this->setPropertyValue($server, 'stop', true); - return null; + throw new TTransportException(self::NO_CONNECTION); } ); diff --git a/lib/php/test/Unit/Lib/Server/TSimpleServerTest.php b/lib/php/test/Unit/Lib/Server/TSimpleServerTest.php index 0c302904ef..aa938ee60f 100644 --- a/lib/php/test/Unit/Lib/Server/TSimpleServerTest.php +++ b/lib/php/test/Unit/Lib/Server/TSimpleServerTest.php @@ -29,6 +29,7 @@ use Test\Thrift\Unit\Lib\Server\Fixture\TestProcessor; use Thrift\Factory\TProtocolFactory; use Thrift\Factory\TTransportFactoryInterface; +use Thrift\Protocol\TProtocol; use Thrift\Server\TServerTransport; use Thrift\Server\TSimpleServer; use Thrift\Transport\TTransport; @@ -111,17 +112,17 @@ public function testServe( $this->inputTransportFactory->expects($this->exactly($serveLoopCount)) ->method('getTransport') - ->willReturn($this->createStub(TServerTransport::class)); + ->willReturn($this->createStub(TTransport::class)); $this->outputTransportFactory->expects($this->exactly($serveLoopCount)) ->method('getTransport') - ->willReturn($this->createStub(TServerTransport::class)); + ->willReturn($this->createStub(TTransport::class)); - $inputProtocol = $this->createStub(TServerTransport::class); + $inputProtocol = $this->createStub(TProtocol::class); $this->inputProtocolFactory->expects($this->exactly($serveLoopCount)) ->method('getProtocol') ->willReturn($inputProtocol); - $outputProtocol = $this->createStub(TServerTransport::class); + $outputProtocol = $this->createStub(TProtocol::class); $this->outputProtocolFactory->expects($this->exactly($serveLoopCount)) ->method('getProtocol') ->willReturn($outputProtocol); diff --git a/test/php/TestServer.php b/test/php/TestServer.php index e16d5f3b94..4ab773cb88 100644 --- a/test/php/TestServer.php +++ b/test/php/TestServer.php @@ -4,14 +4,6 @@ require_once __DIR__ . '/../../vendor/autoload.php'; -class TBinaryProtocolAcceleratedFactory implements \Thrift\Factory\TProtocolFactory -{ - public function getProtocol($trans) - { - return new \Thrift\Protocol\TBinaryProtocolAccelerated($trans, false, true); - } -} - $opts = getopt( 'h::', [ @@ -88,7 +80,7 @@ public function getProtocol($trans) fwrite(STDERR, "Acceleration extension is not loaded\n"); exit(1); } - $protocolFactory = new TBinaryProtocolAcceleratedFactory(); + $protocolFactory = new \Thrift\Factory\TBinaryProtocolAcceleratedFactory(); break; case 'compact': $protocolFactory = new \Thrift\Factory\TCompactProtocolFactory();