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

PHP Notice from the Mastermind HTML5 library #22

Closed
sidkshatriya opened this issue Mar 11, 2016 · 2 comments
Closed

PHP Notice from the Mastermind HTML5 library #22

sidkshatriya opened this issue Mar 11, 2016 · 2 comments
Assignees
@sidkshatriya

Comments

@sidkshatriya
Copy link
Contributor

We keep getting this pesky PHP notice. It needs to hunted down and made to go away.

Notice: Undefined property: DOMElement::$data in 
Masterminds\HTML5\Serializer\OutputRules->element() (line 222 of 
/var/www/amp/docroot/vendor/masterminds/html5/src/HTML5/Serializer/OutputRules.php

As far as I can tell, this specific notice does not cause any problems in the AMP HTML output but we still need to try to figure out why it occurs and how to make it go away. Its probably something simple like the code of the html5 library like not doing an isset / !empty check before using the variable.
@sidkshatriya sidkshatriya mentioned this issue Mar 14, 2016
Merged
@sidkshatriya
Copy link
Contributor Author

The PHP dom html parser and the mastermind html5 output serializer don't play nice together when you have tags nested within <noscript> in <head>.

Along with the PHP notice, an empty <noscript> tag was outputted in <head> . In other words this was not a totally benign notice but a small bug that should be resolved now.

This is a subtle issue and a workaround has been implemented in the above PR.
A ticket has been filed in the mastermind library in the meanwhile to see if there is a better workaround than the current workaround: at Masterminds/html5-php#98

TL;DR: @Mdrummond @mtift You should not see this notice anymore.

@sidkshatriya sidkshatriya self-assigned this Mar 15, 2016
sidkshatriya added a commit that referenced this issue Mar 15, 2016
@sidkshatriya
[#22] More efficient workaround for mastermind noscript issue
8aca10a
@sidkshatriya
Copy link
Contributor Author

I've implemented a much more efficient workaround for the <noscript> issue by implementing a new Pass (Lullabot\AMP\Pass\NoscriptTagWorkaroundPass ).

This avoids re-parsing of the whole page html (which is great!). See above commit for details.

@sidkshatriya sidkshatriya mentioned this issue May 16, 2016
Closed
sidkshatriya added a commit that referenced this issue May 16, 2016
@sidkshatriya
[#53] Remove workaround from #22; update composer.json dependencies
ddd396e
@sidkshatriya sidkshatriya mentioned this issue Jun 21, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sidkshatriya sidkshatriya

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@sidkshatriya