This could be done now with string interpolation, I think, which should be more legible.

ayg wrote:

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.

attachment 6872a.patch ignored as obsolete

ayg wrote:

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.

attachment 6872b.patch ignored as obsolete

z9z8z-wps wrote:

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.

ayg wrote:

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.

attachment 6872b.patch ignored as obsolete

dto wrote:

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?

leon wrote:

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 ;-)

Attached:

alyx486 wrote:

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.

attachment MonoBook.php ignored as obsolete

nickpj wrote:

(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.

alyx486 wrote:

(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.

ayg wrote:

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*

sumanah wrote:

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.