MarcusBarnes / mik

The Move to Islandora Kit is an extensible PHP command-line tool for converting source content and metadata into packages suitable for importing into Islandora (or other digital repository and preservations systems).
GNU General Public License v3.0
34 stars 11 forks source link

Regex in filegetters/CsvNewspapers.php fails on Windows #436

Closed mjordan closed 6 years ago

mjordan commented 6 years ago

Related issue: #428.

Just hit this now. We've seen this problem before: since DIRECTORY_SEPARATOR on Windows is \, regexes can fail since the final pattern delimiter is escaped.

Will open a PR as soon as I get the test data assembled, but here's a diff for the fixed code:

@@ -147,8 +147,8 @@ class CsvNewspapers extends FileGetter
         // Get the path to the issue.
         $item_info = $this->fetcher->getItemInfo($record_key);
         $issue_directory = $item_info->{$this->file_name_field};
-        $escaped_issue_directory = preg_replace('/\-/', '\-', $issue_directory);
-        $directory_regex = '#' . DIRECTORY_SEPARATOR . $escaped_issue_directory . DIRECTORY_SEPARATOR . '#';
+        $directory_regex = '#' . DIRECTORY_SEPARATOR . $issue_directory . DIRECTORY_SEPARATOR . '#';
+        $directory_regex = preg_quote($directory_regex);
         foreach ($this->OBJFilePaths as $paths) {
             foreach ($paths as $path) {
                 if (preg_match($directory_regex, $path)) {
mjordan commented 6 years ago

I'd like to get this fix incorporated into the master branch, but it's only testable on Windows. I am running MIK in the issue branch just to complete some loads here internally. @MarcusBarnes and @bondjimbond, how do you want to handle testing a PR? Maybe settle for a regression test on Linux/OSX? In other words, if generating newspaper issues from CSV still works on those platforms, it's OK to merge?

MarcusBarnes commented 6 years ago

@mjordan Your suggestion for a regression test on Linux/OsX for now is OK with me. I'll look into getting a Windows VM setup for testing MIK PRs in the new year to help with this. @bondjimbond Your thoughts?

bondjimbond commented 6 years ago

Makes sense to me. (More annoyed than ever that my Windows laptop is kaput.)

mjordan commented 6 years ago

~OK, thanks, I'll assemble some test data and open a PR later today or over the weekend.~

Duh, did that a while ago in #437. It's ready to test!