Do not add into collection if init has failed (#407)

atk4 · Mar 16, 2024 · 18741e5 · 18741e5
1 parent ef666b4
commit 18741e5
Show file tree

Hide file tree

Showing 10 changed files with 98 additions and 34 deletions.
diff --git a/docs/factory.md b/docs/factory.md
@@ -53,11 +53,11 @@ Using "class" as opposed to initialized object yields many performance gains,
 as initialization of the class may be delayed until it's required. For instance:
 
 ```
-$model->hasMany('Invoices', Invoice::class);
+$model->hasMany('Invoices', ['model' => [Invoice::class]]);
 
 // is faster than
 
-$model->hasMany('Invoices', new Invoice());
+$model->hasMany('Invoices', ['model' => new Invoice()]);
 ```
 
 That is due to the fact that creating instance of "Invoice" class is not required

diff --git a/docs/index.md b/docs/index.md
@@ -220,8 +220,8 @@ class ShoppingBag extends \Atk4\Data\Model
     {
         parent::init();
 
-        $this->hasOne('user_id', new User());
-        $this->hasMany('Items', new Item())
+        $this->hasOne('user_id', ['model' => [User::class]]);
+        $this->hasMany('Items', ['model' => [Item::class]])
             ->addField('total_price', ['aggregate' => 'sum', 'field' => 'price']);
     }
 }

diff --git a/src/CollectionTrait.php b/src/CollectionTrait.php
@@ -18,7 +18,7 @@ trait CollectionTrait
     /**
      * Use this method trait like this:.
      *
-     * function addField($name, $definition)
+     * function addField(string $name, $definition)
      * {
      *     $field = Field::fromSeed($seed);
      *
@@ -62,14 +62,20 @@ protected function _addIntoCollection(string $name, object $item, string $collec
             }
         }
 
+        $this->{$collection}[$name] = $item;
+
         if (TraitUtil::hasInitializerTrait($item)) {
             if (!$item->isInitialized()) {
-                $item->invokeInit();
+                try {
+                    $item->invokeInit();
+                } catch (\Throwable $e) {
+                    unset($this->{$collection}[$name]);
+
+                    throw $e;
+                }
             }
         }
 
-        $this->{$collection}[$name] = $item;
-
         return $item;
     }
 

diff --git a/src/ContainerTrait.php b/src/ContainerTrait.php
@@ -52,7 +52,15 @@ public function add($obj, $args = []): object
 
         if (TraitUtil::hasInitializerTrait($obj)) {
             if (!$obj->isInitialized()) {
-                $obj->invokeInit();
+                try {
+                    $obj->invokeInit();
+                } catch (\Throwable $e) {
+                    if (TraitUtil::hasTrackableTrait($obj)) {
+                        unset($this->elements[$obj->shortName]);
+                    }
+
+                    throw $e;
+                }
             }
         }
 

diff --git a/src/DiContainerTrait.php b/src/DiContainerTrait.php
@@ -89,24 +89,22 @@ public static function assertInstanceOf(object $object)// :static supported by P
     {
         if (!$object instanceof static) {
             throw (new Exception('Object is not an instance of static class'))
-                ->addMoreInfo('static_class', static::class)
-                ->addMoreInfo('object_class', get_class($object));
+                ->addMoreInfo('staticClass', static::class)
+                ->addMoreInfo('objectClass', get_class($object));
         }
 
         return $object;
     }
 
     /**
      * @param array<mixed>|object $seed
-     *
-     * @return array<mixed>|object
      */
