A snake oil PR, i.e. has no effect, because 1 line later any "\" is replaced by "/" in $path using str_replace, and so at the end all is "/", or not?

Probably you did not understand the issue. Ok, let me explain then.

Consider the following :

JPATH_BASE = 'D:\xampp\htdocs\joomla';
$dir = 'D:\xampp\htdocs\joomla\templates\protostar\favicon.ico';

$path = str_replace(JPATH_BASE . '/', '', $dir);

will not replace anything. Result:

$path = 'D:\xampp\htdocs\joomla\templates\protostar\favicon.ico';

$path = str_replace('\\', '/', $path);

Result:

$path = 'D:/xampp/htdocs/joomla/templates/protostar/favicon.ico';

$this->addFavicon(JUri::base(true) . '/' . $path . 'favicon.ico');

End result:

<link href="joomla/D:/xampp/htdocs/joomla/templates/protostar/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon" />

Now does it sound logical? If not then you can always test it yourself with xampp under Windows 8.1.

Yes, you are right, I was wrong with syntax of str_replace of PHP. Sorry for that. But your explanation is also wrong: The "favicon.ico" at the end of $dir would cause a "favicon.icofavicon.ico" at the end of the href, or not?

My bad. I considered a wrong $dir value. Correcting it below:

JPATH_BASE = 'D:\xampp\htdocs\joomla';
$dir = 'D:\xampp\htdocs\joomla\templates\protostar\';

$path = str_replace(JPATH_BASE . '/', '', $dir);

will not replace anything. Result:

$path = 'D:\xampp\htdocs\joomla\templates\protostar\';

$path = str_replace('\\', '/', $path);

Result:

$path = 'D:/xampp/htdocs/joomla/templates/protostar/';

$this->addFavicon(JUri::base(true) . '/' . $path . 'favicon.ico');

End result:

<link href="joomla/D:/xampp/htdocs/joomla/templates/protostar/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon" />

@test Tested with success, see following screenshots.

1. Before patch:
   

2. After patch:
   

Tested on a windows host with latest staging + this PR patched into it.

P.S.: I had in mind there was some discussion and a PR for the same issue before, and now I have found it, it was #5566 by @smz . I left a comment there that now this PR here has been made.

Object to introducing DIRECTORY_SEPARATOR into Joomla. Just because some people still don't understand writing platform agnostic code! This should be treated as REGRESSION. The PR introducing the \ as directory separator should be reverted. It is the wrong solution for the issue, even mentioned there!
Joomla should standardize on '/' within its code. Platform differences should be translated to the Joomla standard. The usage of DIRECTORY_SEPARATOR should be limited to those occassions where it is absolutely needed.Don't force all extensions to use DIRECTORY_SEPARATOR ! That is considered a break of BC!

@AbhishekDas The introduction of the DIRECTORY_SEPARATOR will not be merged as we actually removed all references to this years ago. Can you update your PR to not use it?

He can't. He can either use $path = JPath::clean($dir, '/'); to standardize the entire path to / separators before doing any other operation in that segment or leave it as is because this is a case where you are doing explicit string comparisons on file paths and a valid case where the use of DIRECTORY_SEPARATOR should be considered.

BUT, doing the former means that JPATH_BASE would also have to be converted to a standardized separator before that is str_replace()'d out of the path.

Unfortunately I do not have any testing environment on Windows anymore, and I currently do not plan to set up some again, so I could not help with testing / re-testing. I'll unsubscribe for this reason from this conversation. Wish you all good luck with solving that.

I think this PR is no longer required after the recent changes.

This is the existing codes in 3.5.1 -

        foreach (array($directory, JPATH_BASE) as $dir)
        {
            if (file_exists($dir . $icon))
            {
                $path = str_replace(JPATH_BASE, '', $dir);
                $path = str_replace('\\', '/', $path);
                $this->addFavicon(JUri::base(true) . $path . $icon);
                break;
            }
        }

Therefore closing it.