Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cloning #96

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Cloning #96

wants to merge 18 commits into from

Conversation

lippserd
Copy link
Member

No description provided.

@cla-bot cla-bot bot added the cla/signed label Nov 29, 2022
@lippserd lippserd force-pushed the cloning branch 2 times, most recently from 834bd7a to be97fd2 Compare November 29, 2022 09:33
@lippserd lippserd force-pushed the cloning branch 2 times, most recently from 477d42e to a5863dc Compare December 13, 2022 12:48
@TAINCER TAINCER force-pushed the cloning branch 10 times, most recently from 15ff4e8 to c5c51ab Compare December 15, 2022 16:29
lippserd
lippserd commented Jan 9, 2023
View reviewed changes
src/Attributes.php Outdated Show resolved Hide resolved
src/Attributes.php Outdated Show resolved Hide resolved
src/Attributes.php Outdated Show resolved Hide resolved
src/Attributes.php Outdated Show resolved Hide resolved
src/Attributes.php Outdated Show resolved Hide resolved
src/BaseHtmlElement.php Outdated Show resolved Hide resolved
src/BaseHtmlElement.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
@TAINCER TAINCER force-pushed the cloning branch 2 times, most recently from ad3f164 to e2b718f Compare January 9, 2023 11:36
lippserd
lippserd commented Jan 9, 2023
View reviewed changes
src/Attributes.php Outdated Show resolved Hide resolved
src/Attributes.php Outdated Show resolved Hide resolved
src/BaseHtmlElement.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
lippserd
lippserd commented Jan 9, 2023
View reviewed changes
Copy link
Member Author

@lippserd lippserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this requires more effort, I would rather test the expected HTML instead of asserting that the output is not the same, as this could be caused by anything.

src/Attributes.php Outdated Show resolved Hide resolved
src/Attributes.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
tests/CloneTest.php Outdated Show resolved Hide resolved
@lippserd lippserd marked this pull request as ready for review January 16, 2023 10:11
@nilmerg nilmerg added this to the v0.7.0 milestone Jan 16, 2023
@lippserd
Copy link
Member Author

@TAINCER Please rebase.

@lippserd
Copy link
Member Author

lippserd commented Jan 17, 2023 •
edited
Loading

@TAINCER @nilmberg has found that by keeping a reference to $this the object can only be destroyed at the end of the script. With a manual unset() call nothing happens. Since PHP 7.4, WeakReference is available as a workaround. But since we are at PHP 7.2+, we have to solve this differently. Instead of keeping a reference to $this, we store the spl_object_id() and compare it later with the ID of the callback this.

@TAINCER TAINCER force-pushed the cloning branch 2 times, most recently from 7d84d39 to 34a7d97 Compare January 17, 2023 10:12
nilmerg
nilmerg reviewed Jan 20, 2023
View reviewed changes
tests/CloneTest.php Outdated Show resolved Hide resolved
nilmerg
nilmerg reviewed Jan 20, 2023
View reviewed changes
tests/CloneTest.php Outdated Show resolved Hide resolved
@TAINCER TAINCER requested a review from nilmerg February 2, 2023 13:52
nilmerg
nilmerg previously approved these changes Feb 2, 2023
View reviewed changes
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Now please push this on a different branch, rebased with master, and create a new PR. You're the author and @lippserd can't approve otherwise.

@nilmerg nilmerg mentioned this pull request Feb 2, 2023
Open
@nilmerg nilmerg removed this from the v0.7.0 milestone Feb 2, 2023
Timm Ortloff and others added 5 commits February 10, 2023 09:16
@lippserd
Test rebinding attribute callbacks after clone
cfb7c39
@lippserd
Rebind attribute callbacks after clone
42bbe4a 
After cloning elements, each own attribute callback must be rebound so
that the clones continue to work together.
@lippserd
Document attribute callback rebinding
badc8c4
@lippserd
Introduce HtmlDocument::initAssemble()
709e808 
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.
@lippserd
Refine PHPDoc of HtmlDocument::ensureAssembled()
370a723
@lippserd
Also set object ID (to rebind callbacks) on assemble
41ef9ef 
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.
@lippserd
HtmlDocument: Deep copy content in one pass instead of cloning recurs…
20d61a9 
…ively

Instead of cloning the content of each content element individually, the
entire content is cloned once, also preserving the object graph.
@lippserd
HtmlDocument: Test deep copy
f4080b3
@lippserd
HtmlDocument: Provide cloned content in subclasses during __clone()
7897a96 
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.
@lippserd
Deep rebind attribute callbacks after clone
5cc2c24
@lippserd
Test deep rebinding attribute callbacks after clone
f924250
@lippserd
Tests: Allow to provide a message to assertHtml()
88298a9
@lippserd
Support cloning forms
0ab437c
@lippserd
Test cloning select elements
ed41c5d
@lippserd
WIP: Support cloning select elements
14225a0
@lippserd lippserd mentioned this pull request Feb 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TAINCER TAINCER TAINCER left review comments

@nilmerg nilmerg nilmerg left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cla/signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lippserd @nilmerg @TAINCER