Dev 123965 general improvements fixes#97
Conversation
…Riskified/php_sdk into DEV-123965_general-improvements_fixes
…Riskified/php_sdk into DEV-123965_general-improvements_fixes # Conflicts: # src/Riskified/DecisionNotification/Model/Notification.php # src/Riskified/polyfill.php
| class BaseException extends \Exception { | ||
| } |
There was a problem hiding this comment.
Can we configure the opposite/keep as it was for these empty classes?
| // @phpstan-ignore deadCode.unreachable | ||
| break; | ||
| case 'array': | ||
| return $this->validate_array($key, $types, $value); | ||
| // @phpstan-ignore deadCode.unreachable |
There was a problem hiding this comment.
If this is dead/unreachable code, why not remove it?
| $exceptions = array(); | ||
| foreach ($this->_fields as $propertyName => $constraints) { | ||
| $types = explode(' ', $constraints); | ||
| // phpcs:ignore Generic.PHP.ForbiddenFunctions.Found -- is_null() is intentional here |
There was a problem hiding this comment.
If we're setting is_null as forbidden, then we need to change the code or remove it as forbidden.
Replacing is_null with x === null, is more performant, but not enough to be relevant, so I'd say remove it as forbidden.
| foreach($array as $key => $value) { | ||
| if (is_null($value)) | ||
| foreach ($array as $key => $value) { | ||
| // phpcs:ignore Generic.PHP.ForbiddenFunctions.Found -- is_null() is intentional here |
There was a problem hiding this comment.
| /** | ||
| * Update a merchant's settings | ||
| * @param hash object named 'settings' with a key-value structure | ||
| * @param $settings object named 'settings' with a key-value structure |
There was a problem hiding this comment.
Why the change?
| // @phpstan-ignore deadCode.unreachable | ||
| return null; |
| } | ||
|
|
||
| $status = curl_getinfo($ch, CURLINFO_HTTP_CODE); | ||
| // phpcs:ignore Generic.PHP.DeprecatedFunctions.Deprecated -- kept for PHP < 8.0 where curl_close() is still required |
There was a problem hiding this comment.
If possible, can we have it be ignore in the configuration? That way, if we change config, we get the warning
| } | ||
|
|
||
| $status = curl_getinfo($ch, CURLINFO_HTTP_CODE); | ||
| // phpcs:ignore Generic.PHP.DeprecatedFunctions.Deprecated -- kept for PHP < 8.0 where curl_close() is still required |
There was a problem hiding this comment.
If possible, can we have it be ignore in the configuration? That way, if we change config, we get the warning
| $this->assertStringContainsString('"settings"', $json); | ||
| $this->assertStringContainsString('"notify_url":"https://example.com/hook"', $json); |
There was a problem hiding this comment.
Why was this test changed?
Not that it is a major change, just that we're now doing a decode unnecessarily.
| // Register Riskified autoloader into the SPL autoloading stack (in order to support multiple autoloaders) | ||
| spl_autoload_register('riskifiedAutoload',null,true); | ||
| // @phpstan-ignore argument.type | ||
| spl_autoload_register('riskifiedAutoload', null, true); |
There was a problem hiding this comment.
The second param of this is a boolean. It's just ignored on PHP >=8.0.0 ref
2 participants
Summery
Follow-up cleanup on top DEV-123965_general-improvements of that gets the codebase to a green composer check (lint + static analysis) and a fully passing test suite. This is a code-quality / standards pass — no changes to runtime behavior or the public API.
what changed?
composer analyseto run code analysis and fixed findingscomposer lintandcomposer fixto run lint and fixed findingsNote
these changes are just those fixes.. no code shuold have change
Relates to #98