Feedback from AS re: improving patches

[tareko]

That's a pretty messy commit. Looking through it, there are some parts that are most likely back-ported bug fixes and won't need porting:

• classes/core/String.inc.php
• classes/article/ArticleGalleyDAO.inc.php
• classes/core/Core.inc.php
• classes/issue/form/IssueForm.inc.php
• classes/oai/OAI.inc.php
• classes/oai/OAIStruct.inc.php
• classes/oai/format/OAIMetadataFormat_NLM.inc.php
• classes/oai/ojs/JournalOAI.inc.php
• classes/oai/ojs/OAIDAO.inc.php
• classes/submission/form/MetadataForm.inc.php

Some of it is cruft:

• classes/issue/form/IssueForm.inc.php.orig
• classes/oai/OAI.inc.php.orig
• classes/submission/form/MetadataForm.inc.php.orig
• ojs/pages/user/fields[0]?true:false
• plugins/blocks/developedBy/block.tpl-withad
• templates/article/article.tpl-new
• templates/common/header.tpl-ad
• templates/common/header.tpl-ad.save
• templates/common/header.tpl-bothads
• templates/common/header.tpl-noad
• templates/common/header.tpl-orig
  (and probably others)

Some of it is third-party (your?) code:

• fcoichecklist/*

Of the actual changes to OJS, were you looking for a code review? Here are a few brief suggestions:

• DOIs generally shouldn't be exposed to te templates; that blurs V with C in the MVC model. (IssueHandler.inc.php and related templates)

There are a few divergences from our coding style (should use "$thingDao =& DAORegistry::getDAO('ThingDAO');" rather than "$ThingDAO = &DAORegistry::getDAO('ThingDAO');", for example, and indentation is sometimes rough), but that's not a big deal.

• plugins/blocks/developedBy/block.tpl: the code header (including copyright) has been removed. It's not localized, but that would only be an issue if you needed a bilingual interface and/or wanted to submit code back to us.
• The CMS plugin still exists in your install, and we've long-since stopped maintaining it. Check out the "Static Pages" plugin instead.
• The "External Feed", pid, and static pages plugins are in the patch; I'd suggest checking to see if there's a newer version. These probably had mods to support your old OJS.
• The XML galleys plugin has known issues (none that you won't already have encountered) noted in the new version of the code. It looks like you've done some modifications as well. This is worth reviewing.

The rest are UI changes, and will port across fairly directly, though you'll find that a lot of code has moved into lib/pkp. That'll mean manual intervention, but at the same time it'll be a chance to review whether the changes were necessary or are accumulations from various iterations of the site (I think there will be some of that too, at a glance). Some of the new templates you introduced (templates/issue/section.tpl, for example) were probably based off other templates in the system, and if you copy them directly across, they'll inherit some old code styles. (No immediate examples, but e.g. I've just re-opened http://pkp.sfu.ca/bugzilla/show_bug.cgi?id=3289 and I see some of the same code duplicated in your new template.)

If it's possible, try to break your commits up by function henceforth (I know this is a migration issue and therefore not as easy); if it's just a page of code at a time, I'll be able to review a lot more quickly and thoroughly.