Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix require path for windows #141

Merged
merged 1 commit into from
Jun 8, 2018
Merged

Conversation

wezzy
Copy link
Contributor

@wezzy wezzy commented Jun 8, 2018

In windows the generated path has the last separator wrong, using default PHP constant DIRECTORY_SEPARATOR allows your code to work correctly in both windows and *nix

In windows the generated path has the last separator wrong, using default PHP constant DIRECTORY_SEPARATOR allows your code to work correctly in both windows and *nix
@narfbg
Copy link

narfbg commented Jun 8, 2018

Just so it's noted - the forward slash does work perfectly well on Windows in PHP and there's nothing wrong with using it. I'm only aware of a single issue with it and it's in libxml.

But that being said, DIRECTORY_SEPARATOR is semantically more correct, so +1 from me.

@wezzy
Copy link
Contributor Author

wezzy commented Jun 8, 2018

You are right, it should work but I don't know if this is due to a particular setup but we have lot of errors like this in out log:

[05-Jun-2018 19:39:55 Europe/Rome] PHP Fatal error: require_once(): Failed opening required 'D:\Siti\project_name\vendor\paragonie\random_compat\lib/byte_safe_strings.php' (include_path='.;C:\php\pear') in D:\Siti\ricordavet.it\vendor\paragonie\random_compat\lib\random.php on line 52

Now that we changed to DIRECTORY_SEPARATOR in our local version those errors are gone

@owenvoke
Copy link

owenvoke commented Jun 8, 2018

That is pretty weird as I've tested on Windows Server 2008, 2012 and 2016 and couldn't replicate this issue. But it's also mentioned in #136.

Anyway, this looks fine to me. 👍

@paragonie-scott
Copy link
Member

There's no downside to this change, but as others have stated, I cannot reproduce this issue either.

@paragonie-scott paragonie-scott merged commit ce94ba2 into paragonie:master Jun 8, 2018
@narfbg
Copy link

narfbg commented Jun 9, 2018

I'd bet the error state was cached ... That is, an include was attempted while the path didn't exist, PHP cached that it doesn't exist and then short-circuits on successive attempts, erroring while the path is now correct.

Changing the slash alters the path and therefore it no longer hits the cache, so that's why it looks like it's of any importance.

@paragonie-scott
Copy link
Member

Oh hey that's new! Github didn't used to show comments on commits in this view before?

@glensc
Copy link
Contributor

glensc commented Jun 15, 2018

what is the "this view"? link? screenshot? :)

@paragonie-scott
Copy link
Member

glensc0

 |
 v

glensc1

@glensc
Copy link
Contributor

glensc commented Jun 15, 2018

i mean what's your url. i see your comment in PR view: #141 (comment)

https://github.com/paragonie/random_compat/pull/141#commitcomment-29381064

@paragonie-scott
Copy link
Member

8a6e54a

@paragonie-scott
Copy link
Member

paragonie-scott commented Jun 15, 2018

The "that's new!" was about it also showing up in PR view, rather than merely on the commit.

@paragonie-scott
Copy link
Member

Anyway, this is all off-topic.

  1. I couldn't reproduce the issue.
  2. The change was idempotent for anyone unaffected by the non-reproducible issue.
  3. It apparently fixed the affected machines.
  4. Commenting on commits rather than creating issues on GitHub is bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants