diff --git a/UPGRADE.md b/UPGRADE.md index 074f7922..762b315a 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -22,6 +22,35 @@ vendor/bin/rector process src Le set est **cumulatif** : le passer une fois rattrape toutes les versions franchies d'un coup. Il ne contient que ce que Rector sait faire sans deviner ; tout le reste est écrit à la main ci-dessous. +## Non publié + +### Les stubs refusent ce que PHP refuse + +**Qui est concerné** : toute application qui appelle un contrat d'activité, d'opération Nexus ou de +workflow enfant par un stub. Rien à écrire ; des appels qui passaient en silence lèvent désormais, +et c'est le but. + +Les trois stubs transforment les arguments reçus par `__call` en une charge nommée. Trois fautes +d'appel y disparaissaient sans un mot, et voyageaient jusque dans le journal — où elles se rejouent +à l'identique, passe après passe, loin de l'appel fautif : + +| L'appel | Avant | Maintenant | Ce que PHP fait sur l'appel ordinaire | +|---|---|---|---| +| argument nommé inconnu | ignoré | `BadMethodCallException` | `Error: Unknown named parameter` | +| paramètre requis non fourni | vaut `null` | `BadMethodCallException` | `ArgumentCountError` | +| paramètre servi en positionnel **et** en nommé | le positionnel gagne | `BadMethodCallException` | `Error: Named parameter overwrites previous argument` | + +Le type est `\BadMethodCallException` et non celui de PHP parce que l'appel passe par `__call` : +c'est l'exception que la SPL réserve à une méthode appelée de travers, et elle reste rattrapable. + +**Si une de ces exceptions apparaît en production**, elle désigne un appel qui était déjà faux : un +workflow enfant démarré avec un paramètre manquant partait avec `null` et attendait un message qui +ne venait jamais. Rector ne peut rien ici — la correction est dans votre code d'appel, pas dans une +forme mécanique. + +Ces exceptions sont déterministes : rejouées à l'identique à chaque redélivrance, elles brûlent les +tentatives de Messenger jusqu'au transport d'échec. Configurez-en un. + ## 0.1.0-alpha8 ### Laravel refuse au démarrage un workflow dont les noms de paramètres divergent du contrat diff --git a/src/Durable/Activity/ActivityStub.php b/src/Durable/Activity/ActivityStub.php index c1d2ce4c..25362b9b 100644 --- a/src/Durable/Activity/ActivityStub.php +++ b/src/Durable/Activity/ActivityStub.php @@ -5,6 +5,7 @@ namespace Gplanchat\Durable\Activity; use Gplanchat\Durable\Awaitable\Awaitable; +use Gplanchat\Durable\Stub\StubArguments; /** * Proxy de planification côté workflow. @@ -55,14 +56,6 @@ public function __call(string $name, array $arguments): Awaitable */ private function argumentsToPayload(string $methodName, array $arguments): array { - $reflection = new \ReflectionMethod($this->contractClass, $methodName); - $params = $reflection->getParameters(); - $payload = []; - foreach ($params as $i => $param) { - $key = $param->getName(); - $payload[$key] = $arguments[$i] ?? ($param->isDefaultValueAvailable() ? $param->getDefaultValue() : null); - } - - return $payload; + return StubArguments::toPayload(new \ReflectionMethod($this->contractClass, $methodName), $arguments); } } diff --git a/src/Durable/Nexus/NexusStub.php b/src/Durable/Nexus/NexusStub.php index 4eed716c..6c46b52a 100644 --- a/src/Durable/Nexus/NexusStub.php +++ b/src/Durable/Nexus/NexusStub.php @@ -6,6 +6,7 @@ use Gplanchat\Durable\Awaitable\Awaitable; use Gplanchat\Durable\Nexus\Serving\NexusContractResolver; +use Gplanchat\Durable\Stub\StubArguments; /** * Proxy de planification côté appelant. @@ -76,12 +77,6 @@ public function __call(string $name, array $arguments): Awaitable */ private function argumentsToPayload(string $methodName, array $arguments): array { - $payload = []; - foreach ((new \ReflectionMethod($this->contractClass, $methodName))->getParameters() as $i => $param) { - $payload[$param->getName()] = $arguments[$i] - ?? ($param->isDefaultValueAvailable() ? $param->getDefaultValue() : null); - } - - return $payload; + return StubArguments::toPayload(new \ReflectionMethod($this->contractClass, $methodName), $arguments); } } diff --git a/src/Durable/Stub/StubArguments.php b/src/Durable/Stub/StubArguments.php new file mode 100644 index 00000000..23a2095f --- /dev/null +++ b/src/Durable/Stub/StubArguments.php @@ -0,0 +1,122 @@ + $arguments tels que `__call` les a reçus : les positionnels + * sous des indices, les nommés sous leur nom + * + * @return array la charge nommée, un paramètre du contrat par clé + * + * @throws \BadMethodCallException si un argument nommé ne correspond à aucun paramètre, si un + * paramètre requis n'est pas fourni, ou si un paramètre est + * servi à la fois en positionnel et en nommé + */ + public static function toPayload(\ReflectionFunctionAbstract $method, array $arguments): array + { + $payload = []; + $connus = []; + + foreach ($method->getParameters() as $i => $param) { + $name = $param->getName(); + $connus[$name] = true; + + // Un variadique n'a ni valeur par défaut ni obligation : il ne peut pas manquer, et + // il n'a pas de place à lui dans une charge nommée. + if ($param->isVariadic()) { + continue; + } + + $parPosition = \array_key_exists($i, $arguments); + $parNom = \array_key_exists($name, $arguments); + + if ($parPosition && $parNom) { + throw new \BadMethodCallException(\sprintf( + 'Parameter $%s of %s() was given both positionally and by name.', + $name, + self::describe($method), + )); + } + + if ($parPosition) { + $payload[$name] = $arguments[$i]; + + continue; + } + + if ($parNom) { + $payload[$name] = $arguments[$name]; + + continue; + } + + if ($param->isDefaultValueAvailable()) { + $payload[$name] = $param->getDefaultValue(); + + continue; + } + + // Le retomber-sur-`null` d'ici traversait le journal sans un mot, et se rejouait à + // l'identique à chaque passe : le paramètre est requis, son absence est une faute + // d'appel, et un type non nullable l'aurait de toute façon refusée à l'arrivée. + throw new \BadMethodCallException(\sprintf( + 'Missing required argument $%s for %s().', + $name, + self::describe($method), + )); + } + + foreach ($arguments as $key => $_) { + if (\is_string($key) && !isset($connus[$key])) { + throw new \BadMethodCallException(\sprintf( + 'Unknown named parameter $%s for %s(); known parameters: %s.', + $key, + self::describe($method), + implode(', ', array_map(static fn(string $n): string => '$' . $n, array_keys($connus))), + )); + } + } + + return $payload; + } + + private static function describe(\ReflectionFunctionAbstract $method): string + { + return $method instanceof \ReflectionMethod + ? $method->getDeclaringClass()->getName() . '::' . $method->getName() + : $method->getName(); + } +} diff --git a/src/Durable/Workflow/ChildWorkflowStub.php b/src/Durable/Workflow/ChildWorkflowStub.php index db541a41..621b3ec9 100644 --- a/src/Durable/Workflow/ChildWorkflowStub.php +++ b/src/Durable/Workflow/ChildWorkflowStub.php @@ -5,6 +5,7 @@ namespace Gplanchat\Durable\Workflow; use Gplanchat\Durable\ChildWorkflowOptions; +use Gplanchat\Durable\Stub\StubArguments; /** * Proxy de planification côté workflow pour exécuter un workflow enfant typé. @@ -59,13 +60,6 @@ public function __call(string $name, array $arguments): \Gplanchat\Durable\Await */ private function argumentsToInput(array $arguments): array { - $params = $this->workflowMethod->getParameters(); - $input = []; - foreach ($params as $i => $param) { - $key = $param->getName(); - $input[$key] = $arguments[$i] ?? ($param->isDefaultValueAvailable() ? $param->getDefaultValue() : null); - } - - return $input; + return StubArguments::toPayload($this->workflowMethod, $arguments); } } diff --git a/tests/unit/Durable/Stub/StubArgumentsTest.php b/tests/unit/Durable/Stub/StubArgumentsTest.php new file mode 100644 index 00000000..e8126520 --- /dev/null +++ b/tests/unit/Durable/Stub/StubArgumentsTest.php @@ -0,0 +1,112 @@ + 'bonjour', 'times' => 3, 'tag' => 'défaut'], $this->map(['bonjour', 3])); + } + + /** + * Le défaut qui a coûté un après-midi : PHP passe les arguments nommés à `__call` dans un + * tableau à **clés de chaînes**. Appariés par indice, ils disparaissaient tous — et chaque + * paramètre retombait sur sa valeur par défaut, sans exception ni trace. + */ + public function testNamedArgumentsLandOnTheirParameter(): void + { + self::assertSame(['text' => 'bonjour', 'times' => 1, 'tag' => 'perso'], $this->map(['tag' => 'perso', 'text' => 'bonjour'])); + } + + public function testPositionalAndNamedArgumentsMix(): void + { + self::assertSame(['text' => 'bonjour', 'times' => 1, 'tag' => 'perso'], $this->map(['bonjour', 'tag' => 'perso'])); + } + + /** + * `??` confondait « absent » et « null » : passer explicitement `null` rendait la valeur par + * défaut, c'est-à-dire l'inverse de ce qui était demandé. + */ + public function testAnExplicitNullIsNotTheDefault(): void + { + self::assertNull($this->map(['text' => 'x', 'tag' => null])['tag']); + self::assertNull($this->map(['x', 1, null])['tag']); + } + + /** + * Une faute de frappe dans un nom d'argument ne doit pas être indiscernable d'une valeur par + * défaut voulue. PHP lève sur un appel ordinaire ; le stub aussi. + */ + public function testAnUnknownNamedArgumentIsRefused(): void + { + $this->expectException(\BadMethodCallException::class); + $this->expectExceptionMessageMatches('/Unknown named parameter \$tags/'); + + $this->map(['text' => 'x', 'tags' => 'perso']); + } + + /** + * Un paramètre requis non fourni lève, comme PHP lèverait `ArgumentCountError` sur l'appel + * ordinaire correspondant. + * + * Le laisser valoir `null` faisait voyager la faute jusque dans le journal, où elle se rejoue + * à l'identique à chaque passe : `$text` est déclaré `string`, une charge portant `null` est + * donc de toute façon refusée à l'arrivée — mais une passe de rejeu plus tard, dans un worker, + * loin de l'appel fautif. + */ + public function testUnParametreRequisNonFourniLeve(): void + { + $this->expectException(\BadMethodCallException::class); + $this->expectExceptionMessageMatches('/Missing required argument \$text/'); + + $this->map([]); + } + + /** + * Servir le même paramètre en positionnel puis en nommé : PHP refuse (« Named parameter $x + * overwrites previous argument »), le stub choisissait le positionnel en silence. + */ + public function testUnParametreServiDeuxFoisLeve(): void + { + $this->expectException(\BadMethodCallException::class); + $this->expectExceptionMessageMatches('/\$text.*both positionally and by name/'); + + $this->map([0 => 'positionnel', 'text' => 'nommé']); + } + + /** + * Le cas voisin, qui doit continuer de passer : un paramètre optionnel non fourni prend sa + * valeur par défaut, et un `null` explicite reste `null`. + */ + public function testUnParametreOptionnelGardeSonDefautEtAccepteNull(): void + { + self::assertSame(1, $this->map(['text' => 'x'])['times']); + self::assertNull($this->map(['text' => 'x', 'tag' => null])['tag']); + } + + /** + * @param array $arguments + * + * @return array + */ + private function map(array $arguments): array + { + $contract = new class { + public function greet(string $text, int $times = 1, ?string $tag = 'défaut'): void {} + }; + + return StubArguments::toPayload(new \ReflectionMethod($contract, 'greet'), $arguments); + } +}