Conversation
* the `BeforePageDisplay` hook handler does nothing, remove it * the JavaScript in the `ext.statusCheck` ResourceLoader module defines a `mw.statusCheck` placeholder - remove it, clean up the module definition, and update SpecialStatusCheck to use `OutputPage::addModuleStyles()` rather than `::addModules()` * remove unused `.gitreview` file
Have BaseCheck store message keys for the check description and the mitigation suggestion, and implement `BaseCheck::getDescription()` and `BaseCheck::getMitigationSuggestion()` to return the messages with those keys. Convert the individual checks to pass the message keys in a constructor call and remove the `::getDescription()` and `::getMitigationSuggestion()` methods from the checks.
Because the default merge strategy for configuration arrays is `array_merge`, this could only be used to *add* additional checks, not to adjust the checks in the default set. Its existence also suggests that we want site administrators to directly provide check classes, which should not be supported yet. Remove the configuration option, and hard-code the list of default checks in `ChecksRunner::DEFAULT_CHECKS`. The migration to storing the list in PHP will also allow for switching to ObjectFactory specs.
Convert the list of checks to run to a list of ObjectFactory specifications to allow injecting services, and replace direct uses of MediaWikiServices with injected dependencies.
Add support for checks to define a CONSTRUCTOR_OPTIONS constant that will result in a ServiceOptions object being passed in to the constructor holding those options. Remove `BaseCheck::getConfig()` which used MediaWikiServices, and remove unneeded entry in extension.json for ConfigRegistry.
Use TitleFactory and IEmailer to abstract calls to creating Title objects and sending emails, this will simplify creating unit tests if/when we add them.
Remove `CheckResult::getErrorsText()`, which used the global state via `wfMessage()`, and instead put the logic for converting the error details to messages in SpecialStatusCheck and StatusCheckEndpoint. Explicitly call `Message::parse()` instead of relying on implicit conversion to strings. Remove `CheckResult::isGood()`, which just proxies the underlying StatusValue object, and `RunResult::isGood()`, which just checks the results of the individual checks - in StatusCheckEndpoint, instead of using `RunResult::isGood()` at the end to determine the overall result, just set the result to failure when processing any individual check that failed. On Special:StatusCheck, don't show the same mitigation suggestion for each error that a check reports, but just once at the end.
Served as a level of indirection around an array of `CheckResult` objects; use such an array directly. The class constants that were used by StatusCheckEndpoint are identical to the ones in the `CheckResult` class; update the api class to reference those instead.
Instead of relying on the global state via `wfMessage()`, and implicitly converting the result `Message` to a string, use a `MessageLocalizer` to process the messages for the description and mitigation suggestions, and have `BaseCheck::getDescription()` and `::getMitigationSuggestions()` return `Message` objects for the caller to format. I had considered just having those methods return the underlying keys and having all of the processing be done in the callers, but by having the messages be constructed by the check classes we can more easily add parameters or other details to those messages in the future.
Rather than unconditionally uploading to `File:StatusCheckTestUpload.jpg`, deleting anything already there in the process, use a UUID generator to try and find the title of a file that does not already exist. In case of collision with an existing file, we retry once - after two collisions, something is probably wrong with the generator, and we skip the checks. Once the uploading has been verified to work, delete the created file page in a manner that also results in log entries, and don't suppress the deleted content. Also, remove unused `$mimeType` from `FileUploadCheck::perform()`.
Once the checks have been run once, change the form submission button from "Run checks" to "Run checks again" to make it clear that the form can be resubmitted. The decision regarding which message to show is based on if the web request to the special page was POSTed or not.
* Database writes are performed, fix `isWriteMode()` to return true. * Require a token to be provided when using the API. * Remove `mustBePosted()` method, now that a token is needed requests should be posted, and ApiBase defaults to requiring POST requests when a token is used.
Between the improvements here and the work getting CI set up I've written or rewritten a lot of the logic here
DanielWTQ
force-pushed
the
improvements
branch
from
309ea69 to
4664dc9
Compare
|
|
||
| namespace MediaWiki\Extension\StatusCheck; | ||
|
|
||
| class RunResult { |
There was a problem hiding this comment.
I actually liked the RunResult abstraction as it generally represented a logical Run entry that consists of a set of Checks performed. Although it was not too useful in the current extension state, I had hopes that it could potentially make it easier to track historical check runs nicely in the future, as every Run potentially could consist of a different set of Checks
Not saying we should bring it back
There was a problem hiding this comment.
if you aren't saying that, they I think it would be a premature optimization and to me it just caused confusion given the CheckResult class
| ) { | ||
| parent::__construct( | ||
| 'statuscheck-check-email-send', | ||
| 'statuscheck-check-email-send-suggestions' |
There was a problem hiding this comment.
I would personally prefer a slightly different interface:
Make the base abstract class require getDescriptionMsg(): str and getMitigationSuggestionsMsg(): str methods to be implemented by the inheriting class, with both methods returning the i18n keys
I find it less cognitively complex and more declarative than passing these values into the parent constructor. Plus, many IDEs will yell at you if you forgot to implement some of the base abstract class methods, while a missing parent constructor call is usually just a notice-level warning
This is not a change request, just thinking out loud. I might also be missing some important benefits of the proposed implementation
There was a problem hiding this comment.
Hmm, wearing my PHP developer hat it might be worth adding a way for constructors to indicate that any child class needs to all call the parent constructor
I figured even if we just switch to the various checks just identifying the key to use, there is still a significant amount of duplication, and in MediaWiki we generally call parent constructors (e.g. api classes, special pages, etc.) and so I didn't think it would be an issue of people forgetting (plus you'll get an error when the base class tries to use the fields that were not set)
2 participants
No description provided.