PHPCS cleanup and PHP cs fixer config #200
Conversation
|
Thanks! -1 to including the license text in the files. It's distracting and apparently for legal reasons it's super difficult to ever remove it in the future. |
|
Can we get the same changes but without the license in the files? :) |
|
Sure, will update .php_cs to remove it too
|
|
Ok, removed licenses and updated php cs fixer config |
| Core::KEY_BYTE_SIZE, | ||
| Core::ENCRYPTION_INFO_STRING, | ||
| $file_salt | ||
| ); |
There was a problem hiding this comment.
Oh god, the diff is going all screwy here. I think it's just whitespace changes but Github isn't highlighting them, so I can't tell if it's just whitespace or if this commit inserts hidden backdoors!
There was a problem hiding this comment.
Does https://github.com/defuse/php-encryption/pull/200/files?w=1 help - the ?w=1 should ignore whitespace changes
There was a problem hiding this comment.
@larowlan: Oh yes, it very much does help, thanks!
There was a problem hiding this comment.
Actually it still screws up displaying the decryptResource stuff :( Oh well
|
LGTM. I was careful to review it for subtly-inserted backdoors and I couldn't find any. Just to be super cautious I'm going to redo the File.php changes myself as I'm merging, since GitHub's diff is glitching and I can't easily see that it's only whitespace changes. (Underhanded Crypto contest entry idea: Find a bug in GitHub's diff algorithm to make it look like whitespace changes when it's not). |
|
|
||
| $thisIv = $iv; |
There was a problem hiding this comment.
It's scary how easy a backdoor could be hidden in all of this!
There was a problem hiding this comment.
Note to others reading: This isn't actually a backdoor, the line isn't actually removed, the diff is just glitching.
|
I just merged @larowlan's PR without applying the changes to @larowlan: Thanks so much for this! The code looks a lot better now :) |
|
I looked over it last night. Per #110 we're going to audit this again before v2.0.0 is tagged, so we'll find out either way. |
|
Neat, thanks
|
|
Structural, functional and styling changes don't belong together in the same patch - that's why it's so hard to review this. |
|
@narfbg: Yeah, I understand. You can still review commit-by-commit. |
|
Well, even if that was the case, I'd still argue in favor of a policy to limit the scope of separate PRs. That's what I was trying to imply here. :) |
Fixes #198
Branched from #197
Changes as follows:
php-cs-fixer fixfrom project root after installing globally.