User:Chris Key/Sandbox/MetadataFormCR

Integrating MetadataForm into refactoring work

One of the most important tickets in Bugzilla is the CZ refactoring work (see http://reid.citizendium.org/bugzilla/show_bug.cgi?id=5). Recently, I have been red-lined fighting fires, e.g., trying to get the servers stable and working without crashes and severe congestion. However, late last week, I once again started working on this ticket.

The intention is most CZ modifications to MW would exist in a single extension, which for the sake of clarity I will call the Citizendium extension. Since MetadataForm is a modification specific to CZ, it should be integrated into the Citizendium extension. As we refactor functionality out of the main code into the Citizendium extension, we need a place to put it. Probably the best approach is to have an include directory in the Citizendium extension and add modifications there. In that case, we probably should put MetadataForm in that directoy and place a require_once(include/Metatdataform) in Citizendium.php. I think most of your code will work without modification using this strategy. However, some of your instructions in the README and the text in MetadataForm output if Metatdataform.php is executed stand-alone will require modification.

Code integrated into CZ extension by Dan. Modified the couple of places required to make this done. --Chris Key 19:05, 7 May 2010 (UTC)

Code review comments

General Comments

1. If you look at most of the code written for MW, much of the text is formated so it fits into 72 columns. Many of your code files extend lines well beyond this limit. This makes it hard to read when using some editors, e.g., vim. I suggest you use linebreaks appropriately so it can be read without the editor forcing a soft line wrap, making the text difficult to read.

Done --Chris Key 19:04, 7 May 2010 (UTC)

2. I am not the techncial lead, but I feel strongly that when we add code, we need to provide some unit tests that can be integrated later into a CZ regression test suite. There are a number of reasons for such tests. One is as we refactor the CZ code, we need to ensure existing factored code continues to work correctly. Another is when we update the MW base and merge the Citizendium extension into it, we need a way to test that the extension still works.

So, I think you need to write some unit tests that exercise the extension's functions. Right now we don't have a regression test harness. The limited regression testing that comes with the MW code base is almost exclusively focused on the parser. These are written using a customized test harness that might be adapted to testing the Citizendium extension. Alternatively, we might use something like PHPUnit (http://www.phpunit.de/).

In any case, you should write the MetadataForm unit tests so they are adaptable to a regression test harness.

README

Is there some reason to make $wgMetadataFormWorkgroups a global variable rather a MetadataForm class attribute? Is there somewhere that this variable has to be accessed other than in association with the MetadataForm class? If not, then I suggest making it a class attribute.

Is the $wgMetadataFormWorkgroups data defined elsewhere, either explicitly or implicitly? If so, we need to centralize its definition. Also, to improve readability, I suggest putting each value of the $wgMetadataFormWorkgroups array on a single line (perhaps prefixed by a tab to make the data stand out).

MetadataForm.php

There is no MW standard text to echo in case an extension is executed stand-alone. However, I suggest you prefix the text you have supplied with a simple statement, such as, "This is the MetadataForm extension."

MetadataForm_body.php

1. You probably should use isLoggedIn() rather than isAllowed('edit') to determine whether the user is logged in.

2. Comment at line 33 "# POST is to create an article" probably should be "# POST is to create metadata". Also, doesn't the extension support editing existing metatdata? If so, then the comment should be "# POST is to create or modify metadata".

3. At line 80 you use the operator "and" rather than "&&". This is obviously for precedence purposes. Personally, I find the precedence differences between 'and' and '&&' to be a source of confusing code. If you must use it, you should format the expression so the 'and' stands out where it is obvious that it is connecting two non-parenthesized expressions (e.g., on a separate line of its own).

4. Line 232 - not sure why the comment:

//$wgOut->addHTML("<form action='".$self->getLocalURL()."' method='POST'>");

is here. Is this fossil code?