Problem/Motivation

HTML5 markup is different than older versions, and there is already a library to parse it added as core dependency.

Proposed resolution

Use \Masterminds\HTML5 as the parser, depending on a parameter inside the dom plugin.

Remaining tasks

Provide a patch
Code review

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#10 interdiff-6-10.txt1.33 KBmarvil07
#10 3096393-10-html5-dom-import.patch2.97 KBmarvil07
#9 3096393-9-html5-dom-import.patch1.33 KBmarvil07
#6 3096393-6-html5-dom.patch2.91 KBmarvil07
#2 3096393-2-html5-dom.patch2.9 KBmarvil07

Issue fork migrate_plus-3096393

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

marvil07 created an issue. See original summary.

marvil07’s picture

marvil07 commented
Issue summary: View changes
StatusFileSize
new 3096393-2-html5-dom.patch2.9 KB
marvil07’s picture

marvil07 commented
Assigned: marvil07 » Unassigned
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Active » Needs review
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Needs work

Patch failed to apply

marvil07’s picture

marvil07 commented
Status: Needs work » Needs review
StatusFileSize
new 3096393-6-html5-dom.patch2.91 KB

It seems like the automated testing does not consider -p0 for patch applying, here a version compatible with -p1.
No actual changes, only generated the patch again.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Needs work 
+++ b/src/Plugin/migrate/process/Dom.php
@@ -163,7 +174,16 @@ class Dom extends ProcessPluginBase {
diff --git a/tests/src/Unit/process/DomTest.php b/tests/src/Unit/process/DomTest.php

Could we add a html5 test too?

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

This looks backwards:

+ * - import_method: (optional) What parser to use. Default to 'html'. Possible
+ *   values:
+ *   - 'html': use html5 parsing.
+ *   - 'html5': use dom extension parsing.
marvil07’s picture

marvil07 commented
StatusFileSize
new 3096393-9-html5-dom-import.patch1.33 KB

Fixed documentation, test pending.

marvil07’s picture

marvil07 commented
StatusFileSize
new 3096393-10-html5-dom-import.patch2.97 KB
new interdiff-6-10.txt1.33 KB

I guess I lost practice, here once again, but the right patch and the related interdiff.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Needs work » Needs review

I am setting the status to NR in order to trigger a test.

Matroskeen made their first commit to this issue’s fork.

Matroskeen opened merge request !56

  • Matroskeen committed e5f2d55 on 8.x-5.x 
    Issue #3096393 by marvil07, Matroskeen: Support html5 parsing on dom...

  • Matroskeen committed ce552ed on 6.0.x 
    Issue #3096393 by marvil07, Matroskeen: Support html5 parsing on dom...
matroskeen’s picture

matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
commented
Status: Needs review » Fixed

It looks good to me! I tested both import variants using Migrate Sandbox module and had the same results in the end.

I applied some minor changes that should not affect the functionality and added one more trivial test. It would be nice to have more advanced test coverage to cover some features of html5 importer, but it can be done in a follow-up task.

Merged into 8.x-5.x and cherry-picked into 6.0.x.
Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.