Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#41622 closed defect (bug) (fixed)

Text widget can show DOMDocument::loadHTML() warnings in admin when is_legacy_widget method is called

Reported by: codezen8's profile codezen8 Owned by: westonruter's profile westonruter
Milestone: 4.8.2 Priority: normal
Severity: normal Version: 4.8.1
Component: Widgets Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

Have a number of sites that use html in widgets, so waited to upgrade from 4.7.5 to 4.8.1 - a few of the sites would not load /wp-admin/widgets.php after upgrading

error occurred in the function that evaluates whether this widget should use legacy text mode or the visual editor.

errors like:

PHP Warning: DOMDocument::loadHTML(): Tag footer invalid in Entity, line: 3 in ---\wp-includes\widgets\class-wp-widget-text.php on line 121

PHP Warning: DOMDocument::loadHTML(): Tag lorem invalid in Entity, line: 3 in --- \wp-includes\widgets\class-wp-widget-text.php on line 121

PHP Warning: DOMDocument::loadHTML(): Unexpected end tag : div in Entity, line: 6 in --- \wp-includes\widgets\class-wp-widget-text.php on line 121

I temporarily changed the function is_legacy_instance( $instance ) in that file to always return true. The widget page opens with all widgets in legacy more; I found some test widgets (haven't updated any production sites yet) that had html5 elements (<footer>), text that looks something like tags (<lorem ipsum), and invalid html (a widget with a <h2>some text</h3>).

Updating these widgets to remove/fix these items and replacing the original file, everything works fine.

I've also found instances where client added html that was not clean, but worked as old text widget. In 4.8.1 parts of that code and the text in the tag (tag that doesn't have end tag for example) disappear in the visual editor and the text view in the new widget. reverting the site back to 4.7.5 shows that the original values are still in the database.

Attachments (1)

41622.0.diff (1.3 KB) - added by westonruter 7 years ago.

Download all attachments as: .zip

Change History (14)

#1 @codezen8
7 years ago

Also want to add that the problem widgets all appear to use html and automatically add paragraphs is checked. Note also that once upgraded without problem code, I can add back html5 elements like <footer> in the legacy text widget and there are no issues.

#2 @westonruter
7 years ago

  • Keywords reporter-feedback added

@codezen8 Thanks for the report. Could you provide some the full examples of text that was in the widgets prior to the upgrade?

So it seems like at the very least we need to add error-suppression for the loadHTML call and specify the HTML5 doctype, like:

<?php
--- src/wp-includes/widgets/class-wp-widget-text.php
+++ src/wp-includes/widgets/class-wp-widget-text.php
@@ -114,8 +114,8 @@ class WP_Widget_Text extends WP_Widget {
                }
 
                $doc = new DOMDocument();
