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

Update DOMLex.php #292

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update DOMLex.php #292

wants to merge 2 commits into from

Conversation

amanpatel
Copy link

Fixed two issues where property references that don't exist cause errors on PHP 8 (on docker setup by "tiredofit").

@amanpatel
Update DOMLex.php
fb8ad91 
Fixed two issues where property references that don't exist cause errors on PHP 8 (on docker setup by "tiredofit").
@amanpatel amanpatel mentioned this pull request May 20, 2021
Closed
@amanpatel
Update DOMLex.php
f4fdd55 
Also fixed another instance of "undefined property" error.
@ezyang
Copy link
Owner

ezyang commented May 21, 2021

By any chance do you know where in the libxml changelog DOM was modified in this way? That would help me assess this change.

@amanpatel
Copy link
Author

Could this be a PHP 8 issue? Normally undefined properties are notice level.

But for some reason in my setup for freescount (running under Docker/PHP 8), the undefined properties are triggering a ErrorExceptions. Perhaps laravel framework sets up error reporting this way. I am a bit confused as well.

Would sharing the HTML string for messages that trigger this help you recreate this error?

@amanpatel
Copy link
Author

@ezyang I am going to close this PR, it turns out this might be what I'm looking for:
https://stackoverflow.com/questions/18497788/laravel-breaks-entire-app-on-php-notices

@amanpatel amanpatel closed this May 21, 2021
@ezyang ezyang reopened this May 23, 2021
@ezyang
Copy link
Owner

ezyang commented May 23, 2021

reopening because we ought to be notice clean

@amanpatel
Copy link
Author

amanpatel commented May 23, 2021 •
edited
Loading

I tried to run the tests locally, but failed (Wanted to see if I could create a failing test case for you). Failing that though, I can recreate this under my envrionment. So I'm attaching the input that causes the error. Also the error log is as below:

[2021-05-22 19:43:55] production.ERROR: Undefined property: DOMAttr::$name 
(View: /www/html/resources/views/conversations/partials/thread.blade.php) 
(View: /www/html/resources/views/conversations/partials/thread.blade.php) 
(View: /www/html/resources/views/conversations/partials/thread.blade.php) 


{"userId":1,"email":"[email protected]", "exception":"[object] (ErrorException(code: 0): Undefined property: DOMAttr::$name 
(View: /www/html/resources/views/conversations/partials/thread.blade.php) 
(View: /www/html/resources/views/conversations/partials/thread.blade.php) 
(View: /www/html/resources/views/conversations/partials/thread.blade.php) at /www/html/vendor/ezyang/htmlpurifier/library/HTMLPurifier/Lexer/DOMLex.php:267, 

ErrorException(code: 0): Undefined property: DOMAttr::$name 
(View: /www/html/resources/views/conversations/partials/thread.blade.php) 
(View: /www/html/resources/views/conversations/partials/thread.blade.php) at /www/html/vendor/ezyang/htmlpurifier/library/HTMLPurifier/Lexer/DOMLex.php:267, 

ErrorException(code: 0): Undefined property: DOMAttr::$name 
(View: /www/html/resources/views/conversations/partials/thread.blade.php) at /www/html/vendor/ezyang/htmlpurifier/library/HTMLPurifier/Lexer/DOMLex.php:267, 

ErrorException(code: 0): Undefined property: DOMAttr::$name at /www/html/vendor/ezyang/htmlpurifier/library/HTMLPurifier/Lexer/DOMLex.php:267)

Fair warning (don't click on any links in the attached file). From that rabbit hole of following the function calls, I was able to determine that most likely the purifier makes its entry over here:

https://github.com/freescout-helpdesk/freescout/blob/f3aa63aa13f0e7964aff0df9e77b72b02dd0d51c/vendor/mews/purifier/src/Purifier.php#L263

html-purifier-domlex-error.txt

@amanpatel
Copy link
Author

amanpatel commented May 24, 2021 •
edited
Loading

@ezyang This seems to be very specific to my environment. When I checked out this repository and ran the purify function with the basic config & the input attached above, it did not display any notices or error exceptions.

I will try to reproduce in a clean php 8 docker container to see if I get the same issue. If that is true, I'll have a reproducible test case for you.

xemlock
xemlock reviewed Aug 28, 2021
View reviewed changes
library/HTMLPurifier/Lexer/DOMLex.php
@@ -194,7 +194,7 @@ protected function createStartNode($node, &$tokens, $collect, $config)
} elseif ($node->nodeType === XML_CDATA_SECTION_NODE) {
// undo libxml's special treatment of <script> and <style> tags
$last = end($tokens);
$data = $node->data;
$data = $node->data ?? "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Use of null coalesce operator should be reflected in min PHP version required in composer.json (e.g.: "php": ">=7.0"), otherwise the library will be broken on older installations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xemlock xemlock xemlock left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amanpatel @ezyang @xemlock