nemiah / phpFinTS

PHP library to communicate with FinTS/HBCI servers
MIT License
131 stars 40 forks source link

fix #384 NullLogger Inheritance #385

Closed lukas-staab closed 2 years ago

lukas-staab commented 2 years ago

Wrapped the PSR NullLoger in the FinTs Log Sanitizer

lukas-staab commented 2 years ago

I am a bit confused. CI says there was a fix, but on my machine php81 composer.phar cs-fix does not result in a comittable change (skipping all, also with deleted cache).

lukas-staab commented 2 years ago

local:

sh csfixer-check.sh 
Variable TRAVIS_COMMIT_RANGE not set, falling back to full git diff
PHP CS Fixer 3.6.0 Roe Deer by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 8.1.8
Loaded config default from ".php-cs-fixer.php".
.                                                                                                                                                                                                                                           1 / 1 (100%)
Legend: ?-unknown, I-invalid file syntax (file ignored), S-skipped (cached or empty file), .-no changes, F-fixed, E-error

Checked all files in 0.568 seconds, 8.000 MB memory used

feels a bit like a broken CI tbh, composer cs-fix also finds nothing

Philipp91 commented 2 years ago

Try composer install first to make sure you have the same version as Travis. For me it produces a bunch of diffs. I'll upload them as a separate PR (#386), so even if you can't get php-cs-fixer to work on your end (which would be somehow sad), you could rebase on that.

In particular, I believe it complains about a line you're not even touching, so you're just unlucky to touch a file that contains a line that a new php-cs-fixer version complains about while previous ones didn't.

lukas-staab commented 2 years ago

I found the issue. composer.lock is not committed. It is even in the .gitignore

Usually it is commited see here why: (first google results) https://stackoverflow.com/a/12896850/4609612 https://www.codementor.io/@decodeweb/importance-of-composer-lock-in-git-14v9ujxn9z#why-should-we-commit-composerlock-as-well

Is there a reason it is not committed, if not, i would prefer to add it. I did not expect it would be necessary to run a composer update to get to the (then kind of randomly changing) state the CI has. If we leave a pull request long enough hanging it could change to failing with the current setup, just because there is a new version of cs-fix

Edit: also the composer documentation suggests to add it to version control https://getcomposer.org/doc/01-basic-usage.md#commit-your-composer-lock-file-to-version-control Edit Edit: for libaries it is more vague: https://getcomposer.org/doc/02-libraries.md#lock-file

lukas-staab commented 2 years ago

Try composer install first to make sure you have the same version as Travis. For me it produces a bunch of diffs. I

That is because there is no .lock file if you pull it fresh (like travis or you did). If you composer installed once before you have a local .lock file and composer will not fetch the updates like it does without the .lock file. Then you explicitly need to call composer update as well.

I am fine with not adding the .lock file it it made difficulties somewhere else, but then it should be added to the contribution guidelines at least, that you might need a composer update or a fresh install every time you want to contribute.

Philipp91 commented 2 years ago

I'd be in favor of adding the lock file to git.

However, this lock file will not have any effect on other projects that depend on it. It only has an effect on the main project.

That's only a thing to be aware of. Maybe a different minor version of a dependent library is incompatible with our code and we wouldn't find out. But that can also happen without the lock file, as we can never guarantee that Travis runs the minor revision that our clients use.