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

Work on #436. #437

Closed mjordan closed 6 years ago

mjordan commented 6 years ago

Github issue: (#436)

What does this Pull Request do?

Escapes a regex pattern so that it doesn't fail on Windows.

What's new?

As explained in the issue, adds the line $directory_regex = preg_quote($directory_regex); to the CsvNewspapers filegetter so the the DIRECTORY_SEPARATOR on Windows (\) is escaped properly.

How should this be tested?

This change can only be tested thoroughly on Windows, but I used it yesterday to successfully generate a batch on Windows. On non-windows platforms, the fix should be invisible. To test this:

  1. unzip the attached file, which contains some sample data and config files.
  2. adjust values in the .ini to match your system.
  3. in the master branch, run ./mik -c issue_436.ini
  4. change the name of the output directory so it is not overwritten in the next step
  5. switch to the issue-436 branch and rerun ./mik -c issue_436.ini
  6. the output from the two branches should be identical (apart from timestamps in log files)

Results of PHP tests should also be the same across the master and issue-436 branches. In the mik directory, run:

phpunit

issue_436.zip

codecov-io commented 6 years ago

Codecov Report

Merging #437 into master will increase coverage by <.01%. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #437      +/-   ##
==========================================
+ Coverage   27.22%   27.22%   +<.01%     
==========================================
  Files          79       79              
  Lines        5087     5090       +3     
==========================================
+ Hits         1385     1386       +1     
- Misses       3702     3704       +2
Impacted Files Coverage Δ
src/filegetters/CsvNewspapers.php 15.06% <0%> (ø) :arrow_up:
src/metadatamanipulators/SplitRepeatedValues.php 70.11% <0%> (-2.51%) :arrow_down:
src/fetchermanipulators/RandomSet.php 69.44% <0%> (+1.38%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 802f5bc...5e1131e. Read the comment docs.

mjordan commented 6 years ago

I wonder if we should be doing a thorough audit of all uses of DIRECTORY_SEPARATOR and regexes and add the preg_quote() if it's not there.

MarcusBarnes commented 6 years ago

@mjordan That's a good idea.

mjordan commented 6 years ago

Or for that matter, creating our own wrapper function, where we could do everything in one place.

mjordan commented 6 years ago

@MarcusBarnes @bondjimbond tests run clean - can one of you merge this? Or I can do it myself if there are no objections.

MarcusBarnes commented 6 years ago

Thank you @mjordan.

mjordan commented 6 years ago

@MarcusBarnes thanks!