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

feat: Use custom HTTP Client, custom Logger, PHPStan #13

Merged
merged 24 commits into from
Feb 5, 2024

Conversation

tyiuhc
Copy link
Collaborator

@tyiuhc tyiuhc commented Dec 28, 2023

Implementation of suggested enhancements.

Updates to support:

  • Custom HTTP Client and Logger - set in RemoteEvaluationConfig, LocalEvaluationConfig, AssignmentConfig
    • HTTP Client (using interface instead of php-http/discovery for more flexibility)
      • Set via the httpClient property
      • The custom client will implement the HttpClientInterface
      • If a custom client is not provided, a default Guzzle-based client is used
        • The default client can be set via the guzzleClientConfig property
    • PSR Logger
      • Set via thelogger property
      • The log level is set via logLevel property
      • If a custom logger is not provided, a default error_log-based logger is used
  • Internal implementation of MurmurHash3
  • Improved testing to use PHPStan; test additional PHP versions
    • Majority of files changed are due to PHPStan style suggestions and not change in implementation

@tyiuhc tyiuhc marked this pull request as ready for review January 16, 2024 16:42
composer.json Outdated
"ext-json": "*",
"guzzlehttp/guzzle": "^7",
"monolog/monolog": "^2"
"psr/log": "^1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since php 8 the standard for logging is PSR-3. Normally it would be enough to add "^1|^2|^3" here and let composer figure it out. However, the interface has changed slightly since PSR-3 and this package has two implementations of the LoggerInterface (InternalLogger and DefaultLogger) which are incompatible with the new PSR.

As far as I know there are three possible solutions:

  • Create two versions of the Default- and InternalLogger and load the correct one based on the loaded PSR
  • Create a separate branch for php 7.4
  • Drop support for php 7.4 (might be premature)

Copy link

@ruudk ruudk Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's easier. I think they can drop support for psr/log v1 and only support ^2 || ^3.

Then, the argument and returns types can be added, and it should just work.

All these projects are able to do it:

$ composer depends psr/log
cloudinary/cloudinary_php   2.12.0                                                 requires psr/log (^1|^2|^3)
composer/xdebug-handler     3.0.3                                                  requires psr/log (^1 || ^2 || ^3)
doctrine/dbal               3.7.3                                                  requires psr/log (^1|^2|^3)
doctrine/migrations         3.7.2                                                  requires psr/log (^1.1.3 || ^2 || ^3)
elastic/transport           v8.8.0                                                 requires psr/log (^1 || ^2 || ^3)
geocoder-php/chain-provider 4.5.0                                                  requires psr/log (^1.0|^2.0|^3.0)
geocoder-php/plugin         1.5.0                                                  requires psr/log (^1.0|^2.0|^3.0)
monolog/monolog             3.5.0                                                  requires psr/log (^2.0 || ^3.0)
php-http/logger-plugin      1.3.0                                                  requires psr/log (^1.0 || ^2 || ^3)
sentry/sentry               3.22.1                                                 requires psr/log (^1.0|^2.0|^3.0)
simple-bus/message-bus      v6.3.1                                                 requires psr/log (^1.1.4 || ^2.0 || ^3.0)
symfony/cache               v7.0.2                                                 requires psr/log (^1.1|^2|^3)
symfony/error-handler       v7.0.0                                                 requires psr/log (^1|^2|^3)
symfony/http-client         v7.0.2                                                 requires psr/log (^1|^2|^3)
symfony/http-kernel         v7.0.2                                                 requires psr/log (^1|^2|^3)
symfony/lock                v7.0.2                                                 requires psr/log (^1|^2|^3)
symfony/messenger           v7.0.1                                                 requires psr/log (^1|^2|^3)

I checked monolog/monolog and it requires ^2 || ^3 and their implementation looks like this:

     /**
     * Adds a log record at the INFO level.
     *
     * This method allows for compatibility with common interfaces.
     *
     * @param string|Stringable $message The log message
     * @param mixed[]           $context The log context
     */
    public function info(string|\Stringable $message, array $context = []): void
    {
        $this->addRecord(Level::Info, (string) $message, $context);
    }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adri
Copy link

adri commented Feb 5, 2024

Just checking, can this be merged? :) We'll use it in production soon and it would be nice to use an actual release.

@tyiuhc tyiuhc merged commit e33e12b into main Feb 5, 2024
6 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 5, 2024
# [0.4.0](0.3.1...0.4.0) (2024-02-05)

### Features

* Use custom HTTP Client, custom Logger, PHPStan ([#13](#13)) ([e33e12b](e33e12b))
Copy link

github-actions bot commented Feb 5, 2024

🎉 This PR is included in version 0.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@tyiuhc
Copy link
Collaborator Author

tyiuhc commented Feb 5, 2024

@adri many thanks for the helpful comments, the changes have been merged into v0.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants