search

smsapi / smsapi-php-client

SMSAPI PHP client that allows you to send messages, manage Short URLs and administrate your SMSAPI account.
https://www.smsapi.pl
Other
64 stars 39 forks source link

Update psr/log #100

Closed snapshotpl closed 2 years ago

snapshotpl commented 2 years ago

@maciejlew when we can expect review, merge and release?

ProteanCode commented 2 years ago

We need this in newer Symfony-based frameworks

snapshotpl commented 2 years ago

I have remove travis and added github actions config, but I need to enable it in repo config.

maciejlew commented 2 years ago

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?

snapshotpl commented 2 years ago

Can you check if GH Actions are enabled here https://github.com/smsapi/smsapi-php-client/settings/actions ?

snapshotpl commented 2 years ago

Oh, see now you have already check this. Strange, workflow should works now

snapshotpl commented 2 years ago

Just added sample workflow from https://docs.github.com/en/actions/quickstart and still nothing. Probably something still need to be setting up.

snapshotpl commented 2 years ago

Now working and all test pass!

maciejlew commented 2 years ago

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.

snapshotpl commented 2 years ago

Check now.

I think we should run integration test on CI, you can provide token by secret, so will be not shared.

snapshotpl commented 2 years ago

LoggerAware is fixed, but found custom test logger now.

snapshotpl commented 2 years ago

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.

maciejlew commented 2 years ago

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.

maciejlew commented 2 years ago

Drop TestLogger.