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

Improve code by using current standards on strict types #122

Closed
wants to merge 4 commits into from
Closed

Improve code by using current standards on strict types #122

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 17, 2023

Description:

I took the time to upgrade the code to current PHP 8.x coding standards but without causing any issues with older PHP versions.

  • I've tested the improved code with PHP 7.2, 7.3, 7.4, 8.0, 8.1 and 8.2.
  • I've run PHPStan at level 8 and received no errors or warnings.
  • I've kept the original function parameters so as not to cause any breakage.
  • Unfortunately some types are wrong and had to be fixed, this may cause some issues.

The old code had a lot of issues with mixed types, like testing for empty on boolean values and expecting a string concatenation. I avoided doing changes to the code that did not directly relate to strict types, my main objective is to have a good starting point for the next generation of code. My next objective is to format the code to PSR standards, so the code is more readable.

Please let me know what you think about these changes.

Thank you.

Review

@ghost
Copy link
Author

ghost commented Aug 19, 2023

For those who want to run PHPStan, here is the configuration file I use:

parameters:
    level: 8
    tipsOfTheDay: false
    checkMissingIterableValueType: false
    reportUnmatchedIgnoredErrors: true
    polluteScopeWithLoopInitialAssignments: false
    polluteScopeWithAlwaysIterableForeach: false
    checkExplicitMixedMissingReturn: true
    checkFunctionNameCase: true
    checkInternalClassCaseSensitivity: true
    reportMaybesInMethodSignatures: true
    reportMaybesInPropertyPhpDocTypes: true
    reportStaticMethodSignatures: true
    checkUninitializedProperties: true
    rememberPossiblyImpureFunctionValues: true
    treatPhpDocTypesAsCertain: false
    #phpVersion: 70200
    #phpVersion: 70300
    #phpVersion: 70400
    #phpVersion: 80000
    #phpVersion: 80100
    phpVersion: 80200

    paths:
        - ./MatomoTracker.php

Save it as phpstan.neon in the same directory as your MatomoTracker.php file.

Run with:

phpstan analyse --memory-limit 1G -c ./phpstan.neon

@ghost
Copy link
Author

ghost commented Aug 29, 2023

PSR formatting. I did not follow all the rules, because that would require to remove the extra commands at the bottom of the file, PSR requires that a loadable class does NOT contain extra code outside the class.

…s unclear why this was placed here since it causes problems.
@ghost
Copy link
Author

ghost commented Aug 29, 2023

Removed undocumented RuntimeException that causes problems, fixes bug #88.

@ghost
Copy link
Author

ghost commented Aug 29, 2023

Remove static stuff, make the class more compatible with object oriented designs, fixes bug #14.

@marineusde
Copy link

Very good work!

But I have one little question:

  • why do you make all the attributes public and not protected? There are some intelligent setters where you can add some more attributes for one type, like setPerformanceTimings()

A succession maybe for a discussion:

  • add phpstan in the composer.json to use it in the run_tests.sh for the pipeline in gitlab
  • just use PHP 8.0+ or 8.1 cause PHP < 8.0 is not supported anymore and < 8.1 in 2 month. After the break we can use types directly for the attributes and methods. If you want, I can do this.
  • a version upgrade to 3.x for all these changes

@ghost
Copy link
Author

ghost commented Sep 6, 2023

Thank you for your kind words.

About the public attributes. It is a great question and I'd appreciate your suggestions, my thinking is this: keep everything backward compatible (as much as possible), since the attributes did not appear as protected in the first place I let them be public. So we have both worlds: $object->requestTimeout = 1; and $object->setRequestTimeout(1); both do the same thing.

About PHP version support. Currently (matomo 4.x) requires PHP v7.2.5 or greater, maybe we need to keep that as a minimum? or is this project independent? Of course my personal preference is PHP v8.0 as a minimum because that is supported by Enterprise Linux distros (RHEL 8.x, Alma 8.x, Rocky 8.x, Oracle 8.x).

Is there a review procedure for my changes? 🥲

@marineusde
Copy link

I think, the Matomo-Project is not the Tracker. The code would be much better and easier with PHP 8+. The most of the popular PHP-projects need 8.x+ now cause its not safe to use older versions.

@ghost ghost mentioned this pull request Sep 9, 2023
@ghost ghost closed this by deleting the head repository Dec 27, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant