Bug 6918 - Native import plugin reference bug may garble multiple supplementary file import
Native import plugin reference bug may garble multiple supplementary file import
Status: RESOLVED FIXED
Product: OJS
Classification: Unclassified
Component: Plug-ins
2.3.7
All All
: P3 normal
Assigned To: PKP Support
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-07 10:10 PDT by Martin Haye
Modified: 2011-10-07 11:30 PDT (History)
1 user (show)

See Also:
Version Reported In:
Also Affects: OCS 2.3.4


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Haye 2011-10-07 10:10:55 PDT
The Native Import plugin contains an interesting bug for which I have a simple fix. The fix is one character, but the explanation is necessarily much longer.

The symptom: 

When importing an article with more than one supplemental file, the second-to-last file will be duplicated in place of the last file.

The patch:

diff --git a/plugins/importexport/native/NativeImportDom.inc.php b/plugins/importexport/native/NativeImportDom.inc.php
index a3c7504..652c5be 100644
--- a/plugins/importexport/native/NativeImportDom.inc.php
+++ b/plugins/importexport/native/NativeImportDom.inc.php
@@ -870,7 +870,7 @@ class NativeImportDom {
                $hasErrors = false;
                $galleyCount = 0;
                for ($index=0; $index < count($articleNode->children); $index++) {
-                       $node =& $articleNode->children[$index];
+                       $node = $articleNode->children[$index];
 
                        if ($node->getName() == 'htmlgalley') $isHtml = true;
                        elseif ($node->getName() == 'galley') $isHtml = false;

The explanation:

It boils down to references vs. objects in PHP 5+. Because all the XML nodes being accessed are already objects, it isn't necessary, and in fact doesn't make sense, to take a reference to them as in the original code above. That said, doing so is usually harmless; in this case it's a problem.

What happens is that, after executing the loop above, $node ends up being a reference to the last entry of the $articleNode->children array. That entry contains an object identifier, identifying the last child. The problem shows up when the following code iterates through the supplemental_file elements, assigning each in turn to $node. Because $node is a reference to the last array entry, the last array entry gets changed to each supplemental_file element in turn, which is obviously not desirable. Thus on the second-to-last iteration the last element is changed to the second-to-last element; on the last iteration it is then reassigned to itself; the last element is lost forever.

The patch makes $node a simple copy of the last array element instead of a reference to it. This is still efficient because we have to remember that in PHP, the array element doesn't contain the entire XML node, only an object identifier for that node. See http://www.php.net/manual/en/language.oop5.references.php for more on that.
Comment 1 Alec Smecher 2011-10-07 10:18:38 PDT
Good catch, those are tricky to debug! Also patched OCS for the same issue. Thanks for contributing.
Comment 2 Alec Smecher 2011-10-07 10:20:03 PDT
Fixed reference glitch
https://github.com/pkp/ocs/commit/8b4e3540de6e8d6f65bfabe01845fb048c57d40a
Comment 3 Alec Smecher 2011-10-07 10:20:03 PDT
Fixed reference glitch
https://github.com/pkp/ocs/commit/63d294fb38bf750849a2d1c956b642cd6b6b5e45
Comment 4 Alec Smecher 2011-10-07 11:30:02 PDT
Fixed reference glitch
https://github.com/pkp/ojs/commit/bd0acc5f6802750527ccca4e660c4c27fb49433d