Skip to content
This repository was archived by the owner on Mar 20, 2024. It is now read-only.

Screenshot filename has invalid characters#253

Open
ollietb wants to merge 1 commit intoBehat:masterfrom
ollietb:patch-1
Open

Screenshot filename has invalid characters#253
ollietb wants to merge 1 commit intoBehat:masterfrom
ollietb:patch-1

Conversation

@ollietb
Copy link
Copy Markdown

@ollietb ollietb commented Jul 20, 2016

The screenshot filename "firefox_2016-07-12T15:19:21+00:00.578f8b3deeff62.22313382.png" contains invalid characters on Windows. This PR changes it to "firefox_2016-07-20_16.31.25_578f8b3deeff62.22313382.png"

The screenshot filename "firefox_2016-07-12T15:19:21+00:00.578f8b3deeff62.22313382.png" contains invalid characters on Windows. This PR changes it to "firefox_2016-07-20_16.31.25_578f8b3deeff62.22313382.png"
@aik099
Copy link
Copy Markdown

aik099 commented Jul 20, 2016

The screenshot filename "firefox_2016-07-12T15:19:21+00:00.578f8b3deeff62.22313382.png" contains invalid characters on Windows.

Which exactly characters?

// Under Cygwin, uniqid with more_entropy must be set to true.
// No effect in other environments.
$filename = $filename ?: sprintf('%s_%s_%s.%s', $this->getMinkParameter('browser_name'), date('c'), uniqid('', true), 'png');
$filename = $filename ?: sprintf('%s_%s_%s.%s', $this->getMinkParameter('browser_name'), date('Y-m-d_H.i.s'), uniqid('', true), 'png');
Copy link
Copy Markdown

@aik099 aik099 Jul 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is better to:

  1. use date('c') as before
  2. afterwards apply regular expression, that would replace all non filename safe characters

?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aik099 I'm not sure why that would be better. It would increase the amount of processing needed when we have full control over the filename anyway.

Copy link
Copy Markdown

@aik099 aik099 Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can guarantee, that other components in filename (e.g. browser_name and uniqid) won't ever contain forbidden symbols, then of course keep PR code as-as.

@ollietb
Copy link
Copy Markdown
Author

ollietb commented Jul 21, 2016

@aik099 The colons are reserved characters in Windows file systems https://msdn.microsoft.com/en-gb/library/windows/desktop/aa365247%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396. Altough the + is valid, I think it's best to stick to normal filename characters.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants