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

Removed ctype extension dependency #975

Closed
wants to merge 1 commit into from

Conversation

eelf
Copy link
Contributor

@eelf eelf commented Jul 29, 2013

Tests passed: OK (1246 tests, 1863 assertions)
I have no PEAR_RunTest class for running phpt tests :(

@whatthejeff
Copy link
Contributor

I think this code is fundamentally flawed. Not all platforms use drive letters. For instance, 'c:/tests' is a relative path on posix platforms.

I know CPython has a more thorough treatment of this in the form of os.path.isabs. For reference:

@Tatsh
Copy link
Contributor

Tatsh commented Jul 31, 2013

        if ($path[0] === '/' || $path[0] === '\\' ||
           (strlen($path) > 3 && preg_match('/[a-zA-Z]/', $path[0]) &&
              $path[1] === ':' && ($path[2] === '\\' || $path[2] === '/'))) {

Because the path length check is there, it doesn't seem like c:/tests would fail. It would correctly hit the 3rd path in the if as long as $path is lower-cased before this check.

An OS check should be added maybe?

@whatthejeff
Copy link
Contributor

Because the path length check is there, it doesn't seem like c:/tests would fail.

But it should fail on POSIX platforms.

It would correctly hit the 3rd path in the if as long as $path is lower-cased before this check.

This is not the correct behavior for POSIX platforms.

An OS check should be added maybe?

Yes, this is exactly what I'm proposing. Consider the following:

POSIX absolute path:

/home/whatthejeff/tests

NT absolute path:

c:\Users\whatthejeff\tests
\\machine\mountpoint\tests

@whatthejeff whatthejeff mentioned this pull request Jul 31, 2013
@glensc
Copy link

glensc commented Sep 24, 2013

is pcre_match locale insensitive? because if it's sensitive, A-Z does not match "the alphabet" when locale is for example et_EE

@Tatsh
Copy link
Contributor

Tatsh commented Sep 24, 2013

preg_match is locale insensitive unless /u modifier is used (which makes the regular expression specific to PHP).

The correct fix is to check for Windows somewhere (plenty of ways to do that, defined('PHP_WINDOWS_VERSION_BUILD') is one cheap way) and then proceed on one of two paths based on that result: Windows or POSIX.

@Tatsh
Copy link
Contributor

Tatsh commented Sep 24, 2013

New pull request with basic OS check:
#1010

@whatthejeff
Copy link
Contributor

Closing this in favor of #1010.

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

4 participants