Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove upgradephp #16650

Draft
wants to merge 12 commits into
base: 5.x-dev
Choose a base branch
from
Draft

Remove upgradephp #16650

wants to merge 12 commits into from

Conversation

Findus23
Copy link
Member

@Findus23 Findus23 commented Nov 2, 2020 •
edited by sgiehl

  • utf8_enode/utf8_decode
    should be fine as it is a core part of PHP 7.2+
  • safe_unserialize
    should be possible to replace with Common::safe_unserialize() in all cases
  • safe_serialize
    ???
  • \Error class
    exists in PHP 7+
  • 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 Call to undefined function Piwik\fnmatch()  #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.

@Findus23 Findus23 added Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Nov 2, 2020
@sgiehl
Copy link
Member

sgiehl commented Nov 2, 2020

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
Copy link
Member

sgiehl commented Nov 2, 2020

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

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

@Findus23
Copy link
Member Author

Findus23 commented Nov 2, 2020

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
Copy link
Member

sgiehl commented Nov 11, 2020

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

@sgiehl sgiehl mentioned this pull request Nov 19, 2020
Merged
9 tasks
@sgiehl sgiehl added this to the 4.9.0 milestone Feb 25, 2022
@sgiehl
Copy link
Member

sgiehl commented Feb 25, 2022

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?

I've just checked that one. If file_get_contents is added as disabled function, this piece of code actually crashes with

PHP Fatal error:  Cannot redeclare file_get_contents() in /srv/matomo/libs/upgradephp/upgrade.php on line 137

@justinvelluppillai justinvelluppillai removed this from the 4.9.0 milestone Apr 12, 2022
@Findus23 Findus23 mentioned this pull request Jun 2, 2022
Merged
11 tasks
@sgiehl sgiehl changed the base branch from 4.x-dev to 5.x-dev March 16, 2023 09:42
Findus23 and others added 12 commits March 28, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Findus23 @sgiehl @justinvelluppillai