From 9197b8147d46540a803718893ef5350661d56a1a Mon Sep 17 00:00:00 2001 From: Nicolas PHILIPPE Date: Mon, 6 Nov 2023 15:17:26 +0100 Subject: [PATCH] refactor: deprecate stuff in Instantiator --- phpunit.dama.xml.dist | 2 +- .../ZenstruckFoundryExtension.php | 3 +- src/Bundle/Resources/config/services.xml | 4 +- src/Configuration.php | 2 +- src/Instantiator.php | 43 ++++++++- tests/Fixtures/Factories/CategoryFactory.php | 2 +- tests/Fixtures/Factories/PostFactory.php | 2 +- .../Functional/FactoryDoctrineCascadeTest.php | 12 +-- .../ZenstruckFoundryExtensionTest.php | 8 +- tests/Unit/InstantiatorTest.php | 94 +++++++++++++------ 10 files changed, 128 insertions(+), 44 deletions(-) diff --git a/phpunit.dama.xml.dist b/phpunit.dama.xml.dist index dafc5081d..1ba329313 100644 --- a/phpunit.dama.xml.dist +++ b/phpunit.dama.xml.dist @@ -11,7 +11,7 @@ - + diff --git a/src/Bundle/DependencyInjection/ZenstruckFoundryExtension.php b/src/Bundle/DependencyInjection/ZenstruckFoundryExtension.php index 33e2a5959..ede98733f 100644 --- a/src/Bundle/DependencyInjection/ZenstruckFoundryExtension.php +++ b/src/Bundle/DependencyInjection/ZenstruckFoundryExtension.php @@ -22,6 +22,7 @@ use Symfony\Component\HttpKernel\DependencyInjection\ConfigurableExtension; use Zenstruck\Foundry\Bundle\Command\StubMakeFactory; use Zenstruck\Foundry\Bundle\Command\StubMakeStory; +use Zenstruck\Foundry\Instantiator; use Zenstruck\Foundry\Persistence\PersistentProxyObjectFactory; use Zenstruck\Foundry\Story; use Zenstruck\Foundry\Test\ORMDatabaseResetter; @@ -87,7 +88,7 @@ private function configureDefaultInstantiator(array $config, ContainerBuilder $c $definition = $container->getDefinition('.zenstruck_foundry.default_instantiator'); if ($config['without_constructor']) { - $definition->addMethodCall('withoutConstructor'); + $definition->setFactory([Instantiator::class, 'withoutConstructor']); } if ($config['allow_extra_attributes']) { diff --git a/src/Bundle/Resources/config/services.xml b/src/Bundle/Resources/config/services.xml index aef3fe9a2..cf9e0931e 100644 --- a/src/Bundle/Resources/config/services.xml +++ b/src/Bundle/Resources/config/services.xml @@ -5,7 +5,9 @@ https://symfony.com/schema/dic/services/services-1.0.xsd"> - + + + diff --git a/src/Configuration.php b/src/Configuration.php index c93ae99d0..e779d62e3 100644 --- a/src/Configuration.php +++ b/src/Configuration.php @@ -55,7 +55,7 @@ public function __construct(private array $ormConnectionsToReset, private array $this->stories = new StoryManager([]); $this->factories = new ModelFactoryManager([]); $this->faker = Faker\Factory::create(); - $this->instantiator = new Instantiator(); + $this->instantiator = Instantiator::withConstructor(); } public function stories(): StoryManager diff --git a/src/Instantiator.php b/src/Instantiator.php index bb33dbed1..4772266af 100644 --- a/src/Instantiator.php +++ b/src/Instantiator.php @@ -19,6 +19,9 @@ /** * @author Kevin Bond + * + * @method static self withoutConstructor() + * @method self withoutConstructor() */ final class Instantiator { @@ -35,6 +38,13 @@ final class Instantiator /** @var string[] */ private array $forceProperties = []; + public function __construct(bool $calledInternally = false) + { + if (!$calledInternally) { + trigger_deprecation('zenstruck\foundry', '1.37.0', sprintf('%1$s constructor will be private in Foundry 2.0. Use either "%1$s::withConstructor()" or "%1$s::withoutConstructor()"', self::class)); + } + } + /** * @param class-string $class */ @@ -90,16 +100,37 @@ public function __invoke(array $attributes, string $class): object return $object; } - /** - * Instantiate objects without calling the constructor. - */ - public function withoutConstructor(): self + public function __call(string $name, array $arguments): self { + if ('withoutConstructor' !== $name) { + throw new \BadMethodCallException(\sprintf('Call to undefined method "%s::%s".', static::class, $name)); + } + + trigger_deprecation('zenstruck/foundry', '1.37.0', 'Calling instance method "%1$s::withoutConstructor()" is deprecated and will be removed in 2.0. Use static call instead: "%1$s::withoutConstructor()" instead.', static::class); + $this->withoutConstructor = true; return $this; } + public static function __callStatic(string $name, array $arguments): self + { + if ('withoutConstructor' !== $name) { + throw new \BadMethodCallException(\sprintf('Call to undefined method "%s::%s".', static::class, $name)); + } + + $instance = new self(calledInternally: true); + + $instance->withoutConstructor = true; + + return $instance; + } + + public static function withConstructor(): self + { + return new self(calledInternally: true); + } + /** * Ignore attributes that can't be set to object. * @@ -240,6 +271,10 @@ private function instantiate(string $class, array &$attributes): object $constructor = $class->getConstructor(); if ($this->withoutConstructor || !$constructor || !$constructor->isPublic()) { + if (!$this->withoutConstructor && $constructor && !$constructor->isPublic()) { + trigger_deprecation('zenstruck\foundry', '1.37.0', 'Instantiator was created to instantiate "%s" by calling the constructor whereas the constructor is not public. This is deprecated and will throw an exception in Foundry 2.0. Use "%s::withoutConstructor()" instead or make constructor public.', $class->getName(), self::class); + } + return $class->newInstanceWithoutConstructor(); } diff --git a/tests/Fixtures/Factories/CategoryFactory.php b/tests/Fixtures/Factories/CategoryFactory.php index 70956fd2b..fc94dbc90 100644 --- a/tests/Fixtures/Factories/CategoryFactory.php +++ b/tests/Fixtures/Factories/CategoryFactory.php @@ -25,7 +25,7 @@ protected function initialize(): static { return $this ->instantiateWith( - (new Instantiator())->allowExtraAttributes(['extraPostsBeforeInstantiate', 'extraPostsAfterInstantiate']) + Instantiator::withConstructor()->allowExtraAttributes(['extraPostsBeforeInstantiate', 'extraPostsAfterInstantiate']) ) ->beforeInstantiate(function (array $attributes): array { if (isset($attributes['extraPostsBeforeInstantiate'])) { diff --git a/tests/Fixtures/Factories/PostFactory.php b/tests/Fixtures/Factories/PostFactory.php index 0dc00fa00..e84357898 100644 --- a/tests/Fixtures/Factories/PostFactory.php +++ b/tests/Fixtures/Factories/PostFactory.php @@ -38,7 +38,7 @@ protected function initialize(): static { return $this ->instantiateWith( - (new Instantiator())->allowExtraAttributes(['extraCategoryBeforeInstantiate', 'extraCategoryAfterInstantiate']) + Instantiator::withConstructor()->allowExtraAttributes(['extraCategoryBeforeInstantiate', 'extraCategoryAfterInstantiate']) ) ->beforeInstantiate(function (array $attributes): array { if (isset($attributes['extraCategoryBeforeInstantiate'])) { diff --git a/tests/Functional/FactoryDoctrineCascadeTest.php b/tests/Functional/FactoryDoctrineCascadeTest.php index 39f0d01af..b781f0a2e 100644 --- a/tests/Functional/FactoryDoctrineCascadeTest.php +++ b/tests/Functional/FactoryDoctrineCascadeTest.php @@ -50,7 +50,7 @@ public function many_to_one_relationship(): void ])->instantiateWith(function(array $attributes, string $class): object { $this->assertNull($attributes['brand']->getId()); - return (new Instantiator())($attributes, $class); + return Instantiator::withConstructor()($attributes, $class); })->create(); $this->assertNotNull($product->getBrand()->getId()); @@ -68,7 +68,7 @@ public function one_to_many_relationship(): void ])->instantiateWith(function(array $attributes, string $class): object { // $this->assertNull($attributes['products'][0]->getId()); - return (new Instantiator())($attributes, $class); + return Instantiator::withConstructor()($attributes, $class); })->create(); $this->assertCount(2, $brand->getProducts()); @@ -87,7 +87,7 @@ public function many_to_many_relationship(): void ])->instantiateWith(function(array $attributes, string $class): object { $this->assertNull($attributes['tags'][0]->getId()); - return (new Instantiator())($attributes, $class); + return Instantiator::withConstructor()($attributes, $class); })->create(); $this->assertCount(1, $product->getTags()); @@ -106,7 +106,7 @@ public function many_to_many_reverse_relationship(): void ])->instantiateWith(function(array $attributes, string $class): object { $this->assertNull($attributes['categories'][0]->getId()); - return (new Instantiator())($attributes, $class); + return Instantiator::withConstructor()($attributes, $class); })->create(); $this->assertCount(1, $product->getCategories()); @@ -125,7 +125,7 @@ public function one_to_one_relationship(): void ])->instantiateWith(function(array $attributes, string $class): object { $this->assertNull($attributes['review']->getId()); - return (new Instantiator())($attributes, $class); + return Instantiator::withConstructor()($attributes, $class); })->create(); $this->assertNotNull($product->getReview()->getId()); @@ -143,7 +143,7 @@ public function one_to_one_reverse_relationship(): void ])->instantiateWith(function(array $attributes, string $class): object { $this->assertNull($attributes['review']->getId()); - return (new Instantiator())($attributes, $class); + return Instantiator::withConstructor()($attributes, $class); })->create(); $this->assertNotNull($product->getReview()->getId()); diff --git a/tests/Unit/Bundle/DependencyInjection/ZenstruckFoundryExtensionTest.php b/tests/Unit/Bundle/DependencyInjection/ZenstruckFoundryExtensionTest.php index f892213c9..2b93e2fa3 100644 --- a/tests/Unit/Bundle/DependencyInjection/ZenstruckFoundryExtensionTest.php +++ b/tests/Unit/Bundle/DependencyInjection/ZenstruckFoundryExtensionTest.php @@ -124,9 +124,15 @@ public function custom_instantiator_config(): void 'always_force_properties' => true, ]]); - $this->assertContainerBuilderHasServiceDefinitionWithMethodCall('.zenstruck_foundry.default_instantiator', 'withoutConstructor'); $this->assertContainerBuilderHasServiceDefinitionWithMethodCall('.zenstruck_foundry.default_instantiator', 'allowExtraAttributes'); $this->assertContainerBuilderHasServiceDefinitionWithMethodCall('.zenstruck_foundry.default_instantiator', 'alwaysForceProperties'); + + $instantiator = $this->container->get('.zenstruck_foundry.default_instantiator'); + + // matthiasnoback/symfony-dependency-injection-test cannot assert if a service is created through a factory. + // so, we're checking that private property "Instantiator::$withoutConstructor" was set to true. + $withoutConstructor = \Closure::bind(static function (Instantiator $instantiator){return $instantiator->withoutConstructor;}, null, Instantiator::class)($instantiator); + self::assertTrue($withoutConstructor); } /** diff --git a/tests/Unit/InstantiatorTest.php b/tests/Unit/InstantiatorTest.php index 24c9fd052..f4b559246 100644 --- a/tests/Unit/InstantiatorTest.php +++ b/tests/Unit/InstantiatorTest.php @@ -27,7 +27,7 @@ final class InstantiatorTest extends TestCase */ public function default_instantiate(): void { - $object = (new Instantiator())([ + $object = Instantiator::withConstructor()([ 'propA' => 'A', 'propB' => 'B', 'propC' => 'C', @@ -49,7 +49,7 @@ public function can_use_snake_case_attributes(): void { $this->expectDeprecation('Since zenstruck\foundry 1.5.0: Using a differently cased attribute is deprecated, use the same case as the object property instead.'); - $object = (new Instantiator())([ + $object = Instantiator::withConstructor()([ 'prop_a' => 'A', 'prop_b' => 'B', 'prop_c' => 'C', @@ -71,7 +71,7 @@ public function can_use_kebab_case_attributes(): void { $this->expectDeprecation('Since zenstruck\foundry 1.5.0: Using a differently cased attribute is deprecated, use the same case as the object property instead.'); - $object = (new Instantiator())([ + $object = Instantiator::withConstructor()([ 'prop-a' => 'A', 'prop-b' => 'B', 'prop-c' => 'C', @@ -90,7 +90,7 @@ public function can_use_kebab_case_attributes(): void */ public function can_leave_off_default_constructor_argument(): void { - $object = (new Instantiator())([ + $object = Instantiator::withConstructor()([ 'propB' => 'B', ], InstantiatorDummy::class); @@ -103,7 +103,27 @@ public function can_leave_off_default_constructor_argument(): void */ public function can_instantiate_object_with_private_constructor(): void { - $object = (new Instantiator())([ + $object = Instantiator::withoutConstructor()([ + 'propA' => 'A', + 'propB' => 'B', + 'propC' => 'C', + 'propD' => 'D', + ], PrivateConstructorInstantiatorDummy::class); + + $this->assertSame('A', $object->propA); + $this->assertSame('A', $object->getPropA()); + $this->assertSame('setter B', $object->getPropB()); + $this->assertSame('setter C', $object->getPropC()); + $this->assertSame('setter D', $object->getPropD()); + } + + /** + * @test + * @group legacy + */ + public function can_instantiate_object_with_private_constructor_and_instantiator_configured_without_constructor(): void + { + $object = Instantiator::withConstructor()([ 'propA' => 'A', 'propB' => 'B', 'propC' => 'C', @@ -125,7 +145,7 @@ public function missing_constructor_argument_throws_exception(): void $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Missing constructor argument "propB" for "Zenstruck\Foundry\Tests\Unit\InstantiatorDummy".'); - (new Instantiator())([], InstantiatorDummy::class); + Instantiator::withConstructor()([], InstantiatorDummy::class); } /** @@ -136,7 +156,7 @@ public function extra_attributes_throws_exception(): void $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Cannot set attribute "extra" for object "Zenstruck\Foundry\Tests\Unit\InstantiatorDummy" (not public and no setter).'); - (new Instantiator())([ + Instantiator::withConstructor()([ 'propB' => 'B', 'extra' => 'foo', ], InstantiatorDummy::class); @@ -147,7 +167,7 @@ public function extra_attributes_throws_exception(): void */ public function can_set_attributes_that_should_be_optional(): void { - $object = (new Instantiator())->allowExtraAttributes(['extra'])([ + $object = Instantiator::withConstructor()->allowExtraAttributes(['extra'])([ 'propB' => 'B', 'extra' => 'foo', ], InstantiatorDummy::class); @@ -163,7 +183,7 @@ public function extra_attributes_not_defined_throws_exception(): void $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Cannot set attribute "extra2" for object "Zenstruck\Foundry\Tests\Unit\InstantiatorDummy" (not public and no setter).'); - (new Instantiator())->allowExtraAttributes(['extra1'])([ + Instantiator::withConstructor()->allowExtraAttributes(['extra1'])([ 'propB' => 'B', 'extra1' => 'foo', 'extra2' => 'bar', @@ -178,7 +198,7 @@ public function can_prefix_extra_attribute_key_with_optional_to_avoid_exception( { $this->expectDeprecation('Since zenstruck\foundry 1.5.0: Using "optional:" attribute prefixes is deprecated, use Instantiator::allowExtraAttributes() instead (https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#instantiation).'); - $object = (new Instantiator())([ + $object = Instantiator::withConstructor()([ 'propB' => 'B', 'optional:extra' => 'foo', ], InstantiatorDummy::class); @@ -191,7 +211,7 @@ public function can_prefix_extra_attribute_key_with_optional_to_avoid_exception( */ public function can_always_allow_extra_attributes(): void { - $object = (new Instantiator())->allowExtraAttributes()([ + $object = Instantiator::withConstructor()->allowExtraAttributes()([ 'propB' => 'B', 'extra' => 'foo', ], InstantiatorDummy::class); @@ -204,7 +224,7 @@ public function can_always_allow_extra_attributes(): void */ public function can_disable_constructor(): void { - $object = (new Instantiator())->withoutConstructor()([ + $object = (Instantiator::withoutConstructor())([ 'propA' => 'A', 'propB' => 'B', 'propC' => 'C', @@ -223,7 +243,7 @@ public function can_disable_constructor(): void */ public function can_set_attributes_that_should_be_force_set(): void { - $object = (new Instantiator())->withoutConstructor()->alwaysForceProperties(['propD'])([ + $object = Instantiator::withoutConstructor()->alwaysForceProperties(['propD'])([ 'propB' => 'B', 'propD' => 'D', ], InstantiatorDummy::class); @@ -232,6 +252,26 @@ public function can_set_attributes_that_should_be_force_set(): void $this->assertSame('D', $object->getPropD()); } + /** + * @test + * @group legacy + */ + public function can_disable_constructor_legacy(): void + { + $object = Instantiator::withConstructor()->withoutConstructor()([ + 'propA' => 'A', + 'propB' => 'B', + 'propC' => 'C', + 'propD' => 'D', + ], InstantiatorDummy::class); + + $this->assertSame('A', $object->propA); + $this->assertSame('A', $object->getPropA()); + $this->assertSame('setter B', $object->getPropB()); + $this->assertSame('setter C', $object->getPropC()); + $this->assertSame('setter D', $object->getPropD()); + } + /** * @test * @group legacy @@ -240,7 +280,7 @@ public function prefixing_attribute_key_with_force_sets_the_property_directly(): { $this->expectDeprecation('Since zenstruck\foundry 1.5.0: Using "force:" property prefixes is deprecated, use Instantiator::alwaysForceProperties() instead (https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#instantiation).'); - $object = (new Instantiator())([ + $object = Instantiator::withConstructor()([ 'propA' => 'A', 'propB' => 'B', 'propC' => 'C', @@ -262,7 +302,7 @@ public function prefixing_snake_case_attribute_key_with_force_sets_the_property_ { $this->expectDeprecation('Since zenstruck\foundry 1.5.0: Using "force:" property prefixes is deprecated, use Instantiator::alwaysForceProperties() instead (https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#instantiation).'); - $object = (new Instantiator())([ + $object = Instantiator::withConstructor()([ 'prop_a' => 'A', 'prop_b' => 'B', 'prop_c' => 'C', @@ -284,7 +324,7 @@ public function prefixing_kebab_case_attribute_key_with_force_sets_the_property_ { $this->expectDeprecation('Since zenstruck\foundry 1.5.0: Using "force:" property prefixes is deprecated, use Instantiator::alwaysForceProperties() instead (https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#instantiation).'); - $object = (new Instantiator())([ + $object = Instantiator::withConstructor()([ 'prop-a' => 'A', 'prop-b' => 'B', 'prop-c' => 'C', @@ -308,7 +348,7 @@ public function prefixing_invalid_attribute_key_with_force_throws_exception(): v $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Class "Zenstruck\Foundry\Tests\Unit\InstantiatorDummy" does not have property "extra".'); - (new Instantiator())([ + Instantiator::withConstructor()([ 'propB' => 'B', 'force:extra' => 'foo', ], InstantiatorDummy::class); @@ -388,7 +428,7 @@ public function force_get_throws_exception_for_invalid_property(): void */ public function can_use_always_force_mode(): void { - $object = (new Instantiator())->alwaysForceProperties()([ + $object = Instantiator::withConstructor()->alwaysForceProperties()([ 'propA' => 'A', 'propB' => 'B', 'propC' => 'C', @@ -410,7 +450,7 @@ public function can_use_always_force_mode_allows_snake_case(): void { $this->expectDeprecation('Since zenstruck\foundry 1.5.0: Using a differently cased attribute is deprecated, use the same case as the object property instead.'); - $object = (new Instantiator())->alwaysForceProperties()([ + $object = Instantiator::withConstructor()->alwaysForceProperties()([ 'prop_a' => 'A', 'prop_b' => 'B', 'prop_c' => 'C', @@ -432,7 +472,7 @@ public function can_use_always_force_mode_allows_kebab_case(): void { $this->expectDeprecation('Since zenstruck\foundry 1.5.0: Using a differently cased attribute is deprecated, use the same case as the object property instead.'); - $object = (new Instantiator())->alwaysForceProperties()([ + $object = Instantiator::withConstructor()->alwaysForceProperties()([ 'prop-a' => 'A', 'prop-b' => 'B', 'prop-c' => 'C', @@ -454,7 +494,7 @@ public function always_force_mode_throws_exception_for_extra_attributes(): void $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Class "Zenstruck\Foundry\Tests\Unit\InstantiatorDummy" does not have property "extra".'); - (new Instantiator())->alwaysForceProperties()([ + Instantiator::withConstructor()->alwaysForceProperties()([ 'propB' => 'B', 'extra' => 'foo', ], InstantiatorDummy::class); @@ -468,7 +508,7 @@ public function always_force_mode_allows_optional_attribute_name_prefix(): void { $this->expectDeprecation('Since zenstruck\foundry 1.5.0: Using "optional:" attribute prefixes is deprecated, use Instantiator::allowExtraAttributes() instead (https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#instantiation).'); - $object = (new Instantiator())->alwaysForceProperties()([ + $object = Instantiator::withConstructor()->alwaysForceProperties()([ 'propB' => 'B', 'propD' => 'D', 'optional:extra' => 'foo', @@ -482,7 +522,7 @@ public function always_force_mode_allows_optional_attribute_name_prefix(): void */ public function always_force_mode_with_allow_extra_attributes_mode(): void { - $object = (new Instantiator())->allowExtraAttributes()->alwaysForceProperties()([ + $object = Instantiator::withConstructor()->allowExtraAttributes()->alwaysForceProperties()([ 'propB' => 'B', 'propD' => 'D', 'extra' => 'foo', @@ -496,7 +536,7 @@ public function always_force_mode_with_allow_extra_attributes_mode(): void */ public function always_force_mode_can_set_parent_class_properties(): void { - $object = (new Instantiator())->alwaysForceProperties()([ + $object = Instantiator::withConstructor()->alwaysForceProperties()([ 'propA' => 'A', 'propB' => 'B', 'propC' => 'C', @@ -519,7 +559,7 @@ public function invalid_attribute_type_with_allow_extra_enabled_throws_exception { $this->expectException(\Throwable::class); - (new Instantiator())->allowExtraAttributes()([ + Instantiator::withConstructor()->allowExtraAttributes()([ 'propB' => 'B', 'propF' => 'F', ], InstantiatorDummy::class); @@ -530,7 +570,7 @@ public function invalid_attribute_type_with_allow_extra_enabled_throws_exception */ public function can_set_variadic_constructor_attributes(): void { - $object = (new Instantiator())([ + $object = Instantiator::withConstructor()([ 'propA' => 'A', 'propB' => ['B', 'C', 'D'], ], VariadicInstantiatorDummy::class); @@ -546,7 +586,7 @@ public function missing_variadic_argument_thtrows(): void { $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Missing constructor argument "propB" for "Zenstruck\Foundry\Tests\Unit\VariadicInstantiatorDummy".'); - (new Instantiator())([ + Instantiator::withConstructor()([ 'propA' => 'A', ], VariadicInstantiatorDummy::class); }