#2191285-17: Text module is not required, but is marked as required discovered that _filter_htmlcorrector()
is used in many places throughout core, not related to Filter module.
→ Convert the internals of _filter_htmlcorrector()
into a Drupal\Core\Utility\Html
class.
Note: #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 might replace this utility class with an external HTML5 library later on, but for now, we need to unblock #2191285: Text module is not required, but is marked as required, so we can fix a hidden broken module dependency problem.
Comment | File | Size | Author |
---|---|---|---|
#19 | interdiff.txt | 3.82 KB | sun |
#19 | filter.dom_.19.patch | 29.85 KB | sun |
#15 | interdiff.txt | 4.04 KB | sun |
#15 | filter.dom_.15.patch | 29.95 KB | sun |
#12 | interdiff.txt | 744 bytes | sun |
Comments
Comment #1
sunAttached patch moves the corresponding functions into a new
Drupal\Component\Utility\Html
class.Next to fixing/modernizing phpDoc + signatures, it also adds a
Html::normalize()
helper function that essentially acts as a 1:1 replacement for_filter_htmlcorrector()
.Comment #2
sunActually, let's get rid of the filter_dom_* functions entirely while being there.
Comment #4
sunReverted change in theme_file_upload_help(). → #2195891: Clean faulty HTML in all administrative strings
Comment #6
amateescu CreditAttribution: amateescu commentedA
Html
class name doesn't really make sense on its own (without taking the namespace into consideration), how aboutDOMDocument
orDOMDocumentHelper
?Comment #7
jibranIMHO $html should be $text or $xml because it is not html.
Can we rename it to doc? We can print $html in the heredoc so no need for $prefix and $suffix.
We should convert it phpUnitTest so that we can use dataProvider.
Comment #9
BerdirNeeds to be \DOMDocument
We discussed PHPUnit too, but that test class tests a bunch of other functions too, which we would either have to convert too or split the test up. I'd prefer to not do that here.
Comment #10
sunFixed Html::load().
re: #6:
I wasn't sure what the most appropriate class name would be at the beginning, but after studying the provided functionality, this class solely provides helper methods for operating on snippets of HTML strings, whereas the most common use-case is just simply
Html::normalize()
.In other words, this is not a helper class for DOMDocument — usage of that is a detail that is internal to the utility class and should not be exposed in the public naming.
I considered
HtmlHelper
first, but given thatString
is also justString
and notStringHelper
, and because I think it is rather unlikely that there's going to be a differentHtml
class anywhere (and both imported into the same file), the nameHtml
appears to be most appropriate and most consistent (→ DX) right now.We can consider to change the utility class names from
Foo
intoFooHelper
, but for DX/consistency, that effort should then rename all of them IMHO.re: #7:
The accepted variable is actually supposed to contain HTML, not plain-text. Therefore,
$html
is the proper variable name.As @Berdir mentioned, I'd like to defer the filter test conversion to a separate issue. The primary purpose of this issue is to unblock #2191285: Text module is not required, but is marked as required ASAP.
Comment #12
sunFixed
text_summary()
is not able to supply all required parameters forFilterInterface::process()
.Comment #14
jibranI am removing phpunit tag and adding needs followup tag also now we need change record before the commit so adding the tag for a remainder.
How about xhtml?
Some minor doc issues.
\DOMDocument, \Html and (X)HTML
I think _drupal_get_js know
Comment #15
sunNo, it's just HTML. Details are internal to the methods and not going to be addressed here.
Fixed Heredoc in Html::load() causes extraneous whitespace upon serialization.
Comment #16
amateescu CreditAttribution: amateescu commentedFWIW: #2184653: Rename Drupal\Component\Utility\Url to UrlHelper
Comment #17
sunWould it be possible to move the naming discussion into a separate follow-up issue, please? The primary objective of this issue here is to unblock #2191285: Text module is not required, but is marked as required ASAP.
Comment #18
jibranJust minor issues other then that RTBC for me. We just need followups and change record after this.
New comment it should be \DOMDocument.
\DOMDocumnet
Instead of DOM object can we use \DOMDocument
Instead of DOM element can we use \DOMNode
Just for the record. We can access \DOMDocument form \DOMNode. Maybe we can fix it in follow up.
Comment #19
sunComment #20
jibranThank you very much for the changes. It is RTBC for me. It just needs followup and change record. :)
Comment #21
sunChange notice: https://drupal.org/node/2197131
I'm leaving the creation of a follow-up issue to whoever feels that it is necessary, as I'm comfortable with the current code. As mentioned before, it would be best if such an issue would then have the objective to rename all of the utility classes into
*Helper
for consistency/DX reasons.Comment #22
jibranThanks for the change notice. Can we add other function as well to it?
Comment #23
sunUpdated change notice for filter_dom_*() functions.
filter_dom_escape_cdata_element()
is not included, because it never was a public API function, only used internally byfilter_dom_serialize()
.@jibran, you could have edited the change notice yourself, especially after nitpicking this patch to death. ;)
Comment #24
jibranThanks for the changes so RTBC. :)
@sun you seem perfectly motivated to make the changes so I don't want to disturb your flow. :D
Comment #25
alexpottCommitted a16b563 and pushed to 8.x. Thanks!
Comment #27
yched CreditAttribution: yched commented[edited out : wrong issue]
Comment #28
yched CreditAttribution: yched commentedOops, never mind the previous post, wrong issue.