-               $doc->loadHTML( sprintf(
+               @$doc->loadHTML( sprintf(
-                       '<html><head><meta charset="%s"></head><body>%s</body></html>',
+                       '<!DOCTYPE html><html><head><meta charset="%s"></head><body>%s</body></html>',
                        esc_attr( get_bloginfo( 'charset' ) ),
                        $instance['text']
                ) );

Otherwise, I'm not clear what else should be done. Maybe detect if there are HTML parsing warnings and use that as another signal for legacy mode?

#3 @codezen8
7 years ago

  • Keywords reporter-feedback removed

I am not that familiar with core, but conceptually it would make sense to trap for any error or warning in the loadHTML function. If there are issues, return true on is_legacy_instance().

I am able to give you a couple of examples, but note that once you've updated to 4.8.1, if you revert the files only and not the db to 4.7.5 updating the widgets to try and re-create the issue doesn't reproduce it. It seems that the widgets had to have been created and saved with files and db at 4.7.5 or maybe even earlier (some of the ones I have issue with are much older in terms of last update prior to this)

Example 1 - <footer> tag
PHP Warning: DOMDocument::loadHTML(): Tag footer invalid in Entity, line: 3 in ---\wp-includes\widgets\class-wp-widget-text.php on line 121

widget code: NOTE only causes the issue if Automatically add Paragraphs is also checked

<blockquote>I have been a client of Sue's for almost 2 years now.  Her individual care, knowledge of massage therapy and passion for healing has made me feel cared for and safe.  Her work truly makes a difference in my stressed and crazy life and helps me feel rejuvenated each visit.  Thank you Sue for making a difference and being there for me.

<footer>Sammie C. - Willsboro, ST</footer>
</blockquote>

force legacy to true, uncheck auto paragraph, go back to 4.8.1 version of file and it works. I can then also recheck auto paragraph, so it only seems to fail on initial update when widget is 4.7.5 or earlier db entry

Example 2: when using a shortcode in the widget as part of an html attribute. Realize this is no longer supported, but this was a much older implementation

widget code:

<div class="footer-widget-cta"><h4>It all starts with a conversation</h4><p>to see if I might be a good fit for your project. If you are ready to make some changes...</p>
<a class="button alignnone" href="[siteurl]/contact/">Contact Me</a></div>

removing [siteurl] shortcode to the current site url fixes issue

Example 3: when including a link that has & (or possibly other unescaped html) AND I think you also must have auto paragraph checked in the widget

widget code:

<a href="/script.php?foo=bar&hello=world">link doesn't work</a>

<a href="/script.php?foo=bar&amp;hello=world">changing to this link does</a>

#4 @westonruter
7 years ago

  • Keywords reporter-feedback added

@codezen8 thank you for the examples.

You're right, in order to test this you must create the new Text widgets in 4.7.x and then switch to 4.8.1 in order to see how the legacy mode detection applies.

I tried creating those three widgets in 4.7.5 and when I switched to 4.8.1 the first two widgets got recognized as legacy as expected, and the third one got migrated to visual as also expected. The only problem I saw was the PHP warnings raised by loadHTML but after saving each widget, the error went away since either TinyMCE fixed the malformed HTML or the legacy mode was permanently applied and so loadHTML no longer was invoked as part of the is_legacy_instance method.

To confirm, is the presence of these “DOMDocument::loadHTML()” warnings the only issue you are reporting?

Here are screenshots of what the widgets looked like after switching to 4.8.1: https://cloudup.com/c-7cAvSo32n

@westonruter
7 years ago

#5 @westonruter
7 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.8.2

#6 @codezen8
7 years ago

  • Keywords reporter-feedback removed

Yes, the warnings are the issue - there may be some cases where the conversion to tinymce stripped out some code due to malformed html once it updated and converted the widget, but that should be taken care of if when there is a warning the widget is left in legacy mode instead of converting.

It may be because of the php settings on my dev server that the warnings prevented the admin widget page from loading at all for me - I had to temporarily force all widgets to legacy in order to even see the widget admin in 4.8.1, then correct the code and re-enable the check. The sites in question on this server do not have debug mode enabled, but it may be that php is set up as development server-wide.

#7 @westonruter
7 years ago

@codezen8 if you set up the Text widgets in 4.7.5 and then switch to 4.8.1 and apply the patch in 41622.0.diff, does everything then work as expected with the Text widgets added in 4.7.x? I don't see why the admin screen would fail to load regardless.

#8 @codezen8
7 years ago

@westonruter I restored two of the sites from backup to version 4.7.5, updated the sites to 4.8.1 and applied the patch. The widget admin screen loads fine after the patch and the widgets work as expected.

The admin screen not loading pre-patch must have something to do with the fact that PHP (7.0.19) is set up as a development server for those sites, because I did not get the same error on admin page load for a warning on the same version of the production sites when we updated them (also running 7.0.19 but in production mode).

#9 @westonruter
7 years ago

  • Keywords commit added
  • Owner set to westonruter
  • Status changed from new to accepted

@codezen8 thanks very much for the confirmation. I'll commit 41622.0.diff and close this ticket out as resolved for 4.8.2

#10 @westonruter
7 years ago

  • Summary changed from Issue with upgrading text widgets that contain html to 4.8.1 to Text widget can show DOMDocument::loadHTML() warnings in admin when is_legacy_widget method is called

#11 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 41251:

Widgets: Suppress PHP warnings raised by DOMDocument::loadHTML() in WP_Widget_Text::is_legacy_instance() which could appear in Text widget forms.

Also explicitly use HTML5 doctype when parsing Text widget contents in legacy mode detection.

Amends [41050].
See #40951.
Fixes #41622.

#12 @westonruter
7 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for consideration in 4.8.2.

#13 @ocean90
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 41391:

Widgets: Suppress PHP warnings raised by DOMDocument::loadHTML() in WP_Widget_Text::is_legacy_instance() which could appear in Text widget forms.

Also explicitly use HTML5 doctype when parsing Text widget contents in legacy mode detection.

Merge of [41251] to the 4.8 branch.

Amends [41050].
See #40951.
Fixes #41622.

Note: See TracTickets for help on using tickets.