I dont know about windows apache servers but that would break the link to the favicon on a linux server

This is what you get when you introduce DIRECTORY_SEPARATOR. in Joomla. The code used to be platform agnostic, standardized on '/'. Now it needs DIRECTORY_SEPARATOR and conversion everywhere!

Breaking BC!

That has been broken long before DIRECTORY_SEPARATOR was (re-)introduced...

Strange, works lfor me in J3.3.6, some people call that REGRESSION!

Surprises me considering the defines use DIRECTORY_SEPARATOR to assemble the base paths while the "standardized on /" behavior only applies to paths appended to the base within Joomla.

But now they add DIRECTORY_SEPARATOR instead of '/' to the path. Any code, core or 3p, expect '/' since the removal of DS at the introduction of J3.

For consistency I would prefer to have PATH_BASE also use '/'. But the separator used there is irrelevant, it is the separator used after that which causes problems.

C:\xampp\htdocs\J3x\j336\administrator\templates\isis\favicon.ico
vs
C:\xampp\htdocs\J3x\j336/administrator/templates/isis/favicon.ico

The DIRECTORY_SEPARATOR patch that was committed to the defines only affects the first separator after the root path. So at worst, with 3.4 it should be C:\xampp\htdocs\J3x\j336\administrator/templates/isis/favicon.ico.

Quite frankly I don't think it's our responsibility to forcibly use / separators in every filesystem processing piece of code (and I argued the same before). The fact our code has issues with mixed separators to me indicates that our code has bugs that need addressed. The patch we're commenting on (#4433) shows there is an edge case where standardized / use in our own code still has issues, in the case of that patch and this specific issue because a literal string is being compared (and / != \).

That is what changed! From C:\xampp\htdocs\J3x\j336/administrator/templates/isis/favicon.ico.to C:\xampp\htdocs\J3x\j336\administrator/templates/isis/favicon.ico. for a bad reason! As explained then, to be ignored! That is why current code, that worked before now needs DIRETORY_SEPARATOR again.

BAD MOVE! Remember the problems introduced by removing DS, you are introducing it back!

A bad reason being a server configuration that failed with the pre-existing code? I get your point that using the constant can cause issues, but my point about forcing / is just as valid. So what's the compromise?

The worst thing you can do is revert to the situation that in Joomla code you need to cater for platform differences. This means the decission to remove DS was the right one, including standardize on '/' as separator. Any code currently available for J3 is working according to that.
As I objected to, introducing DIRECTORY_SEPARATOR ( even without DS ) is a BAD thing. Especially, because there is no need to!

Can't believe that something that breaks BC and causes bad publicity is so difficult to revert. Can't believe it was accepted in the first place!

You're right in that there is no need to use DIRECTORY_SEPARATOR when you're doing filesystem based operations (copy, delete, upload, etc.); PHP handles C:\xampp/htdocs\j336/administrator\components/com_foobar just fine. The bugs in Joomla's code as far as I'm aware in path operations relate to string literal comparisons or string literal functions, such as this use case, where there may be mixed separators. Sure, forcing every path ever used in Joomla to always have / separators solves that, but this scenario is not a strong enough reason for me to justify a PR like #5586 being accepted. In all actuality, something like this snippet would isolate this block from ever being broken because of a difference in the path separators:

$path = JPath::clean($dir, '/');
$this->addFavicon(JUri::base(true) . '/' . $path . 'favicon.ico');

Sure, you can fix any problem at code level. Using a CMS (Framework) I would expect not to be confronted with these problems. Especially when you have been told so with the removal of DS in J3.
As long as you are not introducing DIRECTORY_SEPARATOR here, it is fine with me.

That would still ignore the actual issue here. A PR was accepted that causes REGRESSION. Fortunately you provide an improvement to the code in core for this particular issue. What about all those 3p extensions? Are you going to fix them as well? Do you really want to upset the community again telling them you have changed your mind and want them to return back to using DS again?

Don't know about #5586, don't like that approach either. I am talking about reverting the PR that caused this problem. The one that introduced the DIRECTORY_SEPARATOR in includes.php for the subfolders of Joomla. The one I objected to! Remember that this is a problem introduced by that. It wasn't a problem before that!

Maybe not, but that patch also highlights how fragile that bit of code is and how easily it can be broken. My suggestion, whether the other PR is reverted or not, ensures this issue doesn't crop up regardless of any decision about the path separator.

This worked for me:
// Try to find a favicon by checking the template and root folder

    foreach (array($directory, JPATH_BASE) as $dir)
    {
        $icon = $dir . '/favicon.ico';

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

Can't get your suggestion with JPath::clean to work.

Correction:
foreach (array($directory, JPATH_BASE) as $dir)
{
$icon = $dir . '/favicon.ico';

        if (file_exists($icon))
        {
            $path = str_replace(JPATH_BASE, '', $dir);
            $path = JPath::clean($path, '/');
            $this->addFavicon(JUri::base(true) . $path . '/favicon.ico');
            break;
        }
    }

Works.

I closing here as we have a PR by @sovainfo Thanks!: #6081

Set to "closed" on behalf of @zero-24 by The JTracker Application at issues.joomla.org/joomla-cms/6077