From bc805783b81d2cdb4d1ef46d65e92d1655d4e462 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 10 Nov 2022 09:21:04 +0100 Subject: [PATCH 01/18] Clone attributes --- src/Attributes.php | 7 +++++++ src/BaseHtmlElement.php | 9 +++++++++ 2 files changed, 16 insertions(+) diff --git a/src/Attributes.php b/src/Attributes.php index ae15ef88..66295e13 100644 --- a/src/Attributes.php +++ b/src/Attributes.php @@ -518,4 +518,11 @@ public function getIterator(): Traversable { return new ArrayIterator($this->attributes); } + + public function __clone() + { + foreach ($this->attributes as &$attribute) { + $attribute = clone $attribute; + } + } } diff --git a/src/BaseHtmlElement.php b/src/BaseHtmlElement.php index da8348d0..41b6e4b5 100644 --- a/src/BaseHtmlElement.php +++ b/src/BaseHtmlElement.php @@ -352,4 +352,13 @@ public function renderUnwrapped() $tag ); } + + public function __clone() + { + parent::__clone(); + + if ($this->attributes !== null) { + $this->attributes = clone $this->attributes; + } + } } From 4ae43eb99fef538bb66b5584b71bfcd98e8b4cd5 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Mon, 28 Nov 2022 13:38:47 +0100 Subject: [PATCH 02/18] Fix Attributes::registerAttributeCallback() signature --- src/Attributes.php | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/Attributes.php b/src/Attributes.php index 66295e13..ffe22eea 100644 --- a/src/Attributes.php +++ b/src/Attributes.php @@ -365,29 +365,19 @@ public function setPrefix($prefix) /** * Register callback for an attribute * - * @param string $name Name of the attribute to register the callback for - * @param callable $callback Callback to call when retrieving the attribute - * @param callable $setterCallback Callback to call when setting the attribute + * @param string $name Name of the attribute to register the callback for + * @param ?callable $callback Callback to call when retrieving the attribute + * @param ?callable $setterCallback Callback to call when setting the attribute * * @return $this - * - * @throws InvalidArgumentException If $callback is not callable or if $setterCallback is set and not callable */ - public function registerAttributeCallback($name, $callback, $setterCallback = null) + public function registerAttributeCallback(string $name, ?callable $callback, ?callable $setterCallback = null): self { if ($callback !== null) { - if (! is_callable($callback)) { - throw new InvalidArgumentException(__METHOD__ . ' expects a callable callback'); - } - $this->callbacks[$name] = $callback; } if ($setterCallback !== null) { - if (! is_callable($setterCallback)) { - throw new InvalidArgumentException(__METHOD__ . ' expects a callable setterCallback'); - } - $this->setterCallbacks[$name] = $setterCallback; } From f5171e7fde9de5ea7ca2757e65c47c5776846f25 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 27 Jan 2023 16:37:03 +0100 Subject: [PATCH 03/18] AttributesTest: Test deep cloning --- tests/AttributesTest.php | 24 +++++++++++ tests/RebindAttributeCallbacksTest.php | 56 ++++++++++++++++++++++++++ 2 files changed, 80 insertions(+) create mode 100644 tests/RebindAttributeCallbacksTest.php diff --git a/tests/AttributesTest.php b/tests/AttributesTest.php index 51d2c4a2..e32f5826 100644 --- a/tests/AttributesTest.php +++ b/tests/AttributesTest.php @@ -113,4 +113,28 @@ function ($v) use (&$value) { $this->assertEquals(' foo="bar rab" bar="foo"', $attributes->render()); } + + public function testAttributesAreDeepCloned() + { + $attributes = Attributes::create(['class' => 'one']); + + $clone = clone $attributes; + $clone->add('class', 'two'); + + $this->assertNotSame( + $attributes->get('class'), + $clone->get('class'), + 'Attribute instances are not cloned' + ); + $this->assertSame( + 'one', + $attributes->get('class')->getValue(), + 'Attribute instances are not cloned correctly' + ); + $this->assertSame( + ['one', 'two'], + $clone->get('class')->getValue(), + 'Attribute instances are not cloned correctly' + ); + } } diff --git a/tests/RebindAttributeCallbacksTest.php b/tests/RebindAttributeCallbacksTest.php new file mode 100644 index 00000000..64e9317f --- /dev/null +++ b/tests/RebindAttributeCallbacksTest.php @@ -0,0 +1,56 @@ +value = $value; + } + + public function getValue() + { + return $this->value; + } + + protected function registerAttributeCallbacks(Attributes $attributes) + { + $attributes->registerAttributeCallback('value', [$this, 'getValue'], [$this, 'setValue']); + $attributes->registerAttributeCallback('data-ngos', function () { + return $this->noGetterOrSetter; + }, function ($value) { + $this->noGetterOrSetter = $value; + }); + } + }; + + $element->setAttribute('value', 'foo'); + + $clone = clone $element; + + $clone->setAttribute('value', 'bar') + ->setAttribute('data-ngos', true); + + $this->assertSame( + ' value="foo"', + $element->getAttributes()->render(), + 'Attribute callbacks are not rebound to their new owner' + ); + $this->assertSame( + ' value="bar" data-ngos', + $clone->getAttributes()->render(), + 'Attribute callbacks are not rebound to their new owner' + ); + } +} From cfb7c390207ae3cac8cd5cd7fc4e60e0f95ebf67 Mon Sep 17 00:00:00 2001 From: Timm Ortloff Date: Thu, 2 Feb 2023 14:58:10 +0100 Subject: [PATCH 04/18] Test rebinding attribute callbacks after clone --- tests/AttributesTest.php | 16 +++ tests/Lib/ElementWithAttributeCallbacks.php | 34 +++++ tests/RebindAttributeCallbacksTest.php | 131 ++++++++++++++++++++ 3 files changed, 181 insertions(+) create mode 100644 tests/Lib/ElementWithAttributeCallbacks.php diff --git a/tests/AttributesTest.php b/tests/AttributesTest.php index e32f5826..8017c36d 100644 --- a/tests/AttributesTest.php +++ b/tests/AttributesTest.php @@ -2,6 +2,7 @@ namespace ipl\Tests\Html; +use ipl\Html\Attribute; use ipl\Html\Attributes; use ipl\Html\BaseHtmlElement; @@ -114,6 +115,21 @@ function ($v) use (&$value) { $this->assertEquals(' foo="bar rab" bar="foo"', $attributes->render()); } + public function testCloningAttributes(): void + { + $original = Attributes::create([Attribute::create('class', 'class01')]); + + $clone = clone $original; + foreach ($clone->getAttributes() as $attribute) { + if ($attribute->getName() === 'class') { + $attribute->setValue('class02'); + } + } + + $this->assertSame($original->get('class')->getValue(), 'class01'); + $this->assertSame($clone->get('class')->getValue(), 'class02'); + } + public function testAttributesAreDeepCloned() { $attributes = Attributes::create(['class' => 'one']); diff --git a/tests/Lib/ElementWithAttributeCallbacks.php b/tests/Lib/ElementWithAttributeCallbacks.php new file mode 100644 index 00000000..b3f7e4de --- /dev/null +++ b/tests/Lib/ElementWithAttributeCallbacks.php @@ -0,0 +1,34 @@ +registerAttributeCallback('test-instance-scope-noop-inline', function () { + return 'inline'; + }); + $attributes->registerAttributeCallback('test-instance-noop-attribute', [$this, 'staticMethod']); + $attributes->registerAttributeCallback( + 'test-closure-static-scope-noop', + Closure::fromCallable(self::class . '::staticMethod') + ); + + $attributes->registerAttributeCallback( + 'test-closure-instance-scope-noop', + Closure::fromCallable([$this, 'staticMethod']) + ); + } +} diff --git a/tests/RebindAttributeCallbacksTest.php b/tests/RebindAttributeCallbacksTest.php index 64e9317f..0a0688c2 100644 --- a/tests/RebindAttributeCallbacksTest.php +++ b/tests/RebindAttributeCallbacksTest.php @@ -2,8 +2,12 @@ namespace ipl\Tests\Html; +use Closure; use ipl\Html\Attributes; use ipl\Html\BaseHtmlElement; +use ipl\Tests\Html\Lib\ElementWithAttributeCallbacks; +use ReflectionFunction; +use ReflectionProperty; class RebindAttributeCallbacksTest extends TestCase { @@ -53,4 +57,131 @@ protected function registerAttributeCallbacks(Attributes $attributes) 'Attribute callbacks are not rebound to their new owner' ); } + + public function testHtmlOutput(): void + { + $original = new ElementWithAttributeCallbacks(); + $original->getAttributes()->set('class', 'original_class'); + + $firstClone = clone $original; + $firstClone->getAttributes()->set('class', 'first_clone_class'); + + $secondClone = clone $firstClone; + $secondClone->getAttributes()->set('class', 'second_clone_class'); + + $originalHtml = <<<'HTML' +

+

+HTML; + + $firstCloneHtml = <<<'HTML' +

+

+HTML; + + + $secondCloneHtml = <<<'HTML' +

+

+HTML; + + $this->assertHtml($originalHtml, $original); + $this->assertHtml($firstCloneHtml, $firstClone); + $this->assertHtml($secondCloneHtml, $secondClone); + } + + public function testElementCallbacksCloning(): void + { + $element = new ElementWithAttributeCallbacks(); + $element->getAttributes(); + + $clone = clone $element; + + $this->assertCallbacksFor($element); + $this->assertCallbacksFor($clone); + } + + protected function getCallbackThis(callable $callback): ?object + { + if (! $callback instanceof Closure) { + if (is_array($callback) && ! is_string($callback[0])) { + return $callback[0]; + } else { + return null; + } + } + + return (new ReflectionFunction($callback)) + ->getClosureThis(); + } + + protected function isCallbackGlobalOrStatic(callable $callback): bool + { + if (! $callback instanceof Closure) { + if (is_array($callback) && ! is_string($callback[0])) { + return false; + } + } else { + $closureThis = (new ReflectionFunction($callback)) + ->getClosureThis(); + + if ($closureThis) { + return false; + } + } + + return true; + } + + protected function getAttributeCallback(Attributes $attributes, string $name): callable + { + $callbacksProperty = new ReflectionProperty(get_class($attributes), 'callbacks'); + $callbacksProperty->setAccessible(true); + $callbacks = $callbacksProperty->getValue($attributes); + + return $callbacks[$name]; + } + + protected function assertCallbacksFor(ElementWithAttributeCallbacks $element) + { + $this->assertCallbackBelongsTo($element->getAttributes(), 'test-instance-scope-noop-inline', $element); + $this->assertCallbackBelongsTo( + $element->getAttributes(), + 'test-instance-noop-attribute', + $element + ); + $this->assertGlobalOrStaticCallback( + $element->getAttributes(), + 'test-closure-static-scope-noop' + ); + $this->assertGlobalOrStaticCallback( + $element->getAttributes(), + 'test-closure-instance-scope-noop' + ); + } + + protected function assertGlobalOrStaticCallback(Attributes $attributes, string $callbackName) + { + $callback = $this->getAttributeCallback($attributes, $callbackName); + $this->assertTrue($this->isCallbackGlobalOrStatic($callback)); + } + + protected function assertCallbackBelongsTo(Attributes $attributes, string $callbackName, object $owner) + { + $callback = $this->getAttributeCallback($attributes, $callbackName); + $callbackThis = $this->getCallbackThis($callback); + $this->assertSame($callbackThis, $owner); + } } From 42bbe4a8b5f8fa9e4efb1b9b7ed81400fd769d31 Mon Sep 17 00:00:00 2001 From: Timm Ortloff Date: Thu, 2 Feb 2023 14:59:38 +0100 Subject: [PATCH 05/18] Rebind attribute callbacks after clone After cloning elements, each own attribute callback must be rebound so that the clones continue to work together. --- src/Attributes.php | 35 +++++++++++++++++++++++++++++++++++ src/BaseHtmlElement.php | 10 ++++++++++ 2 files changed, 45 insertions(+) diff --git a/src/Attributes.php b/src/Attributes.php index ffe22eea..5b2da868 100644 --- a/src/Attributes.php +++ b/src/Attributes.php @@ -4,8 +4,10 @@ use ArrayAccess; use ArrayIterator; +use Closure; use InvalidArgumentException; use IteratorAggregate; +use ReflectionFunction; use Traversable; use function ipl\Stdlib\get_php_type; @@ -509,10 +511,43 @@ public function getIterator(): Traversable return new ArrayIterator($this->attributes); } + public function rebindAttributeCallbacks(int $thisObjectId, object $newThis): void + { + $this->rebindCallbacksInPlace($this->callbacks, $thisObjectId, $newThis); + $this->rebindCallbacksInPlace($this->setterCallbacks, $thisObjectId, $newThis); + } + public function __clone() { foreach ($this->attributes as &$attribute) { $attribute = clone $attribute; } } + + private function rebindCallbacksInPlace(array &$callbacks, int $thisObjectId, object $newThis): void + { + foreach ($callbacks as &$callback) { + if (! $callback instanceof Closure) { + if (is_array($callback) && ! is_string($callback[0])) { + if (spl_object_id($callback[0]) === $thisObjectId) { + $callback[0] = $newThis; + } + } + + continue; + } + + $closureThis = (new ReflectionFunction($callback)) + ->getClosureThis(); + + // Closure is most likely static + if ($closureThis === null) { + continue; + } + + if (spl_object_id($closureThis) === $thisObjectId) { + $callback = $callback->bindTo($newThis); + } + } + } } diff --git a/src/BaseHtmlElement.php b/src/BaseHtmlElement.php index 41b6e4b5..8423db42 100644 --- a/src/BaseHtmlElement.php +++ b/src/BaseHtmlElement.php @@ -75,6 +75,8 @@ abstract class BaseHtmlElement extends HtmlDocument /** @var string Tag of element. Set this property in order to provide the element's tag when extending this class */ protected $tag; + private $objectId; + /** * Get the attributes of the element * @@ -83,6 +85,8 @@ abstract class BaseHtmlElement extends HtmlDocument public function getAttributes() { if ($this->attributes === null) { + $this->objectId = spl_object_id($this); + $default = $this->getDefaultAttributes(); if (empty($default)) { $this->attributes = new Attributes(); @@ -105,6 +109,8 @@ public function getAttributes() */ public function setAttributes($attributes) { + $this->objectId = spl_object_id($this); + $this->attributes = Attributes::wantAttributes($attributes); $this->attributeCallbacksRegistered = false; @@ -359,6 +365,10 @@ public function __clone() if ($this->attributes !== null) { $this->attributes = clone $this->attributes; + + $this->attributes->rebindAttributeCallbacks($this->objectId, $this); + + $this->objectId = spl_object_id($this); } } } From badc8c4966a8de647631c30a7d315e1c8527d246 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Feb 2023 22:41:29 +0100 Subject: [PATCH 06/18] Document attribute callback rebinding --- src/Attributes.php | 19 +++++++++++++++++++ src/BaseHtmlElement.php | 3 +++ 2 files changed, 22 insertions(+) diff --git a/src/Attributes.php b/src/Attributes.php index 5b2da868..c672e658 100644 --- a/src/Attributes.php +++ b/src/Attributes.php @@ -511,6 +511,16 @@ public function getIterator(): Traversable return new ArrayIterator($this->attributes); } + /** + * Rebind all attribute callbacks whose `$this` object IDs match the given ID to the object specified in `$newThis` + * + * If both this attributes and objects that registered attribute callbacks are cloned either + * explicitly or implicitly, the callbacks must be rebound if the clones are to continue to work together. + * This method is called automatically for classes extending {@link BaseHtmlElement}. + * + * @param int $thisObjectId {@link spl_object_id() Object ID} for matching the callbacks currently bound objects + * @param object $newThis The object to which the matching callbacks should be bound + */ public function rebindAttributeCallbacks(int $thisObjectId, object $newThis): void { $this->rebindCallbacksInPlace($this->callbacks, $thisObjectId, $newThis); @@ -524,6 +534,15 @@ public function __clone() } } + /** + * Loops over all `$callbacks` and binds them to `$newThis` if + * `$oldThisId` matches the {@link spl_object_id() object ID} of the currently bound object. + * The callbacks are modified directly at the `$callbacks` reference. + * + * @param callable[] $callbacks + * @param int $thisObjectId {@link spl_object_id() Object ID} for matching the callbacks currently bound objects + * @param object $newThis The object to which the matching callbacks should be bound + */ private function rebindCallbacksInPlace(array &$callbacks, int $thisObjectId, object $newThis): void { foreach ($callbacks as &$callback) { diff --git a/src/BaseHtmlElement.php b/src/BaseHtmlElement.php index 8423db42..3c3877e0 100644 --- a/src/BaseHtmlElement.php +++ b/src/BaseHtmlElement.php @@ -3,6 +3,7 @@ namespace ipl\Html; use RuntimeException; +use SplObjectStorage; /** * Base class for HTML elements @@ -75,6 +76,7 @@ abstract class BaseHtmlElement extends HtmlDocument /** @var string Tag of element. Set this property in order to provide the element's tag when extending this class */ protected $tag; + /** @var int This {@link spl_object_id() object ID} which is used after cloning to find and rebind own callbacks */ private $objectId; /** @@ -366,6 +368,7 @@ public function __clone() if ($this->attributes !== null) { $this->attributes = clone $this->attributes; + // $this->objectId is the ID of the object before cloning, $this is the newly cloned object. $this->attributes->rebindAttributeCallbacks($this->objectId, $this); $this->objectId = spl_object_id($this); From 709e8088cead078750256f9ba6dc6bc42e83ae17 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Feb 2023 22:43:15 +0100 Subject: [PATCH 07/18] Introduce HtmlDocument::initAssemble() We already have subclasses that override ensureAssembled() to execute code before assemble() is called. From now on, initAssemble() should be used for such purposes. initAssemble() is also introduced with the idea that it will be used to set the object ID used for the rebinding callbacks in BaseHtmlElement. --- src/HtmlDocument.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/HtmlDocument.php b/src/HtmlDocument.php index 7e6b404e..f6c56f5b 100644 --- a/src/HtmlDocument.php +++ b/src/HtmlDocument.php @@ -291,6 +291,7 @@ public function ensureAssembled() { if (! $this->hasBeenAssembled) { $this->hasBeenAssembled = true; + $this->initAssemble(); $this->assemble(); } @@ -380,6 +381,13 @@ protected function assemble() { } + /** + * Method called before the element is {@link assemble() assembled} via {@link ensureAssembled()} + */ + protected function initAssemble(): void + { + } + /** * Render the document to HTML respecting the set wrapper * From 370a72345280f7202ce211c080b27390e86b3c34 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Feb 2023 22:43:26 +0100 Subject: [PATCH 08/18] Refine PHPDoc of HtmlDocument::ensureAssembled() --- src/HtmlDocument.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/HtmlDocument.php b/src/HtmlDocument.php index f6c56f5b..f6ca1bba 100644 --- a/src/HtmlDocument.php +++ b/src/HtmlDocument.php @@ -283,7 +283,9 @@ public function remove(ValidHtml $html) } /** - * Ensure that the document has been assembled + * Ensure that the document is assembled + * + * Does nothing if the document is already assembled. * * @return $this */ From 41ef9ef5f4ab1333ef3b6bad2619c480d3501bc7 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Feb 2023 22:43:54 +0100 Subject: [PATCH 09/18] Also set object ID (to rebind callbacks) on assemble Currently the object ID is only set if the element has attributes. This made sense since callbacks only need to be rebound after cloning the element if attributes are set. However, with upcoming changes we will also rebind callbacks of all content elements so that the object id needs to be available in cases where the element has no attributes but content. --- src/BaseHtmlElement.php | 46 ++++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/src/BaseHtmlElement.php b/src/BaseHtmlElement.php index 3c3877e0..9b479b62 100644 --- a/src/BaseHtmlElement.php +++ b/src/BaseHtmlElement.php @@ -2,6 +2,7 @@ namespace ipl\Html; +use LogicException; use RuntimeException; use SplObjectStorage; @@ -87,8 +88,6 @@ abstract class BaseHtmlElement extends HtmlDocument public function getAttributes() { if ($this->attributes === null) { - $this->objectId = spl_object_id($this); - $default = $this->getDefaultAttributes(); if (empty($default)) { $this->attributes = new Attributes(); @@ -111,8 +110,6 @@ public function getAttributes() */ public function setAttributes($attributes) { - $this->objectId = spl_object_id($this); - $this->attributes = Attributes::wantAttributes($attributes); $this->attributeCallbacksRegistered = false; @@ -248,6 +245,8 @@ public function ensureAttributeCallbacksRegistered() if (! $this->attributeCallbacksRegistered) { $this->attributeCallbacksRegistered = true; $this->registerAttributeCallbacks($this->attributes); + + $this->ensureObjectId(); } return $this; @@ -288,6 +287,39 @@ public function wrap(HtmlDocument $document) return $this; } + /** + * Ensure that $this {@link spl_object_id() object ID} is set + * + * @param bool $override Whether the currently set object ID should be overridden + */ + protected function ensureObjectId(bool $override = false): void + { + if ($this->objectId === null || $override) { + $this->objectId = spl_object_id($this); + } + } + + protected function initAssemble(): void + { + $this->ensureObjectId(); + } + + /** + * Get the currently set {@link spl_object_id() object ID} + * + * @return int + * + * @throws LogicException If the object ID has not been set yet + */ + protected function objectId(): int + { + if ($this->objectId === null) { + throw new LogicException('Cannot access object ID because it has not been set yet'); + } + + return $this->objectId; + } + /** * Internal method for accessing the tag * @@ -368,10 +400,10 @@ public function __clone() if ($this->attributes !== null) { $this->attributes = clone $this->attributes; - // $this->objectId is the ID of the object before cloning, $this is the newly cloned object. - $this->attributes->rebindAttributeCallbacks($this->objectId, $this); + // $this->objectId() returns the ID of the object before cloning, $this is the newly cloned object. + $this->attributes->rebindAttributeCallbacks($this->objectId(), $this); - $this->objectId = spl_object_id($this); + $this->ensureObjectId(true); } } } From 20d61a9e5b3f0eb9cc288e798a4cde212960fe48 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Feb 2023 22:44:26 +0100 Subject: [PATCH 10/18] HtmlDocument: Deep copy content in one pass instead of cloning recursively Instead of cloning the content of each content element individually, the entire content is cloned once, also preserving the object graph. --- src/HtmlDocument.php | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/src/HtmlDocument.php b/src/HtmlDocument.php index f6ca1bba..452a53bf 100644 --- a/src/HtmlDocument.php +++ b/src/HtmlDocument.php @@ -7,6 +7,7 @@ use InvalidArgumentException; use ipl\Html\Contract\Wrappable; use RuntimeException; +use SplObjectStorage; /** * HTML document @@ -36,6 +37,9 @@ class HtmlDocument implements Countable, Wrappable /** @var array */ private $contentIndex = []; + /** @var bool Whether {@link static::__clone() clone} has been called internally for this element */ + private $implicitlyCopied = false; + /** * Set the element to wrap * @@ -350,10 +354,40 @@ public function renderUnwrapped() public function __clone() { - foreach ($this->content as $key => $element) { - $this->content[$key] = clone $element; + if ($this->implicitlyCopied) { + // Reset bool so that the clone can be cloned explicitly. + $this->implicitlyCopied = false; + + return; } + // Instead of each content element cloning their content one by one, the entire content is cloned once. + $copies = new SplObjectStorage(); + $deepCopy = function (array &$content) use (&$deepCopy, $copies): void { + foreach ($content as $key => $element) { + if (isset($copies[$element])) { + // Preserve object graph. + $copy = $copies[$element]; + } else { + if ($element instanceof self) { + $element->implicitlyCopied = true; + $copy = clone $element; + $deepCopy($copy->content); + // Reset bool so that the element can be cloned explicitly. + $element->implicitlyCopied = false; + $copy->reIndexContent(); + } else { + $copy = clone $element; + } + + $copies[$element] = $copy; + } + + $content[$key] = $copy; + } + }; + $deepCopy($this->content); + $this->reIndexContent(); } From f4080b36154cea2487313d45ca4e3187219142b7 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Feb 2023 22:53:02 +0100 Subject: [PATCH 11/18] HtmlDocument: Test deep copy --- tests/HtmlDocumentTest.php | 51 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/HtmlDocumentTest.php b/tests/HtmlDocumentTest.php index ca1e82c9..56031845 100644 --- a/tests/HtmlDocumentTest.php +++ b/tests/HtmlDocumentTest.php @@ -2,13 +2,17 @@ namespace ipl\Tests\Html; +use ipl\Html\Attributes; use ipl\Html\BaseHtmlElement; use ipl\Html\Html as h; use ipl\Html\HtmlDocument; +use ipl\Html\HtmlElement; +use ipl\Html\Text; use ipl\Tests\Html\TestDummy\AddsContentDuringAssemble; use ipl\Tests\Html\TestDummy\AddsWrapperDuringAssemble; use ipl\Tests\Html\TestDummy\IterableElement; use ipl\Tests\Html\TestDummy\ObjectThatCanBeCastedToString; +use ReflectionProperty; use RuntimeException; class HtmlDocumentTest extends TestCase @@ -421,4 +425,51 @@ public function testIsEmptyRespectsContentAddedInAssemble() { $this->assertFalse((new AddsContentDuringAssemble())->isEmpty()); } + + public function testDeepCopy() + { + $content = new Text('content'); + $li1 = new HtmlElement('li', Attributes::create(['class' => 'li1']), $content); + $li2 = new HtmlElement('li', Attributes::create(['class' => 'li2']), $content); + $ul = new HtmlElement('ul', Attributes::create(['class' => 'ul']), $li1, $li2); + $div = new HtmlElement('div', Attributes::create(['class' => 'div']), $ul); + + $divClone = clone $div; + /** @var HtmlElement $ulClone */ + list($ulClone) = $divClone->getContent(); + /** @var HtmlElement $li1Clone */ + /** @var HtmlElement $li2Clone */ + list($li1Clone, $li2Clone) = $ulClone->getContent(); + /** @var $li1CloneContent Text */ + list($li1CloneContent) = $li1Clone->getContent(); + /** @var $li2CloneContent Text */ + list($li2CloneContent) = $li2Clone->getContent(); + + // Object graph must be preserved. + $this->assertSame($li1CloneContent, $li2CloneContent); + // It is sufficient to test $li1CloneContent as it is the same as $li2CloneContent. + $this->assertNotSame($li1CloneContent, $content); + $this->assertNotSame($li1Clone, $li1); + $this->assertNotSame($li2Clone, $li2); + $this->assertNotSame($ulClone, $ul); + $this->assertNotSame($divClone, $div); + + $resetPropertiesExpectedToBeNotEqual = function (BaseHtmlElement ...$elements): void { + $contentIndex = new ReflectionProperty(HtmlDocument::class, 'contentIndex'); + $contentIndex->setAccessible(true); + $refId = new ReflectionProperty(BaseHtmlElement::class, 'objectId'); + $refId->setAccessible(true); + foreach ($elements as $element) { + $contentIndex->setValue($element, null); + $refId->setValue($element, null); + } + }; + $resetPropertiesExpectedToBeNotEqual($li1, $li1Clone, $li2, $li2Clone, $ul, $ulClone, $div, $divClone); + + $this->assertEquals($divClone, $div); + $this->assertEquals($ulClone, $ul); + $this->assertEquals($li2Clone, $li2); + $this->assertEquals($li1Clone, $li1); + $this->assertEquals($li1CloneContent, $content); + } } From 7897a96ba309a06e7baac8c2d3cc8ccda80a62c0 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Feb 2023 22:53:13 +0100 Subject: [PATCH 12/18] HtmlDocument: Provide cloned content in subclasses during __clone() Subclasses of HtmlDocument that need to adjust the cloned content, for example to rebind callbacks, can now use a promise resolved with an SplObjectStorage that provides each original and its cloned element. --- composer.json | 1 + src/HtmlDocument.php | 69 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/composer.json b/composer.json index 31f2bf37..acd579d0 100644 --- a/composer.json +++ b/composer.json @@ -14,6 +14,7 @@ "ipl/stdlib": ">=0.12.0", "ipl/validator": "dev-master", "psr/http-message": "~1.0", + "react/promise": "^2", "guzzlehttp/psr7": "^1" }, "autoload": { diff --git a/src/HtmlDocument.php b/src/HtmlDocument.php index 452a53bf..a9673608 100644 --- a/src/HtmlDocument.php +++ b/src/HtmlDocument.php @@ -6,6 +6,8 @@ use Exception; use InvalidArgumentException; use ipl\Html\Contract\Wrappable; +use React\Promise\Deferred; +use React\Promise\PromiseInterface; use RuntimeException; use SplObjectStorage; @@ -37,6 +39,9 @@ class HtmlDocument implements Countable, Wrappable /** @var array */ private $contentIndex = []; + /** @var Deferred {@link static::copy() Unit of work} that is completed after this content has been cloned */ + private $copy; + /** @var bool Whether {@link static::__clone() clone} has been called internally for this element */ private $implicitlyCopied = false; @@ -363,7 +368,7 @@ public function __clone() // Instead of each content element cloning their content one by one, the entire content is cloned once. $copies = new SplObjectStorage(); - $deepCopy = function (array &$content) use (&$deepCopy, $copies): void { + $deepCopy = function (array &$content, SplObjectStorage $pass) use (&$deepCopy, $copies): SplObjectStorage { foreach ($content as $key => $element) { if (isset($copies[$element])) { // Preserve object graph. @@ -372,21 +377,35 @@ public function __clone() if ($element instanceof self) { $element->implicitlyCopied = true; $copy = clone $element; - $deepCopy($copy->content); + $pass->addAll($deepCopy($copy->content, new SplObjectStorage())); // Reset bool so that the element can be cloned explicitly. $element->implicitlyCopied = false; $copy->reIndexContent(); + if ($copy->copy !== null) { + /** copy can be null if {@link copy()} was not called. */ + $copy->copy->resolve($pass); + $copy->copy = null; + } } else { $copy = clone $element; } + $pass[$element] = $copy; $copies[$element] = $copy; } $content[$key] = $copy; } + + return $pass; }; - $deepCopy($this->content); + $deepCopy($this->content, new SplObjectStorage()); + + if ($this->copy !== null) { + /** copy can be null if {@link copy()} was not called. */ + $this->copy->resolve($copies); + $this->copy = null; + } $this->reIndexContent(); } @@ -424,6 +443,50 @@ protected function initAssemble(): void { } + /** + * Get the promise that is resolved after cloning the content + * + * The promise is to be used in subclasses that override `__clone()` and + * need to process the cloned content, for example to rebind callbacks or to update references. + * The promise is resolved with an {@link SplObjectStorage} that provides each original and its cloned element. + * Please note that the promise must be used before calling `parent::__clone()`. + * Otherwise, the promise remains unresolved. + * + * **Example usage:** + * + * ```php + * namespace ipl\Html\Example; + * + * use ipl\Html\HtmlDocument; + * + * // Can also be a subclass of BaseHtmlElement or any other class that extends HtmlDocument + * class ExampleDocument extends HtmlDocument + * { + * public function __clone() + * { + * $copy = $this->copy()->then(function (SplObjectStorage $copies): void { + * foreach ($copies as $original) { + * $copy = $copies->getInfo(); + * // ... + * } + * }); + * + * parent::__clone(); + * } + * } + * ``` + * + * @return PromiseInterface + */ + protected function copy(): PromiseInterface + { + if ($this->copy === null) { + $this->copy = new Deferred(); + } + + return $this->copy->promise(); + } + /** * Render the document to HTML respecting the set wrapper * From 5cc2c242c58ca22f8f555d7d18e08f51e9f02bc4 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Feb 2023 23:01:36 +0100 Subject: [PATCH 13/18] Deep rebind attribute callbacks after clone --- src/BaseHtmlElement.php | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/BaseHtmlElement.php b/src/BaseHtmlElement.php index 9b479b62..4dd20382 100644 --- a/src/BaseHtmlElement.php +++ b/src/BaseHtmlElement.php @@ -395,15 +395,29 @@ public function renderUnwrapped() public function __clone() { - parent::__clone(); + $this->copy()->then(function (SplObjectStorage $copies): void { + foreach ($copies as $ignored) { + // SplObjectMap always iterates over the attached objects that + // are the elements before they have been cloned. + // But we want to work with the cloned element. + $copy = $copies->getInfo(); + if ($copy instanceof self) { + if ($copy->attributes !== null) { + $copy->attributes->rebindAttributeCallbacks($this->objectId(), $this); + } + } + } - if ($this->attributes !== null) { - $this->attributes = clone $this->attributes; + if ($this->attributes !== null) { + $this->attributes = clone $this->attributes; - // $this->objectId() returns the ID of the object before cloning, $this is the newly cloned object. - $this->attributes->rebindAttributeCallbacks($this->objectId(), $this); + // $this->objectId() returns the ID of the object before cloning, $this is the newly cloned object. + $this->attributes->rebindAttributeCallbacks($this->objectId(), $this); + } $this->ensureObjectId(true); - } + }); + + parent::__clone(); } } From f92425085565cf9bfab12f1d471e5155e43bf725 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Feb 2023 23:02:46 +0100 Subject: [PATCH 14/18] Test deep rebinding attribute callbacks after clone --- tests/Lib/FormProvidingAttributeCallbacks.php | 30 +++++++++++++++++++ tests/RebindAttributeCallbacksTest.php | 29 ++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 tests/Lib/FormProvidingAttributeCallbacks.php diff --git a/tests/Lib/FormProvidingAttributeCallbacks.php b/tests/Lib/FormProvidingAttributeCallbacks.php new file mode 100644 index 00000000..aa35db4d --- /dev/null +++ b/tests/Lib/FormProvidingAttributeCallbacks.php @@ -0,0 +1,30 @@ +getAction(); + } + public function getFormmethod(): ?string + { + return $this->getMethod(); + } + protected function assemble() + { + $submit = $this->createElement('submit', 'submit'); + $submit + ->getAttributes() + ->registerAttributeCallback('formaction', [$this, 'getFormaction']) + ->registerAttributeCallback('formmethod', [$this, 'getFormmethod']); + /** @var FieldsetElement $fieldset */ + $fieldset = $this->createElement('fieldset', 'fieldset'); + $fieldset->addElement($submit); + $this->addElement($fieldset); + } +} diff --git a/tests/RebindAttributeCallbacksTest.php b/tests/RebindAttributeCallbacksTest.php index 0a0688c2..a276cf10 100644 --- a/tests/RebindAttributeCallbacksTest.php +++ b/tests/RebindAttributeCallbacksTest.php @@ -6,6 +6,7 @@ use ipl\Html\Attributes; use ipl\Html\BaseHtmlElement; use ipl\Tests\Html\Lib\ElementWithAttributeCallbacks; +use ipl\Tests\Html\Lib\FormProvidingAttributeCallbacks; use ReflectionFunction; use ReflectionProperty; @@ -113,6 +114,34 @@ public function testElementCallbacksCloning(): void $this->assertCallbacksFor($clone); } + public function testDeepRebinding(): void + { + $form = (new FormProvidingAttributeCallbacks()) + ->ensureAssembled(); + + $clone = (clone $form) + ->setAction('action') + ->setMethod('GET'); + + $originalHtml = <<<'HTML' +
+
+ +
+
+HTML; + $this->assertHtml($originalHtml, $form); + + $cloneHtml = <<<'HTML' +
+
+ +
+
+HTML; + $this->assertHtml($cloneHtml, $clone); + } + protected function getCallbackThis(callable $callback): ?object { if (! $callback instanceof Closure) { From 88298a9febfe440b47e046694939a7aea4e33b6c Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Mon, 13 Feb 2023 09:43:13 +0100 Subject: [PATCH 15/18] Tests: Allow to provide a message to assertHtml() --- tests/TestCase.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/TestCase.php b/tests/TestCase.php index e2207908..c6914d7e 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -20,8 +20,9 @@ abstract class TestCase extends \PHPUnit\Framework\TestCase * * @param string $expectedHtml * @param ValidHtml $actual + * @oaram string $message */ - protected function assertHtml($expectedHtml, ValidHtml $actual) + protected function assertHtml($expectedHtml, ValidHtml $actual, string $message = '') { $expectedHtml = str_replace( "\n", @@ -37,7 +38,7 @@ protected function assertHtml($expectedHtml, ValidHtml $actual) $actualHtml = (new Xml\Loader())->load($actual->render(), true); } - $this->assertEquals($expectedHtml, $actualHtml); + $this->assertEquals($expectedHtml, $actualHtml, $message); } /** From 0ab437ce159933908bc43f2a048ffa1ae21864fb Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Mon, 13 Feb 2023 17:51:59 +0100 Subject: [PATCH 16/18] Support cloning forms --- src/Form.php | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/Form.php b/src/Form.php index 93bb6d98..0cf882c8 100644 --- a/src/Form.php +++ b/src/Form.php @@ -8,6 +8,7 @@ use ipl\Html\FormElement\FormElements; use ipl\Stdlib\Messages; use Psr\Http\Message\ServerRequestInterface; +use SplObjectStorage; class Form extends BaseHtmlElement { @@ -284,6 +285,34 @@ public function remove(ValidHtml $elementOrHtml) $this->removeElement($elementOrHtml); } + public function __clone() + { + $this->copy()->then(function (SplObjectStorage $copies): void { + $cloned = []; + + foreach ($copies as $original) { + if (! $original instanceof FormElement) { + continue; + } + + if (! $this->hasElement($original)) { + continue; + } + + $cloned[$original->getName()] = $copies->getInfo(); + } + + // Also clone elements that have only been registered. + foreach (array_diff_key($this->elements, $cloned) as $name => $element) { + $cloned[$name] = clone $element; + } + + $this->elements = $cloned; + }); + + parent::__clone(); + } + protected function onError() { $errors = Html::tag('ul', ['class' => 'errors']); From ed41c5d0c1fefbcd248f2ce3c2216213b9727b86 Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Mon, 13 Feb 2023 17:52:20 +0100 Subject: [PATCH 17/18] Test cloning select elements --- tests/FormElement/SelectElementTest.php | 111 ++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/tests/FormElement/SelectElementTest.php b/tests/FormElement/SelectElementTest.php index 3f8689c2..60999e38 100644 --- a/tests/FormElement/SelectElementTest.php +++ b/tests/FormElement/SelectElementTest.php @@ -8,6 +8,7 @@ use ipl\I18n\NoopTranslator; use ipl\I18n\StaticTranslator; use ipl\Tests\Html\TestCase; +use ReflectionFunction; use UnexpectedValueException; class SelectElementTest extends TestCase @@ -615,4 +616,114 @@ public function testGetOptionReturnsPreviouslySetOption() $this->assertNull($select->getOption('')->getValue()); $this->assertSame('car', $select->getOption('car')->getValue()); } + + public function testDirectCloning() + { + $select = new SelectElement('select', [ + 'options' => [ + null => 'Please choose', + 'option1' => 'Option 1', + 'option2' => 'Option 2', + 'option3' => 'Option 3', + 'option4' => 'Option 4' + ], + 'disabledOptions' => ['option3', 'option4'] + ]); + $select->setValue('option1'); + + $clone = (clone $select) + ->setDisabledOptions([]) + ->setValue('option2'); + + $selectHtml = <<<'HTML' + +HTML; + $this->assertHtml($selectHtml, $select, 'Modifying the cloned element also affects the original element'); + + $cloneHtml = <<<'HTML' + +HTML; + $this->assertHtml($cloneHtml, $clone, 'Modifying the cloned element does not have the expected result'); + + $assembledClone = (clone $select) + ->setDisabledOptions([]) + ->setValue('option2'); + $this->assertHtml( + $cloneHtml, + $assembledClone, + 'Modifying the clone of the already assembled element does not have the expected result' + ); + } + + public function testImplicitCloning() + { + $select = new SelectElement('select', [ + 'options' => [ + null => 'Please choose', + 'option1' => 'Option 1', + 'option2' => 'Option 2', + 'option3' => 'Option 3', + 'option4' => 'Option 4' + ], + 'disabledOptions' => ['option3', 'option4'] + ]); + $select->setValue('option1'); + $form = (new Form()) + ->addElement($select); + + $clone = clone $form; + $clone + ->getElement('select') + ->setDisabledOptions([]) + ->setValue('option2'); + + $formHtml = <<<'HTML' +
+ +
+HTML; + $this->assertHtml($formHtml, $form, 'Modifying the cloned element also affects the original element'); + + $cloneHtml = <<<'HTML' +
+ +
+HTML; + $this->assertHtml($cloneHtml, $clone, 'Modifying the cloned element does not have the expected result'); + + $assembledClone = clone $form; + $assembledClone + ->getElement('select') + ->setDisabledOptions([]) + ->setValue('option2'); + $this->assertHtml( + $cloneHtml, + $assembledClone, + 'Modifying the clone of the already assembled element does not have the expected result' + ); + } } From 14225a0da36ec4cb182d3ba7a47935ae5f1ae87a Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Mon, 13 Feb 2023 17:52:44 +0100 Subject: [PATCH 18/18] WIP: Support cloning select elements --- src/FormElement/SelectElement.php | 67 +++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/src/FormElement/SelectElement.php b/src/FormElement/SelectElement.php index e6b4f217..40c299b6 100644 --- a/src/FormElement/SelectElement.php +++ b/src/FormElement/SelectElement.php @@ -2,13 +2,13 @@ namespace ipl\Html\FormElement; -use InvalidArgumentException; use ipl\Html\Attributes; use ipl\Html\Common\MultipleAttribute; use ipl\Html\Html; use ipl\Html\HtmlElement; use ipl\Validator\DeferredInArrayValidator; use ipl\Validator\ValidatorChain; +use SplObjectStorage; use UnexpectedValueException; class SelectElement extends BaseFormElement @@ -116,6 +116,64 @@ public function getNameAttribute() return $this->isMultiple() ? ($name . '[]') : $name; } + public function __clone() + { + if ($this->hasBeenAssembled) { + $contentObjectIds = []; + foreach ($this->optionContent as $value => $content) { + $contentObjectIds[$value] = spl_object_id($content); + } + foreach (array_diff_key($this->options, $contentObjectIds) as $value => $option) { + $contentObjectIds[$value] = spl_object_id($option); + } + $this->copy()->then(function (SplObjectStorage $copies) use ($contentObjectIds) { + $lookup = array_flip($contentObjectIds); + foreach ($copies as $option) { + $objectId = spl_object_id($option); + if (! isset($lookup[$objectId])) { + continue; + } + $value = $lookup[$objectId]; + $copy = $copies->getInfo(); + if (isset($this->optionContent[$value])) { + $this->optionContent[$value] = $copy; + } + if (isset($this->options[$value])) { + $this->options[$value] = $copy; + } + } + }); + + parent::__clone(); + + return; + } + + foreach ($this->optionContent as $value => &$content) { + if (! $content instanceof SelectOption) { + $content->copy()->then(function (SplObjectStorage $copies) use ($value) { + foreach ($copies as $option) { + if (! $option instanceof SelectOption) { + continue; + } + /** @var SelectOption $copy */ + $copy = $copies->getInfo(); + $copy->getAttributes()->rebindAttributeCallbacks($this->objectId(), $this); + $this->options[$value] = $copy; + } + }); + + $content = clone $content; + } else { + $content = clone $content; + $content->getAttributes()->rebindAttributeCallbacks($this->objectId(), $this); + $this->options[$value] = $content; + } + } + + parent::__clone(); + } + /** * Make the selectOption for the specified value and the label * @@ -138,8 +196,11 @@ protected function makeOption($value, $label) $option = (new SelectOption($value, $label)) ->setAttribute('disabled', in_array($value, $this->disabledOptions, ! is_int($value))); - $option->getAttributes()->registerAttributeCallback('selected', function () use ($option) { - return $this->isSelectedOption($option->getValue()); + // The value of select options is immutable, + // so we use that for the callback instead of the option itself to + // make it possible to rebind callbacks after cloning the element. + $option->getAttributes()->registerAttributeCallback('selected', function () use ($value) { + return $this->isSelectedOption($value); }); $this->options[$value] = $option;