@Findus23 opened this Pull Request on November 2nd 2020 Member
  • [x] utf8_enode/utf8_decode
    should be fine as it is a core part of PHP 7.2+
    • [x] safe_unserialize
      should be possible to replace with Common::safe_unserialize() in all cases
    • [ ] safe_serialize
      ???
    • [x] \Error class
      exists in PHP 7+
    • [x] PHP_INT_SIZE/PHP_INT_MAX
      exist since PHP 5.0.5
    • [ ] mysqli_set_charset
      exist since early PHP 5
    • [ ] parse_ini_file()
      as matomo-org/component-ini should handle this well. Might be an issue if parse_ini_file is used anywhere directly (but shouldn't be the case. Maybe also check if this implementation had fixes that are missing in the component
    • [ ] glob/_glob
      is used in many places in Matomo. glob is checked as an optional function in the system check
    • [ ] file_get_contents
      I'm a bit confused by this polyfill. Wouldn't this break checks if file_get_contents was disabled and this was used instead to download files?
    • [ ] _readfile
      Is used by ProxyHttp::serverStaticFile, but only advantage seems to be supporting partial files (which is used nowhere in Matomo)
    • [ ] mb_strtolower/mb_strlen
      possibly still needed. Maybe find a better place for them
    • [ ] gzopen64
      this seems to only be a bug with a specific 5.x release
    • [ ] dump
      it seems like this is used to silence the dump() function from development (var-dumper) during release, but it is used nowhere
    • [ ] fnmatch
      most likely still needed to avoid #11237

This is mostly a proof of concept. It would be great if someone who knows more about why these functions exist could take over.

@sgiehl commented on November 2nd 2020 Member

safe_serialize

Guess we could simply use serialize instead, as we always should use safe_unserialize, which then should block unseralizing bad objects. Or we introduce a Common::safe_serialize, that similar to safe_unserialize takes allowed_classes as new param and blocks all other objects from being serialized.

@sgiehl commented on November 2nd 2020 Member

mb_strtolower/mb_strlen
possibly still needed. Maybe find a better place for them

I'd suggest to require symfony/polyfill-mbstring instead

@Findus23 commented on November 2nd 2020 Member

I'd suggest to require symfony/polyfill-mbstring instead

That's a good idea. As it is already a dependency of twig adding it explicitly shouldn't even add any code.

@sgiehl commented on November 11th 2020 Member

@Findus23 might maybe make sense to split that PR into multiple parts and remove that stuff step by step.

Powered by GitHub Issue Mirror