-
Notifications
You must be signed in to change notification settings - Fork 20
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
Log warnings that are on the second level also #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. A couple of comments.
Note that you could also add a new test, perhaps with the duplicate-upload warning as given in the ticket; that'd make sure this was fixed correctly. :-)
src/MediawikiApi.php
Outdated
} | ||
|
||
foreach ( $value['warnings'] as $module => $warningData ) { | ||
// Accomodate both formatversion=2 and old-style API results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section should not be duplicated from above. Instead, you could a new protected method logWarning()
and call it from each of these places.
src/MediawikiApi.php
Outdated
$this->logger->warning( $logPrefix . $warningData['warnings'], [ 'data' => $warningData ] ); | ||
} | ||
|
||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're returning after the first iteration of the loop. What about if there are multiple warnings?
src/MediawikiApi.php
Outdated
|
||
// ...if no then go one level deeper and check there for them. | ||
foreach ( $result as $value ) { | ||
if ( !array_key_exists( 'warnings', $value ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$value
can be a string (see test results).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Just that little coding-standard fix that Travis is complaining about, and one minor comment.
src/MediawikiApi.php
Outdated
* | ||
* @return bool Whether any warning has been logged or not. | ||
*/ | ||
private function logWarning( $array ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be proteccted, in case subclasses want to do something different with it.
Bug: T177191
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. @addshore are you okay with this?
Bug: T177191
/cc @samwilson