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.

Sign up for GitHub

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

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

Merged
merged 24 commits into from
Feb 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
e57099f
feat: Allow custom HTTP client and PSR-logger
tyiuhc Dec 21, 2023
0faa2e7
remove murmur3 hash dependency
tyiuhc Dec 21, 2023
4a187eb
update test.yml with additional PHP versions
tyiuhc Dec 21, 2023
dca6c02
Update to use PHPStan
tyiuhc Dec 28, 2023
6d5149d
Update comments and README
tyiuhc Dec 29, 2023
ffad979
nit: improve description
tyiuhc Dec 29, 2023
1e5d2ce
update return type of build() in amplitude and assignment config
tyiuhc Dec 30, 2023
ba3ca2b
fix assignment tracking
tyiuhc Dec 30, 2023
c888204
fix unit test utils
tyiuhc Dec 30, 2023
4d5ab89
improve FetchClientInterface comments
tyiuhc Dec 30, 2023
3e6117a
nit: code style
tyiuhc Dec 31, 2023
b6bfd50
fix: GuzzleFetchClient delay function
tyiuhc Jan 8, 2024
a0d7a11
docs: update descriptions
tyiuhc Jan 8, 2024
af5d592
nit: remove echo
tyiuhc Jan 9, 2024
b416164
refactor: custom HTTP client params naming
tyiuhc Jan 9, 2024
939c1ec
refactor: rename fetchClient vars to httpClient
tyiuhc Jan 9, 2024
ac58d23
refactor: update log levels
tyiuhc Jan 9, 2024
942bccf
refactor: update log level default
tyiuhc Jan 9, 2024
24dd6bf
docs: improve HttpClientInterface descriptions
tyiuhc Jan 11, 2024
48815ef
refactor: update log level name
tyiuhc Jan 16, 2024
cb35c52
refactor: remove unneeded import
tyiuhc Jan 16, 2024
48f682a
Allow all versions of PSR log
tyiuhc Jan 23, 2024
505c633
update LocalEvaluationClient - add getFlagConfigs method, change meth…
tyiuhc Jan 24, 2024
5aa4295
fix: update hashCode util function to use explicit int cast
tyiuhc Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,43 @@ name: Test
on:
pull_request:
push:
branches: [ "main" ]

permissions:
contents: read
branches:
- main

jobs:
build:

runs-on: ubuntu-latest
test:
strategy:
fail-fast: false
matrix:
php-version: ['7.4', '8.0', '8.1', '8.2', '8.3']
os: [ubuntu-latest]
runs-on: ${{ matrix.os }}

steps:
- uses: actions/checkout@v3

- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: 7.4
- name: Checkout
uses: actions/checkout@v2

- name: Validate composer.json and composer.lock
run: composer validate --strict

- name: Cache Composer packages
id: composer-cache
uses: actions/cache@v3
uses: actions/cache@v2
with:
path: vendor
key: ${{ runner.os }}-php-${{ hashFiles('**/composer.lock') }}
key: ${{ runner.os }}-php-${{ matrix.php-version }}-${{ hashFiles('**/composer.lock') }}
restore-keys: |
${{ runner.os }}-php-

- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php-version }}

- name: Install dependencies
run: composer install --prefer-dist --no-progress

- name: Run test suite
run: composer run-script test

- name: Run PHPStan
run: vendor/bin/phpstan analyse --ansi
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ $user = \AmplitudeExperiment\User::builder()
->userProperties(['premium' => True])
->build();

$variants = $client->fetch($user)->wait();
$variants = $client->fetch($user);

// (3) Access a flag's variant
$variant = $variants['FLAG_KEY'] ?? null;
Expand All @@ -49,7 +49,7 @@ $experiment = new \AmplitudeExperiment\Experiment();
$client = $experiment->initializeLocal('<DEPLOYMENT_KEY>');

// (2) Start the local evaluation client.
$client->start()->wait();
$client->start();

// (3) Evaluate a user.
$user = \AmplitudeExperiment\User::builder()
Expand Down
9 changes: 6 additions & 3 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,16 @@
"prefer-stable": true,
"require": {
"php": "^7.4 || ^8",
"lastguest/murmurhash": "^1",
"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 •
edited
Loading

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.

Here the fixes we needed to apply to make it work for us:
https://github.com/amplitude/experiment-php-server/compare/amplitude:custom-enhancement...LVoogd:custom-enhancement?expand=1

},
"require-dev": {
"phpunit/phpunit": "9.*"
"phpunit/phpunit": "9.*",
"phpstan/phpstan": "^1"
},
"suggest": {
"monolog/monolog": "Allows more advanced logging of the application flow"
},
"autoload": {
"psr-4": {
Expand Down
Loading
Loading