Doesnt it already do that? In my test it does

@mustaqahmed Your pull request (PR) has several mistakes.

1. When you create a PR, the description already contains headings like "Summary of Changes", "Testing Instructions". They are not there for nothing. They are meant to be completed by the corresponding text.

You might find bad examples of PR's from other people where testing instructions are not provided. In some case this might be ok because it's simple changes only, but you should use them as example for your PR's. You should always provide a description of changes and testing instructions.

2. Your PR has code style errors. You can see that by going down to the bottom of the PR at GitHub and check the section "Some checks were not successful". Here now it shows that a step called "continuous-integration/drone/pr" has failed.

If you follow the "Details" link right beside that failed step, you will come to a page with a list of checks on the left hand side, failed ones in red and passed ones in green colour.

Here for this PR the "phpcs" step fails, which means "PHP code style". If you select that step in the list on the left hand side, the right part of the windows shows the log file with the details. You might have to scroll up a bit to see that. Here the link to this log: https://ci.joomla.org/joomla/joomla-cms/44426/1/3 .

It shows that you have used spaces instead ot tabulators to indent code.

You can also see that by the wrong indentation shown when you check the changes on GitHub: https://github.com/joomla/joomla-cms/pull/34221/files#diff-1f3d4b0ff411d275d94c1538ef487ef75d4450d772ad2cf712b5796467917f6dR684 .

These code style errors need to be fixed if this PR shall be accepted.

3. As @brianteeman already pointed out, there is no problem with the favicon in the root folder being not found, so please check if your PR really fixes any issue.

We require that a pull request has been tested by the author before it is submitted.

This brings us back to the 1st point, providing testing instructions.

When testing yourself it helps you to wrote the instructions.

@brianteeman @richard67, please take a look again. My first commit was done in a hurry only to understand why the simple fix was missed. I have now updated the PR to properly fix the lookup order and also fill the description fields.

IMHO the ability of a template to override a globally set favicon is welcome and does not need to change. Usually different templates provide new styling that also affects the site's branding.
Any anomaly caused by that (e.g. a template is shipped with a favicon), is caused by the implementation of the template developers and not by the feature itself.

@sakiss you miss the point. Joomla does not provide a favicon in the root so there is no globally set favicon. A site owner should be able to set a favicon in the root to safely override any favicon in the template directory without it being replaced on a template update. The only question here is does that happen right now and if not how do we make it happen

@brianteeman Thanks for the info. I am aware of that. My post was about keeping the template's ability to override a globally set favicon (i.e. the opposite of the goal of that PR). If we want to be consistent with that approach, we may have to remove the favicon file from the templates and provide a global one.

The only question here is does that happen right now and if not how do we make it happen

In my tests it works that way. The template's one overrides the global.

what globally set favicon?? I dont see one

I think "globally set" here means site owner creating the file root/favicon.ico. IMHO this file should override the template's icon because removing the icon in the template folder does not work today across updates.

Oh, BTW, I just spotted this landed PR doing the exact same thing but to 4.0! Not sure what it means for my PR.

Oh, BTW, I just spotted this landed PR doing the exact same thing but to 4.0! Not sure what it means for my PR.

@mustaqahmed It might mean that is was decided that the issue has to be fixed, but that the fix is a backwards compatibility (b/c) break and so had to go into version 4.

The discussion in issue #17629 linked by that PR looks as if this was the case.

@Bakual Do you remember why you had to rebase your PR #17677 to 4.0-.dev? Was it a b/c issue? In this case this PR here should be closed. Or was it not, and you think it could still be fixed in 3.x?

I did it from the beginning for 4.0 because it is a minor B/C issue. We should not change this in 3.x at this point.

@Bakual Thanks for feedback and confirmation.

@mustaqahmed As you can see in the comment above, we can't change that in Joomla 3 due to B/C (backwards compatibility) reasons. So I think your PR should be closed. That can happen sometimes, also to experienced contributors. I hope you are not too disappointed. I think this PR was at least a good training for making other PR's, and I'm looking forward to more contributions from you, and thank for your this one.

That's all fair. Thanks for the engaging discussion on my trial PR.