Update psr/log #100
Update psr/log #100
Conversation
|
@maciejlew when we can expect review, merge and release? |
|
We need this in newer Symfony-based frameworks |
| @@ -18,7 +18,7 @@ | |||
| "require": { | |||
| "php": "^7 || ^8.0", | |||
| "ext-json": "*", | |||
| "psr/log": "^1", | |||
| "psr/log": "^1 || ^2 || ^3", | |||
There was a problem hiding this comment.
Run make test-php8 and you will see:
Fatal error: Declaration of Smsapi\Client\Curl\SmsapiHttpClient::setLogger(Psr\Log\LoggerInterface $logger) must be compatible with Psr\Log\LoggerAwareInterface::setLogger(Psr\Log\LoggerInterface $logger): void in /app/src/Curl/SmsapiHttpClient.php on line 33
To fix that you have to drop PHP 7.0 support. Do that and I will release next major version with your changes.
|
I have remove travis and added github actions config, but I need to enable it in repo config. |
|
I'm not sure what action to be taken you expect from me. GitHub Actions are enabled as described here: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#about-github-actions-permissions-for-your-repository, however, I don't see any approve workflow run request as described here: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks. Is that what you expect? Maybe that approve workflow run do not exist because there is no workflow yet? |
|
Can you check if GH Actions are enabled here https://github.com/smsapi/smsapi-php-client/settings/actions ? |
|
Oh, see now you have already check this. Strange, workflow should works now |
|
Just added sample workflow from https://docs.github.com/en/actions/quickstart and still nothing. Probably something still need to be setting up. |
|
Now working and all test pass! |
|
Glad to see that. I'm not sure what has helped, I haven't changed any setting. Maybe one of my PRs with workflow triggered something. Anyway, despite that unit tests are passing, there is still problem with integration ones. HTTP client is being produced and error I've mentioned before occur only when running integration tests. Check out SmsapiClientIntegrationTestCase::prepare. To run integration test you have to pass access token and that is the reason why we do not execute them here. You can configure that tests locally, in your developement environment, even passing incorrect access token, and you will see the error. |
|
Check now. I think we should run integration test on CI, you can provide token by secret, so will be not shared. |
|
LoggerAware is fixed, but found custom test logger now. |
|
I think we should avoid current TestLogger - it's just echo debug information so looks like only for development proces. The same can be done by xdebug. If we want to break version, then we need to release client 4 with log v2 and client 5 with log v3. So I prefer to drop TestLogger. |
|
Running integration tests seems not to be a good decision for test stability and security reasons. Test may fail and test cleanup may not execute. That will make impact on future test runs. Someone may write malicious test code that, for example, may send spam using our test account. Test are now being executed on each pull request so code review will always happen after the fact. Even not accepted pull request may cause security breach. |
|
Drop TestLogger. |
No description provided.