PHP 8.1: fix various deprecations/errors in newest version of PHP#310
PHP 8.1: fix various deprecations/errors in newest version of PHP#310ezyang merged 9 commits intoezyang:masterfrom
Conversation
bytestream
left a comment
There was a problem hiding this comment.
Nice ❤️
May as well add #[\ReturnTypeWillChange] for these two cases as well:
Deprecated: Return type of HTMLPurifier_PropertyListIterator::accept() should either be compatible with FilterIterator::accept(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /home/runner/work/htmlpurifier/htmlpurifier/library/HTMLPurifier/PropertyListIterator.php on line 32
Deprecated: Return type of HTMLPurifier_StringHash::offsetGet($index) should either be compatible with ArrayObject::offsetGet(mixed $key): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /home/runner/work/htmlpurifier/htmlpurifier/library/HTMLPurifier/StringHash.php on line 23
|
Done! |
|
Closes #311 |
|
Anything I can do to help this get merged? |
|
Let me know if you need anything else from me |
|
Just a case of waiting :p - I don't think Edward will ask for any other changes. |
|
What's with all the (string) casts? |
|
It's been a few weeks, but I think the issue was that pre-8.1 these functions did implicit string casts, and now they require a value of type |
| if (!empty($def->content_model)) { | ||
| $this->content_model = | ||
| str_replace("#SUPER", $this->content_model, $def->content_model); | ||
| str_replace("#SUPER", (string)$this->content_model, $def->content_model); |
There was a problem hiding this comment.
This smells like a (possibly benign) bug. The documentation claims that content_model is always a string, so maybe if it is sometimes null we were supposed to not attempt to do this replacement in that case (maybe #SUPER is never set in those cases)
There was a problem hiding this comment.
Did a quick static analysis: A non-string value for $this->content_model originates from: https://github.com/ezyang/htmlpurifier/blob/master/library/HTMLPurifier/HTMLModule.php#L215
(null is passed to the create method here: https://github.com/ezyang/htmlpurifier/blob/master/library/HTMLPurifier/HTMLModule.php#L157)
| return true; | ||
| } | ||
| if (!ctype_lower($this->unit)) { | ||
| if ($this->unit === false || !ctype_lower($this->unit)) { |
There was a problem hiding this comment.
Shouldn't this be the other way around: if unit is false, then we never need to strtolower the unit
There was a problem hiding this comment.
I was just trying to keep the behavior the same, but you may be right that this was masking a bug
There was a problem hiding this comment.
Maybe the clearest thing to do is simply return false if unit is false here: you've already passed the "zero" cases where "no unit" is allowed: the old behavior was turning the false into the empty string via strtolower and then failing later when looking that empty string up in allowedUnits.
If there's no objection to the construction, probably the "cleanest" version of that change leaves this ctype_lower check alone and instead replaces the immediately previous check with:
if ($this->unit === false) {
return ($this->n === '0');
}
There was a problem hiding this comment.
@zerocrates I like your suggestion, its remove the invalid unit and no error for ctype_lower() appears (in my case version 14).
| if ($config->get('Core.NormalizeNewlines')) { | ||
| $html = str_replace("\r\n", "\n", $html); | ||
| $html = str_replace("\r", "\n", $html); | ||
| $html = str_replace("\r\n", "\n", (string)$html); |
There was a problem hiding this comment.
Feels like type confusion again. Shouldn't have passed in a non-string html to this function in the first place.
There was a problem hiding this comment.
I second that - can't find anything within the source code that passes a non-string value (unless user passes a non-string to tokenizeHTML())
| $this->replace['%n'] = $token ? $token->name : null; | ||
| $this->replace['%m'] = $context->get('CurrentAttr', true); | ||
| $this->replace['%p'] = $context->get('CurrentCSSProperty', true); | ||
| $this->replace['%r'] = $context->get('EmbeddedURI', true) ?: ''; |
There was a problem hiding this comment.
huh, I never knew ?: worked even in PHP 5 LOL
There was a problem hiding this comment.
@ezyang It's the ternary operator or "elvis operator", since PHP 5.3 you can leave out the middle part :) - https://www.php.net/manual/en/language.operators.comparison.php#language.operators.comparison.ternary
|
Any updates to this? |
|
Thanks for the great library. 👍 There's another PHP 8.x warning at: https://github.com/ezyang/htmlpurifier/blob/master/library/HTMLPurifier/AttrTransform/TargetBlank.php#L38 |
|
邮件已收到，谢谢！
|
|
any news? |
- if ($scheme->browsable && !$url->isBenign($config, $context)) {
+ if ($scheme !== false && $scheme->browsable && !$url->isBenign($config, $context)) { |
|
Tested this branch through a standalone build on a few project configurations - have not seen any new errors yet. :) Looking forward to this being merged and 4.15.0 released. |
|
Thanks! |
|
Thank you! |
|
邮件已收到，谢谢！
|
Most of these were caught by the tests. The last commit fixed an issue the tests didn't catch that just came up using the library.