Ported ListDiff to DOMDocument #96
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
SavageTiger force-pushed the sven/feature/list_improvements branch from 8948b7c to ba9e8b0 Compare added 2 commits January 27, 2021 15:50
A long time ago DiffList was replaced by DiffListLines, but the old classes have always lingered around as dead code. This commit cleans them up.
These methods never got called, so we can safely remove them.
SavageTiger force-pushed the sven/feature/list_improvements branch 2 times, most recently from e7a62cf to 0d0f608 Compare added 2 commits January 27, 2021 18:00
Right now TableDiff already uses DOMDocument but this component still used a third party library that extended the old SimpleXML extension from PHP. I ported to the code to use DOMDocument, making the library absolete, and slightly improving performance. Mostly this is preperation for moving the entire library to DOMDocument
It is not used anymore since ListDiff get ported to DOMDocument, so we can get rid of it
SavageTiger force-pushed the sven/feature/list_improvements branch from 0d0f608 to c946d79 Compare Since a node with just a space in it is not interpreted as a space by a HTML parsing engine we should not insert empty tags. These seem to result from indent spacing around text being interpreted as regular spaces.
SavageTiger force-pushed the sven/feature/list_improvements branch from 97c64c0 to 2a5a3ce Compare 
| Really liking these improvements here @SavageTiger ! I haven't had a chance to review in depth, and don't let my lack of a review be a blocker to you if you're confident in the work. |
| @jschroed91 I know you trust me to do the right thing and believe me, I have been doing loads of testing, and found loads of tiny bugs in the process haha Just invited you as a reviewer more as a celebratory moment, that some cool improvements have been done again. |
| @SavageTiger Yes this is AWESOME! It sounds like huge improvements without any noticeable tradeoffs, which is great. So glad that you've found and squashes some pesky bugs, and being able to drop an entire dependency of another library will be a great bonus. |
SavageTiger changed the title Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Description
As a preparation to move away from string manipulation (HtmlDiff) and having to ping pong between DOMDocument (TableDiff) / SimpeXML + extended library (ListDiff), I have ported the ListDiff to also use DOMDocument.
We can now get rid of the third party
php-simple-html-dom-parser.Huge string to dom overhead
We have huge overhead now in both ListDiff and TableDiff where we keep going from an html string to objects and back, we should get rid of this once HtmlDiff is also ported
Performance impact
I have been doing loads of testing, and the performance impact is minimal, it turns out it is slightly faster.
In some special cases it can even be 30% faster, but that is not in really realistic scenario's
Big cleanup
All the old ListDiff code was still in the codebase, but none of it was used since it was replaced by HtmlDiffList so that was a huge easy cleanup