Remove thecodingmachine/safe dependency#1484
Remove thecodingmachine/safe dependency#1484SjorsO wants to merge 2 commits intoMyIntervals:mainfrom
thecodingmachine/safe dependency#1484Conversation
| } | ||
| return iconv('utf-32le', $this->charset, $utf32EncodedCharacter); | ||
| /** @phpstan-ignore theCodingMachineSafe.function */ | ||
| return \iconv('utf-32le', $this->charset, $utf32EncodedCharacter); |
There was a problem hiding this comment.
The ?string typehint on this method will throw if iconv returns false
| if (preg_match($expression, $input, $matches, PREG_OFFSET_CAPTURE) !== 1) { | ||
| /** @phpstan-ignore theCodingMachineSafe.function */ | ||
| if (\preg_match($expression, $input, $matches, PREG_OFFSET_CAPTURE) !== 1) { | ||
| throw new UnexpectedTokenException($expression, $this->peek(5), 'expression', $this->lineNumber); |
There was a problem hiding this comment.
No need to check for false here. If this preg_match returns false then we'll already throw an exception
src/Property/Selector.php
Outdated
| // Whitespace can't be adjusted within an attribute selector, as it would change its meaning | ||
| $this->selector = !$hasAttribute ? preg_replace('/\\s++/', ' ', $selector) : $selector; | ||
| /** @phpstan-ignore theCodingMachineSafe.function */ | ||
| $this->selector = !$hasAttribute ? \preg_replace('/\\s++/', ' ', $selector) : $selector; |
There was a problem hiding this comment.
preg_replace cannot return false. The dependency has preg_replace as a special case.
I think it should be fine to use the normal version of preg_replace
There was a problem hiding this comment.
According to https://www.php.net/manual/en/function.preg-replace.php#refsect1-function.preg-replace-returnvalues, it can return a string, an array or null.
There was a problem hiding this comment.
You're right, I missed that. I've pushed a commit that checks for null
| /** @phpstan-ignore theCodingMachineSafe.function */ | ||
| preg_match('/\\s/', $parserState->peek(1, -1)) !== 1 | ||
| /** @phpstan-ignore theCodingMachineSafe.function */ | ||
| || preg_match('/\\s/', $parserState->peek(1, 1)) !== 1 |
There was a problem hiding this comment.
if either of these returns false we'll already throw an exception
|
Appreciate your efforts, @SjorsO (code changes look solid), but I honestly don't think this is the best way forward. I think we need to resolve the bloat in the in the 'Safe' package, for which I created thecodingmachine/safe#706, and have had a response. |
Given how widely used PHP-CSS-Parser and emogrifier are, I consider getting rid of any dependency a huge win. Even if I think this PR is a nice middle ground: no dependency that thousands of people have to install, but still a PHPStan rule that warns about unsafe usage during development. |
oliverklee
changed the title
thecodingmachine/safe dependency (2)thecodingmachine/safe dependency
|
thecodingmachine/safe#660 is another reason to avoid using this dependency. In WordPress, each plugin (or theme) brings in its own versions of each dependency, which leads to conflicts, though, in fairness, that is an issue with WordPress. |
|
Marking this as draft as we're currently working to drastically shrink that package in thecodingmachine/safe#706. We should revisit this PR here once a new version of Safe has been released. |
With version 3.4, the size is reduced from 6.31Mb to 1.58Mb. To put it in context, this is comparable to the size of one or two photos in HD quality. Another factor that impacts website managers is the number of individual files. Hosting providers using cPanel usually impose a limit on the number of file handles. The new version reduces the number of files from 573 to 318. The new version is only availble for PHP versions >=8.1, and I don't expect will be backported - the older PHP versions are already EOL. Where do we go from here? I have no strong opinion, but don't see the download size and number of files as a significant impact, and would prefer to use the library to keep the code robust and clean, without having to implement additional checks and tests. |
I fully agree. |
|
For the record, I still strongly feel that adding 1.58mb and 318 files to thousands (millions?) of applications is way too much. Removing this dependency is a good step to prevent everyone's |
4 participants
This PR achieves the same as #1482 but with a different approach.
I've kept the
thecodingmachine/phpstan-safe-rulePHPStan rule. I've disabled this rule for test classes, I don't think these unsafe functions are a concern in unit testing code.The maintenance burden of this PR is lower than my previous PR. With this approach, any new code that uses unsafe functions will fail the PHPStan check. This code can then be changed to explicitly check for
false, and then the PHPStan error can be ignored with/** @phpstan-ignore theCodingMachineSafe.function */