Last modified: 2012-02-10 15:58:39 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 6872 - Make Monobook.php more readable
Make Monobook.php more readable
Product: MediaWiki
Classification: Unclassified
General/Unknown (Other open bugs)
All All
: Low enhancement with 5 votes (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-reviewed
Depends on:
Blocks: code_quality
  Show dependency treegraph
Reported: 2006-07-31 03:02 UTC by Aryeh Gregor (not reading bugmail, please e-mail directly)
Modified: 2012-02-10 15:58 UTC (History)
6 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---

Replace all ?>...<?php with proper concatenation (21.40 KB, patch)
2006-07-31 22:49 UTC, Aryeh Gregor (not reading bugmail, please e-mail directly)
Use interpolation instead of concatenation (25.62 KB, patch)
2006-08-02 05:06 UTC, Aryeh Gregor (not reading bugmail, please e-mail directly)
Use interpolation instead of concatenation (25.22 KB, patch)
2006-08-02 16:28 UTC, Aryeh Gregor (not reading bugmail, please e-mail directly)
example monobook output (5.04 KB, text/plain)
2007-01-06 21:30 UTC, Leon Weber
this is how it could look like... (17.78 KB, patch)
2007-10-28 23:49 UTC, Alyx

Description Aryeh Gregor (not reading bugmail, please e-mail directly) 2006-07-31 03:02:07 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?
Comment 1 Brion Vibber 2006-07-31 05:30:57 UTC
This could be done now with string interpolation, I think, which should be more legible.
Comment 2 Aryeh Gregor (not reading bugmail, please e-mail directly) 2006-07-31 22:49:30 UTC
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.
Comment 3 Aryeh Gregor (not reading bugmail, please e-mail directly) 2006-08-02 05:06:49 UTC
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.
Comment 4 Schnargel 2006-08-02 12:05:14 UTC
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 

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.
Comment 5 Aryeh Gregor (not reading bugmail, please e-mail directly) 2006-08-02 16:28:38 UTC
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
Comment 6 Dan Li 2006-09-03 04:54:32 UTC
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?
Comment 7 Leon Weber 2007-01-06 21:30:18 UTC
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

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 ;-)
Comment 8 Alyx 2007-10-28 23:49:02 UTC
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.
Comment 9 Nick Jenkins 2007-10-30 00:36:29 UTC
(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.

Comment 10 Alyx 2007-10-31 11:26:51 UTC
(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*/

Comment 11 Niklas Laxström 2009-02-28 19:05:14 UTC
I don't see a working patch, and without it this kind of bug is useless.
Comment 12 Aryeh Gregor (not reading bugmail, please e-mail directly) 2009-03-01 00:28:44 UTC
No more than any other bug.  It points out something to be fixed, that's all.
Comment 13 p858snake 2011-04-30 00:09:59 UTC
*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*
Comment 14 Sumana Harihareswara 2011-11-09 02:58:45 UTC
Alyx, I'm marking this bug with the "reviewed" keyword since you have received some comments about the patch.  Thanks for contributing to MediaWiki.
Comment 15 Daniel Friesen 2011-11-09 06:03:55 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.