changed delimiter #17
Conversation
There was a problem hiding this comment.
Thanks, @ajstanley ! The use of the PHP_EOL is preferred, if possible. I'm not immediately familiar with all of the larger context here, though. Is the $pid input to these functions consistently created with local PHP line ending assumptions, or could this ever come from a file created with alternate assumptions?
Overall this is a good PR, but I want to sanity check some of the original assumptions, particularly w/r/t the "\r" and "\r\n" lines. Perhaps @wgilling would know?
| $pids = islandora_datastreams_io_pids_namespace_accessible($pids); | ||
| $bad_pids = $skipped = array(); | ||
| $updated_count = 0; | ||
| $pids_arr = explode("\r\n", $pids); |
There was a problem hiding this comment.
This is an interesting use case. Why was this assuming CRLF when others were assuming LFs?
| $pids_arr = explode("\n", $pids); | ||
| $pids_arr = explode(PHP_EOL, $pids); | ||
| foreach ($pids_arr as $pid) { | ||
| $pid = str_replace("\\r", "", trim($pid)); |
There was a problem hiding this comment.
This raises questions. Is this really a replace of literal '\r' , or is this a replace of CRs unhandled by the LF explode?
There was a problem hiding this comment.
AFAIK the strings are harvested internally. We could explode on a regex if we didn't know where the source files were coming from.
There was a problem hiding this comment.
The input of PID values would be whatever the user entered in the textarea and I am not an expert on all of the operating systems' or text editors' copy/paste formats -- but did have to put this in there to clean up the $pid value per line because one editor did not have the \r at all and another one did. I would think that the changes would certainly not break anything - possibly the str_replace on line 250 is harmlessly redundant.
I'd used this module on a prod site a few weeks ago, but had to change the explode delimiter in the transform form. The PHP constant should work on any environment.