User "Bryan" posted a comment on MediaWiki.r86041.
Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/86041#c15947
Commit summary:
r86001, now with less scariness :P I took out the delete action and did purge
instead, which is a much more self-contained action-with-a-form. Also
implement a few changes suggested by Brion on IRC last night.
Comment:
You're awesome. I think you have cured the nightmares that many people get when
they open Article.php.
A few remarks:
<pre>
+ /**
+ * Returns the name that goes in the \<h1\> page title
+ *
+ * Derived classes can override this, but usually it is easier to keep
the
+ * default behaviour. Messages can be added at run-time, see
+ * MessageCache.php.
+ *
+ * @return String
+ */
+ protected function getDescription() {
+ return wfMsg( strtolower( $this->getName() ) );
+ }
</pre>
I think you should return a wfMessage() instead of wfMsg(). This allows callers
to determine themselves how to represent the message. For example, the API
wants to return the plaintext, possibly parsemag'ed version of the message,
while the regular UI wants a HTML representation.
Something that I don't like about this design, is the fact that UI-generation
and backend-actions are tightly coupled, by means of the Form(less)Action class
which derives from Action. I would personally decouple them something like this:
<pre>
class Action {}
class ProtectAction extends Action {
[...]
}
class FormAction {
[...]
}
$form = new FormAction;
$form->setAction( 'ProtectAction' );
$form->show();
class ApiForAction {
[...]
}
$api = new ApiAction;
$api->setAction( 'ProtectAction' );
$api->execute();
</pre>
(I haven't though very well about it, and it might be overzealous abstraction,
but I'd at least look into this way)
_______________________________________________
MediaWiki-CodeReview mailing list
mediawiki-coderev...@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
