Patrick Reilly wrote: > On Aug 15, 2011, at 6:27 PM, MZMcBride <[email protected]> wrote: >> Is the MobileFrontend extension development being monitored or reviewed by >> any senior developers? > > Yes, it has been reviewed.
By whom? >> I took a look at the code today and it seemed >> incredibly strange. > > Could you send along some examples please. The use of a views directory containing .html.php files stuck out to me. As far as I know, this is the only extension in Wikimedia's SVN repo using this type of code structure. It looks like a holdover from the old Ruby code. Is that right? Is it going to be rewritten? >> Some of its layout and architecture doesn't seem to be >> consistent with MediaWiki conventions/style and a lot of code (particularly >> variable assignments) looked duplicative. > > Can you please provide an example. Variables such as $randomButton are defined four times in the same file (extensions/MobileFrontend/MobileFrontend.php). I don't know too much about PHP, but it seemed very strange. Is there a reason for the duplication? Could it be reduced? In general, the URL parameters are odd. I'm not sure if the English Wikipedia is running the newest code, but if you visit a URL such as <http://en.wikipedia.org/w/index.php?title=Main_Page&useFormat=mobile>, none of the links within it maintain the &useFormat parameter, including and especially the links at the bottom. If you click one of the links at the bottom such as "enable images on the mobile site," it removes all parameters including the ?title= parameter. Rather than something sane and predictable for the URL parameter to view the standard (non-mobile) site (such as &useFormat=standard), the code uses ?mAction=view_normal_site. As I commented in CodeReview today, a boolean parameter (?disableImages=1) has now been complemented with a completely separate parameter (?enableImages=1) rather than using ?disableImages=0, which I find to be very strange. The HTML at the bottom of the page at <http://en.wikipedia.org/w/index.php?title=Main_Page&useFormat=mobile> appears to also be completely invalid (pasted below): ---- <div id='copyright'>Text is available under the <a rel="license" href="http://en.wikipedia.org/wiki/Wikipedia:Text_of_Creative_Commons_Attrib ution-ShareAlike_3.0_Unported_License">Creative Commons Attribution-ShareAlike License</a><a rel="license" href="http://creativecommons.org/licenses/by-sa/3.0/"; style="display:none;"></a>; additional terms may apply. See <a href="http://wikimediafoundation.org/wiki/Terms_of_use";>Terms of use</a> for details.<br/> Wikipedia® is a registered trademark of the <a href="http://www.wikimediafoundation.org/";>Wikimedia Foundation, Inc.</a>, a non-profit organization.<br /></li><li class="noprint"><a class='internal' href="http://en.wikipedia.org/wiki/Wikipedia:Contact_us";>Contact us</a></div> </div> ---- As far as I can see, the code is defining list items without specifying a <ul> or <ol> pair. One of the <li> elements is unclosed. And there's also some very strange display:none; code in this snippet. All of this seems very odd and quirky. I believe some of this quirkiness would be mitigated by using pre-built functions and following established coding styles, rather than using DIY templates. Again, I'm wondering which senior developer (or even non-senior developer) is reviewing this code. Some clarification on that would be appreciated. I've probably written about ten lines of PHP in my life, so feel free to tell me off if these comments are out of left field and/or don't make any sense. These were just my observations from reading through some of the code today. >> Some revisions are also apparently being pushed live without any outside >> review. > > Yes, this is known. Every effort has been made to get eyes on every change, > but some things have been pushed quickly for testing and to meet our schedule > for the opt-in launch. It's incredibly dangerous to push out code without at least cursory outside review. It's an enormous risk you're taking, though it's apparently yours to take. It's a very bad situation, in my opinion. MZMcBride _______________________________________________ Wikitech-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikitech-l
