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

Use native_function_invocation rule #858

Merged
merged 1 commit into from
Feb 4, 2020

Conversation

ob-stripe
Copy link
Contributor

r? @brandur-stripe @richardm-stripe
cc @stripe/api-libraries

Enables the native_function_invocation rule.

Replaces #806.

->setRules([
'@PHP56Migration:risky' => true,
'@PHPUnit57Migration:risky' => true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've already run the fixes for those two rules in #846 and #847. Adding them to the configuration file ensures any new code will also match those rules.

$logger = new DefaultLogger();
$logger->error("message");
// DefaultLogger uses PHP's `error_log` function. In order to capture
// the output, we need to temporarily redirect it to a temporary file.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to completely rewrite this test, as previously it was using a hack where we manually defined an error_log() function in the \Stripe\Util namespace and relied on the fact that DefaultLogger called error_log() in its non-qualified form.

The new test actually captures the output of PHP's \error_log() function.

} finally {
\ini_set('error_log', $origErrorLog);
\fclose($capture);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While running the test suite, I noticed that this test was printing "Stripe Notice: Undefined property of Stripe\StripeObject instance: nonexistent" to stdout. I used the same technique as DefaultLoggerTest to capture the output. This has two upsides: the test suite's output is clean, and we actually test the behavior that accessing a non-existent property outputs a notice to the logger.

Copy link
Contributor

@richardm-stripe richardm-stripe left a comment

Choose a reason for hiding this comment

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

lgtm

@ob-stripe ob-stripe merged commit 71b97a3 into master Feb 4, 2020
@ob-stripe ob-stripe deleted the ob-native-function-invocation branch February 4, 2020 17:41
@ob-stripe
Copy link
Contributor Author

Released as 7.23.0.

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.

4 participants