Add favicon to template app to fix Rails 7.2 test suite run #8461
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8461 +/- ##
=======================================
Coverage 99.11% 99.11%
=======================================
Files 141 141
Lines 4069 4069
=======================================
Hits 4033 4033
Misses 36 36 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I don't think we need to do this with a partial and changing the layout. I know browsers will request that favicon file regardless. I believe we just need to include a dummy public/favicon.ico image in the generated app and that should resolve the issue. It shouldn't have to be user facing.
|
@mgrunberg by including a dummy |
|
@javierjulio can you recondider? Trusting on favicon existance forces all new projects to install our views or tweak the favico (if they notice the missing icon). In this sense, I prefere to render the icon meta if assests exists. As a non so strong argument, I don't use favicon nor logo.png (new default). I don't want to install the head just for these. All that been said, despite the extra effort, I'm not against fix the test in one PR and solve user facing problems in another. |
|
There isn't a user facing problem here though. There is no need to configure a favicon in the layout for them, they are expected to do that if they have one. They can update it as they need to. I don't understand why we'd want to configure something for them. It's not something we need to maintain in the layout file we copy over either. We just need it to fix our test suite where we are using the |
We have some specs failing in Rails 7.2. The only hint we have in the log is ``` No route matches [GET] "/favicon.ico" (ActionController::RoutingError) ``` Rails 7.2 new app generator does not use favicon.ico anymore. It uses a png + meta "icon" tag. Let's configure the icon in the template app to prevent this kind of error.
9717276 to
f92213e
Compare
|
I understand that you think adding the favicon back is the simplest change to fix the issues and I changed the PR to reflect that. My point is that our layout uses an old Rails default which increases the chances that users need to make a customization. But site icon per see could require a customization anyway (I'm not using any of the Rails defaults for site icon). Customizations are not good or wrong. After much thought, I realize I'm inclined to prevent customizations mostly because of the javascript we have on the head. So, I opened another PR (#8462) to change the only thing that makes me wonder about head customization. I hope you find both PR ok now. |
|
@mgrunberg ok great, thank you! Yes, this all we need, the simplest solution for now. |
Our test suit fails due to
Our head partial does not define any
icontag which makes the browser requestfavicon.ico.Rails 7.2 changed the icon of the new generated app from
favicon.icoto a PNG file.