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.

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

Strengthen type checking in AuditTrait #228

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

janklan
Copy link
Contributor

@janklan janklan commented Oct 29, 2024

Fixes #227

I'm between two minds whether (a) doing what I did is better than (b) tweaking AuditTrait::deepDiff() so that it can handle non-arrays. But then it's not a deep diff anymore, so changing the signature would justify changing the name. I went with (a) because it does the job, and the only other place where deepDiff is used already enforces the correct data type.

@janklan janklan force-pushed the bugfix/json-string branch from 6f4b028 to 088ebb1 Compare October 29, 2024 22:31
@DamienHarper
Copy link
Owner

DamienHarper commented Oct 31, 2024

Thanks @janklan, would you mind applying PHP-CS-Fixer (make cs-fix or composer cs-fix) and update your PR please?

@DamienHarper
Copy link
Owner

Also, a test that emphasizes your fix would be greatly appreciated ;)

@DamienHarper
Copy link
Owner

Thanks @janklan, would you mind applying PHP-CS-Fixer (make cs-fix or composer cs-fix) and update your PR please?

I did it for you ;)

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.81%. Comparing base (94e297d) to head (148f77d).
Report is 21 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   97.15%   96.81%   -0.34%     
==========================================
  Files          42       42              
  Lines        1721     1792      +71     
==========================================
+ Hits         1672     1735      +63     
- Misses         49       57       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DamienHarper DamienHarper merged commit 8077b2d into DamienHarper:master Oct 31, 2024
20 checks passed
@janklan
Copy link
Contributor Author

janklan commented Nov 4, 2024

Hey Damien, thanks. TBH I did want to run the CS fixer etc., but I didn't get it working in the time I could spend on it on that day. I planned to get back to it later, so thanks for being faster and doing it instead of me!

Does the cs fix tooling work, though? I tried the composer method and got yelled at because of old PHP CS Fixer not liking PHP 8.3, and the makefile method fails the same. BTW the Makefile is not mentioned anywhere, I didn't even notice it until now - poor attention on my end, I know, but maybe the contribution guide may need a bump.

I'm getting:

janklan@Mac auditor % make cs-fix
Changed current directory to /root/.composer
PHP_VERSION=8.3 SYMFONY_VERSION=7.1 \
	sh -c "docker compose -f tools/docker/compose.yaml run --rm --remove-orphans php-cli tools/php-cs-fixer/vendor/bin/php-cs-fixer fix --using-cache=no --verbose --ansi"
PHP needs to be a minimum version of PHP 7.4.0 and maximum version of PHP 8.1.*.
Current PHP version: 8.3.13.
To ignore this requirement please set `PHP_CS_FIXER_IGNORE_ENV`.
If you use PHP version higher than supported, you may experience code modified in a wrong way.
Please report such cases at https://github.com/FriendsOfPHP/PHP-CS-Fixer .
make: *** [cs-fix] Error 1

@DamienHarper
Copy link
Owner

Hi @janklan!
I haven't had time to update the documentation, that's why the makefile is not mentioned anywhere now.
BTW, the cs-fix tooling should work :/ it works well on several computer on which I use it regularly, that's strange.
Please, could you try to remove all the docker volumes and images named auditor* and retry the make cs-fix command?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken JSON change detection when the JSON value is not an array.
2 participants