-    private static function _fromSeedPrecheck($seed, bool $unsafe)
+    private static function _fromSeedPrecheck($seed, bool $unsafe): void
     {
         if (!is_object($seed)) {
             if (!is_array($seed)) { // @phpstan-ignore-line
                 throw (new Exception('Seed must be an array or an object'))
-                    ->addMoreInfo('seed_type', gettype($seed));
+                    ->addMoreInfo('seed', $seed);
             }
 
             if (!isset($seed[0])) {
@@ -117,12 +115,10 @@ private static function _fromSeedPrecheck($seed, bool $unsafe)
             $cl = $seed[0];
             if (!$unsafe && !is_a($cl, static::class, true)) {
                 throw (new Exception('Seed class is not a subtype of static class'))
-                    ->addMoreInfo('static_class', static::class)
-                    ->addMoreInfo('seed_class', $cl);
+                    ->addMoreInfo('staticClass', static::class)
+                    ->addMoreInfo('seedClass', $cl);
             }
         }
-
-        return $seed;
     }
 
     /**
@@ -138,7 +134,7 @@ private static function _fromSeedPrecheck($seed, bool $unsafe)
      */
     public static function fromSeed($seed = [], $defaults = [])// :static supported by PHP8+
     {
-        $seed = self::_fromSeedPrecheck($seed, false);
+        self::_fromSeedPrecheck($seed, false);
         $object = Factory::factory($seed, $defaults);
 
         return static::assertInstanceOf($object);
@@ -154,7 +150,7 @@ public static function fromSeed($seed = [], $defaults = [])// :static supported
      */
     public static function fromSeedUnsafe($seed = [], $defaults = [])
     {
-        $seed = self::_fromSeedPrecheck($seed, true);
+        self::_fromSeedPrecheck($seed, true);
         $object = Factory::factory($seed, $defaults);
 
         return $object; // @phpstan-ignore-line

diff --git a/src/HookTrait.php b/src/HookTrait.php
@@ -148,14 +148,10 @@ public function onHookShort(string $spot, \Closure $fx, array $args = [], int $p
         $fxRefl = new \ReflectionFunction($fx);
         $fxScopeClassRefl = $fxRefl->getClosureScopeClass();
         $fxThis = $fxRefl->getClosureThis();
-        if ($fxScopeClassRefl === null) {
-            $fxLong = static function ($ignore, &...$args) use ($fx) {
-                return $fx(...$args);
-            };
-        } elseif ($fxThis === null) {
+        if ($fxThis === null) {
             $fxLong = \Closure::bind(static function ($ignore, &...$args) use ($fx) {
                 return $fx(...$args);
-            }, null, $fxScopeClassRefl->getName());
+            }, null, $fxScopeClassRefl !== null ? $fxScopeClassRefl->getName() : null);
         } else {
             $fxLong = $this->_unbindHookFxIfBoundToThis($fx, true);
             if ($fxLong === $fx) {
@@ -302,6 +298,7 @@ public function removeHook(string $spot, int $priority = null, bool $priorityIsI
     public function hook(string $spot, array $args = [], HookBreaker &$brokenBy = null)
     {
         $brokenBy = null;
+
         $this->_rebindHooksIfCloned();
 
         $return = [];
@@ -323,7 +320,7 @@ public function hook(string $spot, array $args = [], HookBreaker &$brokenBy = nu
             }
         }
 
-        return $return; // @phpstan-ignore-line https://github.com/phpstan/phpstan/issues/10684
+        return $return;
     }
 
     /**

diff --git a/src/StaticAddToTrait.php b/src/StaticAddToTrait.php
@@ -68,7 +68,7 @@ public static function addToWithCl(object $parent, $seed = [], array $addArgs =
      *
      * @return static
      */
-    public static function addToWithClUnsafe(object $parent, $seed = [], array $addArgs = [], bool $skipAdd = false)// :self is too strict with unsafe behaviour
+    public static function addToWithClUnsafe(object $parent, $seed = [], array $addArgs = [], bool $skipAdd = false)
     {
         $object = static::fromSeedUnsafe($seed);
 

diff --git a/tests/CollectionMock.php b/tests/CollectionMock.php
@@ -23,9 +23,6 @@ public function addField(string $name, $seed = null): FieldMock
 
         $field = Factory::factory($seed);
 
-        $shortNameProp = $field instanceof FieldMockCustom ? 'shortName' : 'name';
-        Factory::factory($seed, [$shortNameProp => $name]);
-
         return $this->_addIntoCollection($name, $field, 'fields');
     }
 

diff --git a/tests/CollectionTraitTest.php b/tests/CollectionTraitTest.php
@@ -151,19 +151,42 @@ public function testException5(): void
         \Closure::bind(static fn () => $m->_removeFromCollection('dont_exist', 'fields'), null, CollectionMock::class)(); // does not exist
     }
 
-    public function testException6(): void
+    public function testParentInitNotCalledException(): void
     {
         $m = new CollectionMock();
 
         $this->expectException(Exception::class);
         $this->expectExceptionMessage('Object was not initialized');
-        $m->addField('test', new class() extends FieldMock {
+        $m->addField('foo', new class() extends FieldMock {
             use InitializerTrait;
 
             protected function init(): void {}
         });
     }
 
+    public function testExceptionInInitMustNotAdd(): void
+    {
+        $m = new CollectionMock();
+
+        $e = null;
+        try {
+            $m->addField('foo', new class() extends FieldMock {
+                use InitializerTrait;
+
+                protected function init(): void
+                {
+                    throw new Exception('from init');
+                }
+            });
+        } catch (Exception $e) {
+        }
+        self::assertSame('from init', $e->getMessage());
+
+        self::assertFalse($m->hasField('foo'));
+        $m->addField('foo', new FieldMock());
+        self::assertTrue($m->hasField('foo'));
+    }
+
     public function testClone(): void
     {
         $m = new CollectionMock();

diff --git a/tests/ContainerTraitTest.php b/tests/ContainerTraitTest.php
@@ -8,6 +8,7 @@
 use Atk4\Core\ContainerTrait;
 use Atk4\Core\DiContainerTrait;
 use Atk4\Core\Exception;
+use Atk4\Core\InitializerTrait;
 use Atk4\Core\NameTrait;
 use Atk4\Core\Phpunit\TestCase;
 use Atk4\Core\TrackableTrait;
@@ -275,6 +276,42 @@ public function testException5(): void
         $this->expectExceptionMessage('Child element not found');
         $m->removeElement('dont_exist');
     }
+
+    public function testParentInitNotCalledException(): void
+    {
+        $m = new ContainerMock();
+
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage('Object was not initialized');
+        $m->add(new class() extends TrackableMock {
+            use InitializerTrait;
+
+            protected function init(): void {}
+        });
+    }
+
+    public function testExceptionInInitMustNotAdd(): void
+    {
+        $m = new ContainerMock();
+
+        $e = null;
+        try {
+            $m->add(new class() extends TrackableMock {
+                use InitializerTrait;
+
+                protected function init(): void
+                {
+                    throw new Exception('from init');
+                }
+            }, 'foo');
+        } catch (Exception $e) {
+        }
+        self::assertSame('from init', $e->getMessage());
+
+        self::assertFalse($m->hasElement('foo'));
+        $m->add(new TrackableMock(), 'foo');
+        self::assertTrue($m->hasElement('foo'));
+    }
 }
 
 class TrackableMock