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

[4.0] Change the lookup order for the favicon.ico #17677

Merged
merged 3 commits into from
May 19, 2019

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented Aug 22, 2017

Pull Request for Issue #17629 .

Summary of Changes

This PR changes the lookup order for the favicon so it first looks in the root folder and afterwards in the template one.
Currently, the lookup works the other way (first template, then root).

I think the current way has historic reasons because (I think) we used to ship a favicon.ico in root in earlier versions. The favicon in the template folder then allowed to override that favicon.

Today we don't ship a favicon in root, but we ship one in the template folders, thus it imho makes more sense to reverse the override place.

Testing Instructions

  • Put different favicon.ico files into the root folder and the active template folder.
  • Check which favicon is used

Expected result

The one from root is used, overriding the one from the template

Actual result

The one from the template is used, overriding the one from root.

Documentation Changes Required

Don't know if that hehavior is documented somewhere. If yes, it should be adjusted.
Also it should be documented as a changed behaviour between 3.x and 4.0.

@brianteeman
Copy link
Contributor

This needs documenting - not just as a change but as a feature - otherwise there is no point in the pr ;)

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Aug 22, 2017

The one from the template is used, overriding the one from root.

@Bakual I thought that templates are always the parts that make the final decision, e.g. js is overridden if there is file in template/js. So if we do this here then we have inconsistent behaviour, (unless I got this wrong)

EDIT: Ignore the above comment, this now aligns the behaviours

@Bakual Bakual changed the title Change the lookup order for the favicon.ico [4.0] Change the lookup order for the favicon.ico Aug 22, 2017
@mbabker
Copy link
Contributor

mbabker commented Aug 22, 2017

Generally, you're right. But, the favicon is an awkward thing. If a template is shipping one, how do you override it at the template level? The entire problem we're having is users are building templates either from core or from a template provider who ships a favicon as part of the template, meaning it'll be overwritten on update unless it's excluded from updates. So how do you local override a template file?

@ghost
Copy link

ghost commented Aug 23, 2017

@Bakual can you please resolve conflicting Files for test?

@Bakual
Copy link
Contributor Author

Bakual commented Aug 23, 2017

Rebased PR, should be fine for testing again.

@ghost
Copy link

ghost commented Aug 23, 2017

I have tested this item ✅ successfully on 9580794


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17677.

@kofaysi
Copy link
Contributor

kofaysi commented Oct 14, 2017

Thank you for considering the issue serious enough for a discussion.

I like to use of the user.ico file over the suggested root dir vs template dir location check. The reason for my preference is the following: On one standard site, two templates are being used -- one for the frontend (in protostar) and one for the administrator (backend, in isis). With the OP's original suggestion, two locations for two different favicon files are degraded to a single location (and single favicon). I'm using two favicons (both differing from the default Joomla favicon).

Note: The favicon for the backend is a visual negative of the frontend favicon.

Thank you for considering a different method and refraining from overwritting user favicons in the frontend template and backend template as well.

@Bakual
Copy link
Contributor Author

Bakual commented Dec 4, 2017

@kofaysi Backend of course would use a different favicon from the frontend anyway, since the "root" directories are different in that case. "Root" is a bit misleading since technicalls it's the "Base" directory.

@Bakual
Copy link
Contributor Author

Bakual commented Jan 8, 2018

Rebased this PR

@ghost ghost added the J4 Issue label Apr 5, 2019
@ghost ghost removed the J4 Issue label Apr 13, 2019
@sanderpotjer
Copy link
Member

I have tested this item ✅ successfully on f3ca2a5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17677.

@sanderpotjer
Copy link
Member

@franz-wohlkoenig maybe you can test this PR again after the latest rebase to get it merged

@ghost
Copy link

ghost commented May 5, 2019

@sanderpotjer i'm waiting for further on beta as there upgrades from nightly to nightly are possible.

@alikon
Copy link
Contributor

alikon commented May 6, 2019

I have tested this item ✅ successfully on f3ca2a5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17677.

@joomla-cms-bot joomla-cms-bot added J4 Issue RTC This Pull Request is Ready To Commit and removed PR-4.0-dev labels May 6, 2019
@alikon
Copy link
Contributor

alikon commented May 6, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17677.

@roland-d roland-d merged commit 7bc3e7e into joomla:4.0-dev May 19, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 19, 2019
@roland-d
Copy link
Contributor

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet