From 40b240dc33ceee636bad1da9ea2a87a3add3bf7a Mon Sep 17 00:00:00 2001
From: Alberto Piai <apiai@sensational.ch>
Date: Mon, 23 Dec 2024 12:09:53 +0100
Subject: [PATCH] fix nondeterminism in OptionalMethodCallProcessor (#1231)

An optional method call (`__calls : [ setLocation (80%?): [40.689269,
-74.044737] ]`) would be resolved using an external source of
randomness, rather than the FakerGenerator instance used by all other
value generators, resulting in nondeterministic behavior.

We're making the generator an optional parameter to
OptionalMethodCallProcessor and falling back to random_int for
backwards-compatibility. This can be made non nullable in a next major
release.

(Also see OptionalValueResolver, this is handled in the same way there
https://github.com/nelmio/alice/blob/a7e15b08a64d4f6fb64b4c3b58bd6a710f71050e/src/Generator/Resolver/Value/Chainable/OptionalValueResolver.php#L101-L103)

Fixes gh-1230
---
 .../Chainable/OptionalMethodCallProcessor.php | 19 +++++++++++--
 src/Loader/NativeLoader.php                   |  2 +-
 tests/Loader/LoaderIntegrationTest.php        | 28 +++++++++++++++++++
 3 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/src/Generator/Caller/Chainable/OptionalMethodCallProcessor.php b/src/Generator/Caller/Chainable/OptionalMethodCallProcessor.php
index 16eb9c7c..ad53314d 100644
--- a/src/Generator/Caller/Chainable/OptionalMethodCallProcessor.php
+++ b/src/Generator/Caller/Chainable/OptionalMethodCallProcessor.php
@@ -13,6 +13,7 @@
 
 namespace Nelmio\Alice\Generator\Caller\Chainable;
 
+use Faker\Generator as FakerGenerator;
 use LogicException;
 use Nelmio\Alice\Definition\MethodCall\OptionalMethodCall;
 use Nelmio\Alice\Definition\MethodCallInterface;
@@ -33,14 +34,20 @@ final class OptionalMethodCallProcessor implements ChainableCallProcessorInterfa
      */
     private $processor;
 
-    public function __construct(?CallProcessorInterface $processor = null)
+    /**
+     * @var ?FakerGenerator
+     */
+    private $faker;
+
+    public function __construct(?CallProcessorInterface $processor = null, ?FakerGenerator $faker = null)
     {
         $this->processor = $processor;
+        $this->faker = $faker;
     }
 
     public function withProcessor(CallProcessorInterface $processor): self
     {
-        return new self($processor);
+        return new self($processor, $this->faker);
     }
 
     public function canProcess(MethodCallInterface $methodCall): bool
@@ -65,7 +72,13 @@ public function process(
             throw new LogicException('TODO');
         }
 
-        if (random_int(0, 99) >= $methodCall->getPercentage()) {
+        // TODO: for BC purposes, let's fallback to random_int. The generator should be made
+        //   non-nullable in 4.x and the random_int usage removed. (See OptionalValueResolver)
+        $random = null !== $this->faker
+            ? $this->faker->numberBetween(0, 99)
+            : random_int(0, 99);
+
+        if ($random >= $methodCall->getPercentage()) {
             return $fixtureSet;
         }
 
diff --git a/src/Loader/NativeLoader.php b/src/Loader/NativeLoader.php
index e5c00417..094ef2ec 100644
--- a/src/Loader/NativeLoader.php
+++ b/src/Loader/NativeLoader.php
@@ -633,7 +633,7 @@ protected function createCallProcessor(): CallProcessorInterface
         return new CallProcessorRegistry([
             new ConfiguratorMethodCallProcessor(),
             new MethodCallWithReferenceProcessor(),
-            new OptionalMethodCallProcessor(),
+            new OptionalMethodCallProcessor(null, $this->getFakerGenerator()),
             new SimpleMethodCallProcessor(),
         ]);
     }
diff --git a/tests/Loader/LoaderIntegrationTest.php b/tests/Loader/LoaderIntegrationTest.php
index 3e159924..db8d69b3 100644
--- a/tests/Loader/LoaderIntegrationTest.php
+++ b/tests/Loader/LoaderIntegrationTest.php
@@ -37,6 +37,7 @@
 use Nelmio\Alice\UserDetail;
 use PHPUnit\Framework\TestCase;
 use ReflectionClass;
+use ReflectionObject;
 use stdClass;
 use Throwable;
 use TypeError;
@@ -1575,6 +1576,33 @@ public function testNewlinesInIdentity(): void
         );
     }
 
+    public function testOptionalMethodCallsAreDeterministic(): void
+    {
+        $gen_sequence = function (NativeLoader $loader) {
+            $set = $loader->loadData([
+                FixtureEntity\Caller\Dummy::class => [
+                    'dummy{0..9}' => [
+                        '__construct' => false,
+                        '__calls' => [
+                            ['setTitle (50%?)' => ['Foo']],
+                        ],
+                    ],
+                ],
+            ]);
+            $objs = array_values($set->getObjects());
+
+            return array_map(
+                fn ($obj) => (new ReflectionObject($obj))->getProperty('title')->getValue($obj),
+                $objs,
+            );
+        };
+
+        $seq_1 = $gen_sequence(new NativeLoader());
+        $seq_2 = $gen_sequence(new NativeLoader());
+
+        self::assertSame($seq_1, $seq_2);
+    }
+
     public static function provideFixturesToInstantiate(): iterable
     {
         yield 'with default constructor – use default constructor' => [