search

WyriHaximus / HtmlCompress

MIT License
78 stars 17 forks source link

Enhancement Request: Remove Dependency on Safe-PHP #120

Open andysnell opened 3 years ago

andysnell commented 3 years ago

This package currently depends on the Safe-PHP library, thecodingmachine/safe, to provide a substr function that throws an exception on failure, instead of returning false. As far as I can see this is only used twice, both within the same function of WyriHaximus\HtmlCompress\Pattern\Style. Safe-PHP works by using Composer file autoloading to redeclare every PHP builtin function and a couple classes under its own namespace. This means that on each and every request, Composer includes ~84 files and over 1,000 userland functions and classes are declared. Luckily, PHP is actually very fast, and with opcaching, the performance impact is minimal -- but there is a performance hit.

There is nothing wrong with using this library, but it really seems to be the kind of dependency that gets required on a per-project basis, and not by other dependencies. It feels off that requiring a dependency to compress html results in duplicate entries for every PHP function in PhpStorm's autocomplete. In fact, I first noticed this during code review when PhpStorm auto-imported one of the \Safe functions instead of the PHP built in -- if not caught, this could/would have introduced a bug into our code. Of course, it's our responsibility to check and test, but again -- this is not really behavior expected from requiring this library in a codebase.

The small performance hit on each request and the injection of 1000+ duplicate-but-namespaced functions does not seem worth the cost of two === false checks.

WyriHaximus commented 3 years ago

Hey @andysnell, you're right and I agree with the big lines of your proposal.

Personally, I'm not too worried about the performance hit because all my projects run @reactphp and thus this is only once per service start. However, for FPM powered services, this would mean that performance hit is there for every request. (Would really like function autoloading in PHP for this.) The performance hit might be mitigated by using 7.4's prefetching, but that isn't guaranteed.

Now my main reason for including it is sheer laziness and https://github.com/thecodingmachine/phpstan-safe-rule that tells me which functions can be swapped to use safe to uncover and hidden failures.

With PHP 8 the substr function changed AFAIK so the code has to be changed anyway and safe isn't needed anymore. If you want, I would gladly fully accept a PR to change this.