?
Referenced as Pull Request for: # 5566
avatar smz
smz
30 Dec 2014

Am I the only one having issues with favicon (both frontend and backend) with the latest staging code? I don't see it anymore or I see it randomly. I also don't see a request (GET) for it while monitoring network activity...

Have we changed something at this regard?

avatar smz smz - open - 30 Dec 2014
avatar losedk
losedk - comment - 30 Dec 2014

Hmm can't seem to reproduce this

avatar losedk
losedk - comment - 30 Dec 2014

Have you tried to enter the path to the favicon directly and refresh it?

avatar smz
smz - comment - 30 Dec 2014

@losedk You mean something like http://example.org/templates/protostar/favicon.ico ? If yes, then yes, I can see the favicon, but still in my FF tabs a generic icon is displayed for my site.

Try completely cleaning your cache...

avatar losedk
losedk - comment - 30 Dec 2014

Just tested in a completely fresh install of FF, still can't reproduce the issue :/

avatar smz
smz - comment - 30 Dec 2014

curious... I have several local virtual hosts and the only one where I see this issue is the one using 3.4.0-dev...

I'll do further testing, but in the meanwhile let's leave this issue open (maybe at low priority if someone want to change it: I can't) and see if someone else pops out with the same issue...

avatar smz
smz - comment - 30 Dec 2014

... I also seems to remember that something has been done to favicon with this rev, but I can't remember exactly what... (my poor neurons... :confused:)

avatar losedk
losedk - comment - 30 Dec 2014

hehe :) Yeah, just leave it open

avatar richard67
richard67 - comment - 30 Dec 2014

@smz Did you try with different browsers? Do you have any mod_rewrite directives in .htaccess regarding templates folders? Does clear cache of browser not help?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5559.
avatar smz
smz - comment - 30 Dec 2014

Hi Richard!

  • Yes: FF 34.0, Chrome 39, IE 11 and Safari 5.1.7
  • No, I have no special mod_rewrite directive: just plain stock Joomla .htaccess
  • No, cache clear (native for each browser and CCleaner) does not help
avatar smz
smz - comment - 30 Dec 2014

(logging off for some hours...)

avatar Bakual
Bakual - comment - 30 Dec 2014

From memory we don't overwrite the favicon anymore with an update. But I'd have to check to be sure.

avatar smz
smz - comment - 30 Dec 2014

@bakual Exactly! That was the one I was thinking of! (... actually "trying to think of"!)
It should be irrelevant in this context: I have the favicon.ico (the standard one)

avatar Bakual
Bakual - comment - 30 Dec 2014

By default, the browser will look in the root folder of the website for a favicon.
However Joomla automatically creates the correct links by checking the template and root folder for the file and creating the proper link in the document header. See https://github.com/joomla/joomla-cms/blob/f6c93e0294ba6e75b9604366f71bcf6b66970c04/libraries/joomla/document/html/html.php#L583

avatar smz
smz - comment - 30 Dec 2014

I think we have a real issue.

