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.

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

Update DOM process to use a library that better supports HTML5 #11

Conversation

davidjosephhayes
Copy link

Installed extension on CiviCRM 5.30 w/ php7.3.

This extension was breaking with a number of html5 features such as <script type="text/template"> that are used in the core now. This was most noticeable on the event page. The DOMDocument processor was adding </script> tags anywhere a closing tab (eg </div>) was found inside and <script> tag.

The HTML5DOMDocument library found here https://github.com/ivopetkov/html5-dom-document-php seemed to work the best and be the most active for support and updates. This Pull Request replaces the built in DOMDocument class with IvoPetkov\HTML5DOMDocument. Since IvoPetkov\HTML5DOMDocument extends DOMDocument only providing a thin wrapper, the replacement was very straight forward.

The library was installed with composer. I am not certain if this is the best path for a CiviCRM extension although it works well in my testing.

One file was modified/hacked in a small from the current release of HTML5DOMDocument to fix an issue with the way the library processes some elements. This pull request (ivopetkov/html5-dom-document-php#38) is merge into the upstream library but not available in the current release.

@eileenmcnaughton
Copy link
Owner

I don't mind merging this - but it's not really maintained now that any entity can support custom data

@eileenmcnaughton eileenmcnaughton merged commit 06a2d65 into eileenmcnaughton:master Nov 4, 2020
@davidjosephhayes
Copy link
Author

That makes sense. I was experimenting a bit with the redirects on events. It was a good example of how to approach this in my own extension. Thanks!

@eileenmcnaughton
Copy link
Owner

@davidjosephhayes so if you look at something like related permissions - it was converted away from using this here eileenmcnaughton/nz.co.fuzion.relatedpermissions#17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants