r92864 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r92863‎ | r92864 | r92865 >
Date:16:49, 22 July 2011
Author:yaron
Status:reverted (Comments)
Tags:
Comment:
Undoing r92755 - if you want to revert these changes, please let's discuss it first
Modified paths:
  • /trunk/extensions/ConfirmEdit/MathCaptcha.class.php (modified) (history)

Diff [purge]

Index: trunk/extensions/ConfirmEdit/MathCaptcha.class.php
——@@ -39,11 +39,17 @@
4040
4141 /** Fetch the math */
4242 function fetchMath( $sum ) {
43 - if( MWInit::classExists( 'MathRenderer' ) ){
44 - $math = new MathRenderer( $sum );
45 - } else {
 43+ // class_exists() unfortunately doesn't work with HipHop, and
 44+ // its replacement, MWInit::classExists(), wasn't added until
 45+ // MW 1.18, and is thus unusable here - so instead, we'll
 46+ // just duplicate the code of MWInit::classExists().
 47+ try {
 48+ $r = new ReflectionClass( 'MathRenderer' );
 49+ } catch( ReflectionException $r ) {
4650 throw new MWException( 'MathCaptcha requires the Math extension for MediaWiki versions 1.18 and above.' );
4751 }
 52+
 53+ $math = new MathRenderer( $sum );
4854 $math->setOutputMode( MW_MATH_PNG );
4955 $html = $math->render();
5056 return preg_replace( '/alt=".*?"/', '', $html );

Follow-up revisions

RevisionCommit summaryAuthorDate
r92901Revert r92864 per discussion on CRdemon21:51, 22 July 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r92755Revert r92013, r92018 per CRaaron17:17, 21 July 2011

Comments

#Comment by 😂 (talk | contribs)   16:54, 22 July 2011

We did discuss it. You were the only one I saw objecting to using MWInit.

#Comment by Aaron Schulz (talk | contribs)   16:59, 22 July 2011

Yes, it would be best not to rehash this. An older MW install can use the older REL_ branch.

#Comment by Yaron Koren (talk | contribs)   17:02, 22 July 2011

I may have been the only one, but I think I made the best argument. :) Did you see it?

#Comment by 😂 (talk | contribs)   17:26, 22 July 2011

I saw your argument and I disagree with it. I really really hate fighting this in circles over and over and over every single time this happens. We obviously have completely different views of what's acceptable for maintaining compatibility between branches; and I've resigned myself to letting you guys do whatever crazy stuff you want in the Semantic extensions to do so.

ConfirmAccount is not one of those extensions though and runs on WMF sites, so HipHop support is needed. While yes, this ReflectionClass version of it is identical to MWInit's version...that's not the point. You're forking code, which is almost always a bad idea.

#Comment by Yaron Koren (talk | contribs)   18:22, 22 July 2011

Fine - the fact that it runs on WMF sites is, I suppose, a trump card in this discussion. So feel free to revert my change. I'll just note that my version of the code does in fact support HipHop, and otherwise works fully, as far as I know.

#Comment by Werdna (talk | contribs)   18:34, 22 July 2011

True, but using established interfaces, practices, etc leads to better quality code, in my opinion. Having code to do the same thing in the same place is much better and leads to more maintainable code.

I can definitely see the argument for backwards-compatibility for other extensions, there's definitely a trade-off involved. However, ConfirmEdit isn't being actively improved, so there isn't a compelling reason to use the unstable version on a stable release. I think in this case using the established interface outweighs the backwards compatibility concerns.

#Comment by Yaron Koren (talk | contribs)   18:41, 22 July 2011

Well, for what it's worth, ConfirmEdit actually is being actively improved - I added the "ReCaptcha" module in January, and the "Asirra" module just two weeks ago.

Status & tagging log