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

Bugfix/serializable validation errors #312

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mjervis
Copy link

@mjervis mjervis commented Feb 4, 2022

Summary

Checklist

  • [* ] Added changelog entry
  • [x ] Ran unit tests (Check the README for instructions)
php ./vendor/bin/phpcs --config-set show_progress 1
Using config file: /braintree-php/vendor/squizlabs/php_codesniffer/CodeSniffer.conf

Config value "show_progress" added successfully
php ./vendor/bin/phpcs --config-set colors 1
Using config file: /braintree-php/vendor/squizlabs/php_codesniffer/CodeSniffer.conf

Config value "colors" added successfully
php ./vendor/bin/phpcs --config-set php_version 70300
Using config file: /braintree-php/vendor/squizlabs/php_codesniffer/CodeSniffer.conf

Config value "php_version" added successfully
php ./vendor/bin/phpcs --standard=phpcs.xml --report=summary -s lib tests
............................................................  60 / 279 (22%)
............................................................ 120 / 279 (43%)
............................................................ 180 / 279 (65%)
............................................................ 240 / 279 (86%)
.......................................                      279 / 279 (100%)


Time: 4.52 secs; Memory: 66.01MB

php ./vendor/bin/phpunit --testsuite unit --do-not-cache-result
PHPUnit 9.5.13 by Sebastian Bergmann and contributors.

...............................................................  63 / 360 ( 17%)
............................................................... 126 / 360 ( 35%)
........................................................PHP Deprecated:  Return type of Braintree\Error\Validation::jsonSerialize() should either be compatible with JsonSerializable::jsonSerialize(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /braintree-php/lib/Braintree/Error/Validation.php on line 50
....... 189 / 360 ( 52%)
............................................................... 252 / 360 ( 70%)
............................................................... 315 / 360 ( 87%)
.............................................                   360 / 360 (100%)

Time: 00:00.372, Memory: 12.00 MB

OK (360 tests, 1077 assertions)
  • [x ] I understand that unless this is a Draft PR or has a DO NOT MERGE label, this PR is considered to be in a deploy ready state and can be deployed if merged to main

@mjervis mjervis requested a review from a team as a code owner February 4, 2022 09:15
Copy link
Contributor

@crookedneighbor crookedneighbor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. Just left a few minor styling suggestions.

CHANGELOG.md Outdated Show resolved Hide resolved
lib/Braintree/Error/Validation.php Outdated Show resolved Hide resolved
@crookedneighbor
Copy link
Contributor

Though this warning in the console seems like it should be addressed:

PHP Deprecated: Return type of Braintree\Error\Validation::jsonSerialize() should either be compatible with JsonSerializable::jsonSerialize(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /braintree-php/lib/Braintree/Error/Validation.php on line 50

@mjervis
Copy link
Author

mjervis commented Feb 7, 2022

Fixed the code style issues, not sure how I got those in TBH , I'd run the linter.

How did you get the console error? Running the unit tests and the tests doesn't throw these up?

I've fixed it inline with the other JsonSerializeable methods in the existing codebase.

Comment on lines +53 to +57
return array(
'code' => $this->__get('code'),
'attribute' => $this->__get('attribute'),
'message' => $this->__get('message')
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return array(
'code' => $this->__get('code'),
'attribute' => $this->__get('attribute'),
'message' => $this->__get('message')
);
return [
'code' => $this->__get('code'),
'attribute' => $this->__get('attribute'),
'message' => $this->__get('message'),
];

@hollabaq86
Copy link
Contributor

hey folks, thanks for the PR! We'll take a look and provide feedback/merge.

For braintree folks, ticket 2047

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

Successfully merging this pull request may close these issues.

5 participants