First of all I looked in the very obvious place (where I didn't before, shame on me!), the actual generated HTML, where the problem is evident:

  <link href="/D:/UniServerZ/vhosts/smztest/templates/protostar/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon" />

Then, thanks @Bakual that pointed me to the right code: please use it for reference while reading here below.

I dumped some variables and found that:

$directory = D:/UniServerZ/vhosts/smztest\templates
JPATH_BASE = D:/UniServerZ/vhosts/smztest

and hence the code to remove JPATH_BASE from $directory:

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

cannot work because D:/UniServerZ/vhosts/smztest/ != D:/UniServerZ/vhosts/smztest\

Tracing back where $directory is populated I landed to _fetchTemplate() where we have:

$directory = isset($params['directory']) ? $params['directory'] : 'templates';

Here... I'm a bit lost because I cannot understand where that 'directory' parameter comes from.
In any case it is the cause as it is set to D:/UniServerZ/vhosts/smztest\templates.

Substituting the above line of code in _fetchTemplate() with the following one solved the issue:

$directory = isset($params['directory']) ? str_replace('\\', '/', $params['directory']) : 'templates';

PR is on its way...

avatar brianteeman
brianteeman - comment - 30 Dec 2014

This really sounds like another issue with your uniserver setup that no one else can replicate.

Until anyone else can replicate ir then it definitely is not a serious issue.

avatar smz
smz - comment - 30 Dec 2014

PR is #5566

@brianteeman As you are raising a specific hypothesis, the onus of the proof is on you.

avatar brianteeman
brianteeman - comment - 30 Dec 2014

Well as no one else has yet confirmed the issue then sorry but no its not.
I have tested on all platforms available to me and cannot replicate
On 30 Dec 2014 23:51, "Sergio Manzi" notifications@github.com wrote:

PR is #5566 #5566

@brianteeman https://github.com/brianteeman As you are raising a
specific hypothesis, the onus of the proof is on you.


Reply to this email directly or view it on GitHub
#5559 (comment).

avatar smz
smz - comment - 31 Dec 2014

I traced back code and data, and demonstrated that a wrong parameter is stored in Joomla.

The conditions bringing to this can be many: OS version, PHP version, MySQL version, libraries version, who knows, maybe even the CPU...

Your hypothesis of Uniform Server being the cause is just that, an hypothesis, until proofed. If you insist on it without proofing it will become FUD.

On my side I also have the fact that at least other 5 virtual servers in the same Uniform Server installation do not present the issue.

avatar sovainfo
sovainfo - comment - 31 Dec 2014

This is one of the reasons I objected to the change of using DIRECTORY_SEPARATOR. Code in Joomla expects '/' as directory separator. The platform difference should not be introduced to Joomla. That would require to deal with it at all places instead on only on input!

avatar smz
smz - comment - 31 Dec 2014

@sovainfo It must be that somewhere, someone, builds that parameter by a concatenation of this kind:

$directory = JPATH_BASE . DIRECTORY_SEPARATOR . 'templates';

but then we try to strip out JPATH_BASE . '/' and we of course fail.

I'm still trying to find where the error is in the first place ("the original sin"), but sanitizing $params['directory'] on load can't do any harm, for sure!!

avatar smz
smz - comment - 31 Dec 2014

Here are a few ones:

capture

I'd really like to understand how this can affect only my server...

avatar sovainfo
sovainfo - comment - 31 Dec 2014

The standard includes/defines.php is doing that. look at the pr's that changed it. The correct code used to be '/templates', '/cache' ....

avatar brianteeman
brianteeman - comment - 31 Dec 2014

See #4433

On 31 December 2014 at 00:47, sovainfo notifications@github.com wrote:

The standard includes/defines.php is doing that. look at the pr's that
changed it. The correct code used to be '/templates', '/cache' ....


Reply to this email directly or view it on GitHub
#5559 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar smz
smz - comment - 31 Dec 2014

Correct, @sovainfo! PR #4433 of October 2

avatar smz
smz - comment - 31 Dec 2014

So... was this an Uniform Server issue? :stuck_out_tongue_winking_eye:

avatar smz
smz - comment - 31 Dec 2014

Now I'm calling in PLT to decide which is the correct fix: either revert #4433 (IMHO the right thing to do...) OR go bug hunting for every instance where we concatenate '/' to a path and change it to DIRECTORY_SEPARATOR.

In any case my PR is probably useless as it is some other str_replace(\, '/', $whatever); spread around for good measure...

avatar sovainfo
sovainfo - comment - 31 Dec 2014

@smz Thanks for proving my point.
Sounds like PLT already decided judging the posts of mbabker and wilsong!

avatar smz
smz - comment - 31 Dec 2014

The PLT, facing an obvious case where that PR caused a regression, can always change its mind...

AFAIK in PHP there is actually no need to use DIRECTORY_SEPARATOR as all file functions will account for specific local OS differences and apply the needed modifications automatically.

avatar smz
smz - comment - 31 Dec 2014

PLT, think of this: how many 3rd party extensions you risk to break with that PR? Talk about B/C...

avatar smz
smz - comment - 31 Dec 2014

Please also note that under Windows JPATH_BASE defined as __DIR__, all __DIR__, and all __FILE__ instances are converted by PHP with paths with normal slashes / and adding DIRECTORY_SEPARATOR to those will create "mixed style" paths which are IMO a really-bad-bad-thing.

If there are edge cases under IIS where this is causing issues, those edge cases should be solved specifically in the context of IIS

avatar mbabker
mbabker - comment - 31 Dec 2014

The server environment Phil was able to create the original issue on proves
that PHP does not always handle / separators well. In the case of that PR,
the proposed change fixed an edge case without breaking anything obvious at
the time. Circumstances have changed since then.

There are known path handling issues in the Joomla code base. Even the
unit test suite fails on Windows because of this. So our options at the
framework and API levels are to fix the issues or discontinue support for
Windows. And the latter is not an option.

On Tuesday, December 30, 2014, Sergio Manzi notifications@github.com
wrote:

PLT, think of this: how many 3rd party extensions you risk to break with
that PR? Talk about B/C...


Reply to this email directly or view it on GitHub
#5559 (comment).

avatar smz
smz - comment - 31 Dec 2014

@mbabker, I think that the fact that PHP internal functions do returns paths with normal slashes should be strong hint for us on which is the right choice...

avatar mbabker
mbabker - comment - 31 Dec 2014

Also this is not entirely a IIS versus Apache issue on Windows. When I ran
a Windows system for testing, I would have test failures running our test
suite on XAMPP.

On Tuesday, December 30, 2014, Michael Babker mbabker@flbab.com wrote:

The server environment Phil was able to create the original issue on
proves that PHP does not always handle / separators well. In the case of
that PR, the proposed change fixed an edge case without breaking anything
obvious at the time. Circumstances have changed since then.

There are known path handling issues in the Joomla code base. Even the
unit test suite fails on Windows because of this. So our options at the
framework and API levels are to fix the issues or discontinue support for
Windows. And the latter is not an option.

On Tuesday, December 30, 2014, Sergio Manzi > wrote:

PLT, think of this: how many 3rd party extensions you risk to break with
that PR? Talk about B/C...


Reply to this email directly or view it on GitHub
#5559 (comment).

avatar smz
smz - comment - 31 Dec 2014

There may be some other bugs hidden somewhere where using DIRECTORY_SEPARATOR does indeed solves the issue, but that doesn't automatically and "per se" means that DIRECTORY_SEPARATOR is the good choice...

avatar mbabker
mbabker - comment - 31 Dec 2014

Not saying it is either. But in general, I'm saying we need to address
areas in our code where there are issues on Windows when it comes to
filesystem operations.

On Tuesday, December 30, 2014, Sergio Manzi notifications@github.com
wrote:

There may be some other bugs hidden somewhere where using
DIRECTORY_SEPARATOR does indeed solves the issue, but that doesn't
automatically and "per se" means that DIRECTORY_SEPARATOR is the good
choice...


Reply to this email directly or view it on GitHub
#5559 (comment).

avatar smz
smz - comment - 31 Dec 2014

I absolutely agree! But first let's decide which is the correct way to go, standardize on it, and proceed. I don't want to be too pushy and repeat myself too often, but again: look at what PHP itself does...

avatar mbabker
mbabker - comment - 31 Dec 2014

We've basically standardized on / in our code. There are places where that has caused issues, so let's address those issues now.

Also, just to make clear since I still see people mentioning my name with the group, I'm not PLT anymore and try not to get involved in decision making matters very much now. I'm just another (loud) voice in the crowd :stuck_out_tongue:

avatar smz
smz - comment - 31 Dec 2014

:stuck_out_tongue_winking_eye: got it! ... but you surely can bring PLT attention on this...

avatar smz
smz - comment - 31 Dec 2014

Really, I think the odds of breaking 3rd party templates are quite high here...

avatar smz
smz - comment - 31 Dec 2014

Sorry but I made false assumptions here above: under Windows __DIR__ and __FILE__ returns path with the backslash separator (\)

I've been tricked by the fact that JPATH_BASE (which has forward slashes) is defined as define('JPATH_BASE', __DIR__); in /administrator/index.php, but for frontend (/index.php) it is defined as define('JPATH_BASE', str_replace('\\', '/', __DIR__));... OMG!

I'm about trying to make global code modifications getting rid of DIRECTORY_SEPARATOR, standardizing on / for separator and check all contexts where __DIR__ and __FILE__ are used... I'll let you know if I can produce something working...

avatar smz
smz - comment - 31 Dec 2014

#5569 is my "ambitious" PR where I try to standardize on '/' as the directory separator.
Basic things in frontend and backend seems to work...

avatar brianteeman
brianteeman - comment - 2 Jan 2015

Closed see #5580 and/or #5566


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5559.
avatar brianteeman brianteeman - change - 2 Jan 2015
Status New Closed
Closed_Date 0000-00-00 00:00:00 2015-01-02 20:17:35
Closed_By brianteeman
avatar brianteeman brianteeman - close - 2 Jan 2015
avatar zero-24 zero-24 - close - 2 Jan 2015
avatar brianteeman brianteeman - change - 2 Jan 2015
Closed_Date 2015-01-02 20:17:35 2015-01-02 20:17:36
avatar zero-24 zero-24 - change - 7 Jul 2015
Labels Added: ?

Add a Comment

Login with GitHub to post a comment