Last modified: 2012-02-10 15:58:39 UTC
Currently, Monobook.php is something of a mess, a textbook case of how not to write PHP. HTML is interspersed with PHP, obliterating the structure of the PHP logic in favor of the HTML structure. It's a huge pain to read and edit, with everything clogged up with <?php's and ?>'s and all the indentation following the HTML rather than the PHP. Could its structure please reflect the structure of the PHP, not of the HTML?
This could be done now with string interpolation, I think, which should be more legible.
Created attachment 2188 [details] Replace all ?>...<?php with proper concatenation The question is, is this more readable? It makes the PHP logic obvious, while making the HTML structure non-obvious.
Created attachment 2189 [details] Use interpolation instead of concatenation Now *this* patch makes Monobook much easier to read. It basically just uses in-string variable parsing for double-quoted strings to stick as many variables as possible in the output itself. (All those "$this->text(blah)"s were, of course, removed.) Its output is, as far as I can determine, 100% identical to the current Monobook except for spacing (I've made the indentation of the output nicer) plus one or two extra HTML comments.
Just after a quick look: You put + function getSkinName() { + return "monobook"; + } there though MonoBook.php is included by other skin files (Chick.php, MySkin.php, ...) so it might be necessary to override that function there. Or maybe this would be the opportunity to rename MonoBook.php to something like MonoBookBase.php, create a new, short MonoBook.php, and have all MonoBook based skins include MonoBookBase.php to avoid such confusion in future.
Created attachment 2192 [details] Use interpolation instead of concatenation Whoops, I didn't mean to include that part. That was something unrelated. And dammit, I left in error-reporting code too. Sigh . . . I *really* need to be less sloppy. New version includes *only* changes to Monobook::execute(), as intended.
If we're going to use interpolation (with double quotes), should the html attributes be enclosed in single quotes where possible? Also, is {$data['bodytext']} or $data[bodytext] more readable?
Created attachment 3017 [details] example monobook output In a different web application I wrote I'm also using the MonoBook skin. This is the getFinalOutput() function from the output class, with some comments added to make it more understandable out of the context. This way is in my opinion quite readable (but as the author I'm not on a npov, of course), both in the PHP code and in the HTML output. In the code I used variables in double quotation marks. Also it uses tabs in the source instead of \t, which is more readable I think. It's outputting an consistently indented html. This is done by exploding multiple line strings, such as the body text, which is produced in many other functions and classes, by \n, and thus prefixing every line with the current indention. I think this would be a quite nice way to write Monobook.php; I don't know how you think about it, though. It should be good as an idea, so please don't stab me ;-)
Created attachment 4285 [details] this is how it could look like... Well, this could be better. It still needs some testing (especially in the "if" statements), but I'm working on it. I hope you like it.
(In reply to attachment from comment #8) > function initPage( &$out ) { - Probably no ampersand needed, since $out is an object. > * @access private > function getData( $str ) { - Probably just declare the getData function as private, rather than documenting it as such. - Also most of the MonoBookTemplate class should be indented one more tab/level. - Could potentially also break execute() up into private showHeader(), private showBody(), private shopTopMenu(), private showLogo(), private showMenuNavBar(), private showMenuSearchBar(), private showToolbox(), private showOtherLanguages(), and private showFooter(). - Others may disagree, or have different suggestions. Since to a large extent this bug is about code style, rather than a binary bug/no bug situation, this is to be expected.
(In reply to comment #9) I tried not to change the code, only the style. But it's a good idea, I'll work on it. /*sorry for my english*/
I don't see a working patch, and without it this kind of bug is useless.
No more than any other bug. It points out something to be fixed, that's all.
*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*
Alyx, I'm marking this bug with the "reviewed" keyword since you have received some comments about the patch. Thanks for contributing to MediaWiki.
Patch is extremely out of date (I'm not talking out of date simply because of my 1.18 changes, this patch references ism-s that were last seen in 1.15). Templates ARE markup they aren't piles of php logic. The php logic should be minimal, we shouldn't be making changes that add even more php logic to what is supposed to be a template of the general layout of the skin's markup. If I were feeling bold I'd WONTFIX this right now. It's the complete opposite of what needs to be done to improve the skin system.
