Browse Source

Merge branch '4.next' into 5.x

Corey Taylor 4 years ago
parent
commit
cd28c6d5bd

+ 6 - 1
src/Console/ConsoleOptionParser.php

@@ -705,8 +705,13 @@ class ConsoleOptionParser
                 $args = $this->_parseArg($token, $args);
             }
         }
+
+        if (isset($params['help'])) {
+            return [$params, $args];
+        }
+
         foreach ($this->_args as $i => $arg) {
-            if ($arg->isRequired() && !isset($args[$i]) && empty($params['help'])) {
+            if ($arg->isRequired() && !isset($args[$i])) {
                 throw new ConsoleException(
                     sprintf('Missing required argument. The `%s` argument is required.', $arg->name())
                 );

+ 13 - 4
src/Controller/ControllerFactory.php

@@ -172,8 +172,16 @@ class ControllerFactory implements ControllerFactoryInterface, RequestHandlerInt
 
             // Check for dependency injection for classes
             if ($type instanceof ReflectionNamedType && !$type->isBuiltin()) {
-                if ($this->container->has($type->getName())) {
-                    $resolved[] = $this->container->get($type->getName());
+                $typeName = $type->getName();
+                if ($this->container->has($typeName)) {
+                    $resolved[] = $this->container->get($typeName);
+                    continue;
+                }
+
+                // Use passedParams as a source of typed dependencies.
+                // The accepted types for passedParams was never defined and userland code relies on that.
+                if ($passedParams && is_object($passedParams[0]) && $passedParams[0] instanceof $typeName) {
+                    $resolved[] = array_shift($passedParams);
                     continue;
                 }
 
@@ -187,6 +195,7 @@ class ControllerFactory implements ControllerFactoryInterface, RequestHandlerInt
                 throw new InvalidParameterException([
                     'template' => 'missing_dependency',
                     'parameter' => $parameter->getName(),
+                    'type' => $typeName,
                     'controller' => $this->controller->getName(),
                     'action' => $this->controller->getRequest()->getParam('action'),
                     'prefix' => $this->controller->getRequest()->getParam('prefix'),
@@ -197,7 +206,7 @@ class ControllerFactory implements ControllerFactoryInterface, RequestHandlerInt
             // Use any passed params as positional arguments
             if ($passedParams) {
                 $argument = array_shift($passedParams);
-                if ($type instanceof ReflectionNamedType) {
+                if (is_string($argument) && $type instanceof ReflectionNamedType) {
                     $typedArgument = $this->coerceStringToType($argument, $type);
 
                     if ($typedArgument === null) {
@@ -262,7 +271,7 @@ class ControllerFactory implements ControllerFactoryInterface, RequestHandlerInt
             case 'bool':
                 return $argument === '0' ? false : ($argument === '1' ? true : null);
             case 'array':
-                return explode(',', $argument);
+                return $argument === '' ? [] : explode(',', $argument);
         }
 
         return null;

+ 2 - 1
src/Controller/Exception/InvalidParameterException.php

@@ -27,7 +27,8 @@ class InvalidParameterException extends CakeException
      */
     protected array $templates = [
         'failed_coercion' => 'Unable to coerce "%s" to `%s` for `%s` in action %s::%s().',
-        'missing_dependency' => 'Failed to inject dependency from service container for `%s` in action %s::%s().',
+        'missing_dependency' => 'Failed to inject dependency from service container for parameter `%s` ' .
+            'with type `%s` in action %s::%s().',
         'missing_parameter' => 'Missing passed parameter for `%s` in action %s::%s().',
         'unsupported_type' => 'Type declaration for `%s` in action %s::%s() is unsupported.',
     ];

+ 6 - 3
src/Error/ErrorTrap.php

@@ -16,7 +16,7 @@ use InvalidArgumentException;
  *
  * Using the `register()` method you can attach an ErrorTrap to PHP's default error handler.
  *
- * When errors are trapped, errors are logged (if logging is enabled). Then the `Error.handled` event is triggered.
+ * When errors are trapped, errors are logged (if logging is enabled). Then the `Error.beforeRender` event is triggered.
  * Finally, errors are 'rendered' using the defined renderer. If no error renderer is defined in configuration
  * one of the default implementations will be chosen based on the PHP SAPI.
  */
@@ -93,7 +93,7 @@ class ErrorTrap
      * Will use the configured renderer to generate output
      * and output it.
      *
-     * This method will dispatch the `Error.handled` event which can be listened
+     * This method will dispatch the `Error.beforeRender` event which can be listened
      * to on the global event manager.
      *
      * @param int $code Code of error
@@ -126,7 +126,10 @@ class ErrorTrap
         try {
             // Log first incase rendering or event listeners fail
             $logger->logMessage($error->getLabel(), $error->getMessage());
-            $this->dispatchEvent('Error.handled', ['error' => $error]);
+            $event = $this->dispatchEvent('Error.beforeRender', ['error' => $error]);
+            if ($event->isStopped()) {
+                return true;
+            }
             $renderer->write($renderer->render($error, $debug));
         } catch (Exception $e) {
             $logger->logMessage('error', 'Could not render error. Got: ' . $e->getMessage());

+ 7 - 3
src/Error/ExceptionTrap.php

@@ -17,9 +17,13 @@ use Throwable;
  * Using the `register()` method you can attach an ExceptionTrap to PHP's default exception handler and register
  * a shutdown handler to handle fatal errors.
  *
- * When exceptions are trapped the `Exception.handled` event is triggered.
+ * When exceptions are trapped the `Exception.beforeRender` event is triggered.
  * Then exceptions are logged (if enabled) and finally 'rendered' using the defined renderer.
  *
+ * Stopping the `Exception.beforeRender` event has no effect, as we always need to render
+ * a response to an exception and custom renderers should be used if you want to replace or
+ * skip rendering an exception.
+ *
  * If undefined, an ExceptionRenderer will be selected based on the current SAPI (CLI or Web).
  */
 class ExceptionTrap
@@ -309,7 +313,7 @@ class ExceptionTrap
      * Primarily a public function to ensure consistency between global exception handling
      * and the ErrorHandlerMiddleware.
      *
-     * After logging, the `Exception.handled` event is triggered.
+     * After logging, the `Exception.beforeRender` event is triggered.
      *
      * @param \Throwable $exception The exception to log
      * @param \Psr\Http\Message\ServerRequestInterface|null $request The optional request
@@ -319,7 +323,7 @@ class ExceptionTrap
     {
         $logger = $this->logger();
         $logger->log($exception, $request);
-        $this->dispatchEvent('Exception.handled', ['exception' => $exception]);
+        $this->dispatchEvent('Exception.beforeRender', ['exception' => $exception]);
     }
 
     /**

+ 5 - 0
src/I18n/TranslatorRegistry.php

@@ -202,6 +202,11 @@ class TranslatorRegistry
         $key = "translations.{$keyName}.{$locale}";
         /** @var \Cake\I18n\Translator|null $translator */
         $translator = $this->_cacher->get($key);
+
+        // PHP <8.1 does not correctly garbage collect strings created
+        // by unserialized arrays.
+        gc_collect_cycles();
+
         if (!$translator) {
             $translator = $this->_getTranslator($name, $locale);
             $this->_cacher->set($key, $translator);

+ 15 - 2
tests/TestCase/Console/ConsoleOptionParserTest.php

@@ -695,9 +695,9 @@ class ConsoleOptionParserTest extends TestCase
     }
 
     /**
-     * test that no exception is triggered when help is being generated
+     * test that no exception is triggered for required arguments when help is being generated
      */
-    public function testHelpNoExceptionWhenGettingHelp(): void
+    public function testHelpNoExceptionForRequiredArgumentsWhenGettingHelp(): void
     {
         $parser = new ConsoleOptionParser('mycommand', false);
         $parser->addOption('test', ['help' => 'A test option.'])
@@ -708,6 +708,19 @@ class ConsoleOptionParserTest extends TestCase
     }
 
     /**
+     * test that no exception is triggered for required options when help is being generated
+     */
+    public function testHelpNoExceptionForRequiredOptionsWhenGettingHelp(): void
+    {
+        $parser = new ConsoleOptionParser('mycommand', false);
+        $parser->addOption('test', ['help' => 'A test option.'])
+            ->addOption('model', ['help' => 'The model to make.', 'required' => true]);
+
+        $result = $parser->parse(['--help']);
+        $this->assertTrue($result[0]['help']);
+    }
+
+    /**
      * test that help() with a command param shows the help for a subcommand
      */
     public function testHelpSubcommandHelp(): void

+ 69 - 4
tests/TestCase/Controller/ControllerFactoryTest.php

@@ -246,7 +246,7 @@ class ControllerFactoryTest extends TestCase
     }
 
     /**
-     * Test create() injecting dependcies on defined controllers.
+     * Test create() injecting dependencies on defined controllers.
      */
     public function testCreateWithContainerDependenciesNoController(): void
     {
@@ -265,7 +265,7 @@ class ControllerFactoryTest extends TestCase
     }
 
     /**
-     * Test create() injecting dependcies on defined controllers.
+     * Test create() injecting dependencies on defined controllers.
      */
     public function testCreateWithContainerDependenciesWithController(): void
     {
@@ -440,7 +440,7 @@ class ControllerFactoryTest extends TestCase
     }
 
     /**
-     * Ensure that a controllers startup process can emit a response
+     * Test invoke passing basic typed data from pass parameters.
      */
     public function testInvokeInjectParametersOptionalWithPassedParameters(): void
     {
@@ -465,6 +465,52 @@ class ControllerFactoryTest extends TestCase
     }
 
     /**
+     * Test invoke() injecting dependencies that exist in passed params as objects.
+     * The accepted types of `params.pass` was never enforced and userland code has
+     * creative uses of this previously unspecified behavior.
+     */
+    public function testCreateWithContainerDependenciesWithObjectRouteParam(): void
+    {
+        $inject = new stdClass();
+        $inject->id = uniqid();
+
+        $request = new ServerRequest([
+            'url' => 'test_plugin_three/dependencies/index',
+            'params' => [
+                'plugin' => null,
+                'controller' => 'Dependencies',
+                'action' => 'requiredDep',
+                'pass' => [$inject],
+            ],
+        ]);
+        $controller = $this->factory->create($request);
+        $response = $this->factory->invoke($controller);
+
+        $data = json_decode((string)$response->getBody());
+        $this->assertNotNull($data);
+        $this->assertEquals($data->dep->id, $inject->id);
+    }
+
+    public function testCreateWithNonStringScalarRouteParam(): void
+    {
+        $request = new ServerRequest([
+            'url' => 'test_plugin_three/dependencies/required_typed',
+            'params' => [
+                'plugin' => null,
+                'controller' => 'Dependencies',
+                'action' => 'requiredTyped',
+                'pass' => [1.1, 2, true, ['foo' => 'bar']],
+            ],
+        ]);
+        $controller = $this->factory->create($request);
+        $response = $this->factory->invoke($controller);
+
+        $expected = ['one' => 1.1, 'two' => 2, 'three' => true, 'four' => ['foo' => 'bar']];
+        $data = json_decode((string)$response->getBody(), true);
+        $this->assertSame($expected, $data);
+    }
+
+    /**
      * Ensure that a controllers startup process can emit a response
      */
     public function testInvokeInjectParametersRequiredDefined(): void
@@ -504,7 +550,9 @@ class ControllerFactoryTest extends TestCase
         $controller = $this->factory->create($request);
 
         $this->expectException(InvalidParameterException::class);
-        $this->expectExceptionMessage('Failed to inject dependency from service container for `dep` in action Dependencies::requiredDep()');
+        $this->expectExceptionMessage(
+            'Failed to inject dependency from service container for parameter `dep` with type `stdClass` in action Dependencies::requiredDep()'
+        );
         $this->factory->invoke($controller);
     }
 
@@ -695,6 +743,23 @@ class ControllerFactoryTest extends TestCase
 
         $this->assertNotNull($data);
         $this->assertSame(['one' => 1.0, 'two' => 2, 'three' => false, 'four' => ['8', '9']], $data);
+
+        $request = new ServerRequest([
+            'url' => 'test_plugin_three/dependencies/requiredTyped',
+            'params' => [
+                'plugin' => null,
+                'controller' => 'Dependencies',
+                'action' => 'requiredTyped',
+                'pass' => ['1.0', '02', '0', ''],
+            ],
+        ]);
+        $controller = $this->factory->create($request);
+
+        $result = $this->factory->invoke($controller);
+        $data = json_decode((string)$result->getBody(), true);
+
+        $this->assertNotNull($data);
+        $this->assertSame(['one' => 1.0, 'two' => 2, 'three' => false, 'four' => []], $data);
     }
 
     /**

+ 18 - 1
tests/TestCase/Error/ErrorTrapTest.php

@@ -150,7 +150,7 @@ class ErrorTrapTest extends TestCase
     {
         $trap = new ErrorTrap(['errorRenderer' => TextErrorRenderer::class]);
         $trap->register();
-        $trap->getEventManager()->on('Error.handled', function ($event, PhpError $error) {
+        $trap->getEventManager()->on('Error.beforeRender', function ($event, PhpError $error) {
             $this->assertEquals(E_USER_NOTICE, $error->getCode());
             $this->assertStringContainsString('Oh no it was bad', $error->getMessage());
         });
@@ -161,4 +161,21 @@ class ErrorTrapTest extends TestCase
         restore_error_handler();
         $this->assertNotEmpty($out);
     }
+
+    public function testEventTriggeredAbortRender()
+    {
+        $trap = new ErrorTrap(['errorRenderer' => TextErrorRenderer::class]);
+        $trap->register();
+        $trap->getEventManager()->on('Error.beforeRender', function ($event, PhpError $error) {
+            $this->assertEquals(E_USER_NOTICE, $error->getCode());
+            $this->assertStringContainsString('Oh no it was bad', $error->getMessage());
+            $event->stopPropagation();
+        });
+
+        ob_start();
+        trigger_error('Oh no it was bad', E_USER_NOTICE);
+        $out = ob_get_clean();
+        restore_error_handler();
+        $this->assertSame('', $out);
+    }
 }

+ 2 - 1
tests/TestCase/Error/ExceptionTrapTest.php

@@ -219,9 +219,10 @@ class ExceptionTrapTest extends TestCase
     public function testEventTriggered()
     {
         $trap = new ExceptionTrap(['exceptionRenderer' => TextExceptionRenderer::class]);
-        $trap->getEventManager()->on('Exception.handled', function ($event, Throwable $error) {
+        $trap->getEventManager()->on('Exception.beforeRender', function ($event, Throwable $error) {
             $this->assertEquals(100, $error->getCode());
             $this->assertStringContainsString('nope', $error->getMessage());
+            $event->stopPropagation();
         });
         $error = new InvalidArgumentException('nope', 100);