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

Update XdebugVersion.php #94

Closed
wants to merge 3 commits into from
Closed

Conversation

wenz
Copy link

@wenz wenz commented Oct 1, 2020

Probably fixes #89

Copy link
Contributor

@derickr derickr left a comment

Choose a reason for hiding this comment

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

This patch is most certainly not correct, as it misses the equivalent check in line 410. In any case, what needs to happen is to check whether the value in $this->extensionDir is a relative path or an absolute path. If it's absolute, it should include it, if it's relative, it should not — regardless of whether you're on Windows or not.

@wenz
Copy link
Author

wenz commented Nov 26, 2020

Sorry for dropping the ball on this. Good catch with line 410. But I still don't understand why we should use absolute paths at all. When we load an extension with path or filename "x", PHP tries to load the physical file extension_dir + "x". The installation instructions recommend putting the Xdebug extension in the extension dir anyway. So regardless of whether the extension_dir is set to an absolute path, or a relativ path, or not set at all, just using the file name of the extension works in all cases IMO.

@derickr
Copy link
Contributor

derickr commented Jan 29, 2021

Absolute paths were required in the PHP 5 days for zend extensions, so that's why the instructions have that. Are you interested in fixing this PR so that I can merge it?

@wenz
Copy link
Author

wenz commented Jan 29, 2021

Will do - since PHP 5 is not supported any longer, we go with the relative paths, right?

@wenz wenz requested a review from derickr February 22, 2021 11:11
@derickr
Copy link
Contributor

derickr commented Mar 15, 2021

Yes, that's fine.

@derickr
Copy link
Contributor

derickr commented Sep 20, 2021

I think this PR is no longer relevant, as I've recently updated the logic. I'm therefore closing it.

@derickr derickr closed this Sep 20, 2021
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.

/wizard gives incorrect instruction for installing on windows
2 participants