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

Windows related changes, ahead/behind only current branch #207

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

alfsb
Copy link
Member

@alfsb alfsb commented Dec 13, 2024

This PR makes languages.php more functional on Windows, and also changes repository status to not show ahead/behind of unrelated branches.

As a heads up, also remove a ugly hack for dirs named PDO vs pdo. If you are on Windows, and start receiving errors of undefined entities that contains PDO (upper case), you may need to filesystem remove all files from en/reference/pdo*, and then git restore then, as git on Windows may not honor case changing moves.

scripts/file-entities.php Outdated Show resolved Hide resolved
@alfsb alfsb mentioned this pull request Dec 14, 2024
27 tasks
@Girgias Girgias requested a review from cmb69 December 20, 2024 12:42
scripts/file-entities.php Show resolved Hide resolved
Comment on lines 321 to 325
$path = str_replace( "\\" , '/' , $path );
if ( $touch && ! file_exists( $path ) )
touch( $path );

$res = realpath( $path );
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand why you first replace backward slashes with forward slashes, and later call realpath() which will reintroduce the backslashes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because realpath( "C:\phpdoc\en" ) will fail on Windows, I know that is because escaping and not directly related with realpath, but it really frustrating to have some literal "file part" sometimes behaving differently.

And to avoid code end looking like this:

const RNG_SCHEMA_DIR = __DIR__ . DIRECTORY_SEPARATOR . 'docbook' . DIRECTORY_SEPARATOR . 'docbook-v5.2-os' . DIRECTORY_SEPARATOR . 'rng' . DIRECTORY_SEPARATOR;

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot test ir now (soon starting some new year holidays), but I vaguely remember that libxml will accept foward slashes on Windows, backward slashes, but not mixed slashes, e.g., C:\phpdoc/en.

Copy link
Member

Choose a reason for hiding this comment

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

Well, yeah, you can usually just use forward slashes on Windows. But I still don't understand why you first replace backward slashes with forward slashes, and later call realpath() which will reintroduce the backslashes. I mean that has nothing to do with literal file paths, nor with mixing forward and backward slashes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will have to test again, without this PR, to see exactly what was broken. I think there is something git related, as this function was copied from languages.php, that does a lot of git invocations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Remembered now. realpath() does not reintroduces foward slashes in all cases. It's not even guaranteed to return string. This function is called for expected non existent files, where realpath() returns bool, and I was under impression that this may fails on Windows, and at the time (as now) it's difficult for me to access Windows boxes to make tests. Yet. I remembered that forwards slashes always worked.

Also, these is some evidence this was the norm on doc-base scripts. xml-check.php does it, and I remember that previous file-entities.php there is a lot oft this too.

Copy link
Member

Choose a reason for hiding this comment

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

$realpath = str_replace('\\', '/', realpath($_SERVER["argv"][1]));

That code calls realpath() and afterwards replaces the backslashes with forward slashes. That can make sense. Even if the file does not exists, you get false and the str_replace() will return an empty string. Even that might make sense.

I still fail to understand, why you first replace forward slashes with backward slashes, and then call realpath() which will always use backward slashes on Windows (unless the file does not exist, in which case the given filepath will be returned with forward slashes). Maybe you want something like this:

{
    if ( $touch && ! file_exists( $path ) )
        touch( $path );

    $res = realpath( $path );
    if ($res !== false)
        $path = $res;

    return str_replace( "\\" , '/' , $path );
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I still fail to understand, why you first replace forward slashes with backward slashes

To avoid mixed slashes (C:\phpdoc/en) when realpath() fails. All backwards, all fowards, never mixed.

and then call realpath() which will always use backward slashes on Windows

The "foward slashes everywhere" refers to using foward slashes everywhere in source code. Having all slashes being foward on Windows when used (replaced), is better for compatibility.

That said, I will remove this line.

scripts/file-entities.php Outdated Show resolved Hide resolved
scripts/file-entities.php Outdated Show resolved Hide resolved
alfsb and others added 3 commits December 20, 2024 13:54
Co-authored-by: Christoph M. Becker <[email protected]>
Co-authored-by: Christoph M. Becker <[email protected]>
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.

3 participants