Bug 20173 - Clean up koha-tmpl directory
Summary: Clean up koha-tmpl directory
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-12 02:44 UTC by David Cook
Modified: 2018-06-04 20:15 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20173 - Clean up koha-tmpl directory (2.55 KB, patch)
2018-02-12 02:45 UTC, David Cook
Details | Diff | Splinter Review
Bug 20173 - Clean up koha-tmpl directory (2.60 KB, patch)
2018-02-16 04:12 UTC, M. Tompsett
Details | Diff | Splinter Review
Fix favicon.ico path in installer (1.51 KB, patch)
2018-02-18 23:12 UTC, David Cook
Details | Diff | Splinter Review
Bug 20173: Clean up koha-tmpl directory (2.64 KB, patch)
2018-02-27 08:29 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 20173: Fix favicon.ico path in installer (1.57 KB, patch)
2018-02-27 08:30 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 20173: Clean up koha-tmpl directory (2.70 KB, patch)
2018-02-28 07:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20173: Fix favicon.ico path in installer (1.63 KB, patch)
2018-02-28 07:08 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2018-02-12 02:44:19 UTC
At the moment, there are the following files in git:

koha-tmpl/favicon.ico
koha-tmpl/index.html
koha-tmpl/intranet.html
koha-tmpl/opac.html

As far as I can tell, these files aren't actually used for anything. 

Looking at the git log, it looks like these are a holdover from 2003 when koha-tmpl replaced koha-html? 

Looks quite safe to remove these files now.
Comment 1 David Cook 2018-02-12 02:45:43 UTC
Created attachment 71408 [details] [review]
Bug 20173 - Clean up koha-tmpl directory

This patch removes some HTML and ico files that are no longer
used in Koha.
Comment 2 David Cook 2018-02-12 02:51:31 UTC
Note:

These files don't even make it through the build process of Koha, as far as I can tell.
Comment 3 M. Tompsett 2018-02-12 04:16:25 UTC
I confirmed the favicon.ico, but then your patch to remove things didn't include it.

I also noticed that the apache configuration files had ProxyPass, ProxyPassReverse, and ScriptAlias values related to index.html as well, so perhaps we should clean those up too?

And yes, intranet and opac html files are only in the index.html file.

I'll mark this "Failed QA" for definitely the first point, but I would like clarification on why not the second paragraph.
Comment 4 Katrin Fischer 2018-02-12 06:08:49 UTC
Why do you think the favicon is not used?
Comment 5 M. Tompsett 2018-02-12 13:33:55 UTC
(In reply to Katrin Fischer from comment #4)
> Why do you think the favicon is not used?

Because the templates only use the ones in the image subdirectories, not the koha-tmpl directory directly.
Comment 6 Katrin Fischer 2018-02-12 20:22:12 UTC
Owen confirmed on IRC that the favicon.ico can go. 

Do we have an idea if the rest of the files is needed or why they were added in the first place?
Comment 7 David Cook 2018-02-16 00:47:18 UTC
(In reply to M. Tompsett from comment #3)
> I confirmed the favicon.ico, but then your patch to remove things didn't
> include it.
> 

Yes it does. Look at "Splinter Review" then "Overview". It will mention that the binary file is deleted.

> I also noticed that the apache configuration files had ProxyPass,
> ProxyPassReverse, and ScriptAlias values related to index.html as well, so
> perhaps we should clean those up too?
> 

You're referring to the Apache/Plack configuration files? That's not referring to this index.html. That's referring to the default index.html that web browsers look for. 

> And yes, intranet and opac html files are only in the index.html file.
> 
> I'll mark this "Failed QA" for definitely the first point, but I would like
> clarification on why not the second paragraph.

You're wrong about the first point, although you could apply the patch to find that out for sure.
Comment 8 M. Tompsett 2018-02-16 01:57:30 UTC
(In reply to David Cook from comment #7)
> (In reply to M. Tompsett from comment #3)
> > I confirmed the favicon.ico, but then your patch to remove things didn't
> > include it.
> > 
> 
> Yes it does. Look at "Splinter Review" then "Overview". It will mention that
> the binary file is deleted.

I stand corrected. Okay. But could someone clarify the proper course of action with index.html?


> ... this index.html ... [is] ... referring to the default index.html
> that web browsers look for.

Right, so should we modify it or remove it? Leaving in discussion.
Comment 9 David Cook 2018-02-16 03:27:41 UTC
(In reply to M. Tompsett from comment #8)
> I stand corrected. Okay. But could someone clarify the proper course of
> action with index.html?
> 
> 
> > ... this index.html ... [is] ... referring to the default index.html
> > that web browsers look for.
> 
> Right, so should we modify it or remove it? Leaving in discussion.

We should remove it. In Apache, that index.html is just an alias to a Perl script.

Plus, the index.html that I'm removing isn't even in the DocumentRoot. Without some other directive to reference it, it's not going to be used by the web server.
Comment 10 M. Tompsett 2018-02-16 04:12:36 UTC
Created attachment 71695 [details] [review]
Bug 20173 - Clean up koha-tmpl directory

This patch removes some HTML and ico files that are no longer
used in Koha.

NOTE: index.html decision pushed up to QA people.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 11 Julian Maurice 2018-02-16 11:38:44 UTC
There is still a reference to this favicon.ico in koha-tmpl/intranet-tmpl/prog/en/includes/installer-doc-head-close.inc:

<link rel="shortcut icon" href="/favicon.ico" type="image/x-icon" />
Comment 12 David Cook 2018-02-18 22:31:20 UTC
(In reply to Julian Maurice from comment #11)
> There is still a reference to this favicon.ico in
> koha-tmpl/intranet-tmpl/prog/en/includes/installer-doc-head-close.inc:
> 
> <link rel="shortcut icon" href="/favicon.ico" type="image/x-icon" />

I don't really think that's worth failing something over, since that line will have generated an error since forever ago (since ./koha-tmpl/favicon.ico isn't in the DocumentRoot) but OK.
Comment 13 David Cook 2018-02-18 22:45:34 UTC
(In reply to Julian Maurice from comment #11)
> There is still a reference to this favicon.ico in
> koha-tmpl/intranet-tmpl/prog/en/includes/installer-doc-head-close.inc:
> 
> <link rel="shortcut icon" href="/favicon.ico" type="image/x-icon" />

So I'm actually finding something pretty weird here. I've applied the patch and this is what I see in my browser's network tab:

favicon.ico	302	x-perl	Other	257 B	1.39 s	648889

Looking at my Apache virtual host config... I see with a git install that "../git/koha-tmpl" is the document root, so I suppose ./koha-tmpl/favicon.ico would show in a git install. (Although in a Debian install the document roots should be "/usr/share/koha/intranet/htdocs" and "/usr/share/koha/opac/htdocs".)

In any case... the ./koha-tmpl/favicon.ico is gone and I see nothing in the Apache config that should be causing a redirect. 

I see the favicon in my browser and it seems to be downloading it and not retrieving it from a browser cache...
Comment 14 David Cook 2018-02-18 22:46:57 UTC
Created a fake favicon.ico and the browser is still showing the real Koha one... but I see the following in the network tab:

favicon.ico	200	x-icon	Other	269 B	3 ms	652379
Comment 15 David Cook 2018-02-18 22:50:23 UTC
If I try to visit the favicon, I see the following:

favicon.ico	200	document	Other	269 B	9 ms	655653	favicon.ico	200	x-icon	Other	(from disk cache)	1 ms	0

I reckon Chrome must be caching the favicon and using it on a host basis.
Comment 16 David Cook 2018-02-18 22:54:00 UTC
Whoa, this is cool/scary:

I created ./koha-tmpl/favicon2.ico and I'm visiting the URL: "<git install>/favicon2.ico" on Tab 2 of my browser. On Tab 1, I'm visiting "/cgi-bin/koha/installer/install.pl?step=1&op=updatestructure". 

I see the following in my network tab on tab 2:

favicon2.ico	200	document	Other	269 B	15 ms	656742	
favicon.ico	302	x-perl	Other	257 B	1.36 s	656742	
install.pl?step=1&op=updatestructure	200	text/html	favicon.ico	3.7 KB	1.38 s	656742

I reckon that Chrome is looking at other tabs to see if it has a favicon for that same host. That's creepy. I'll have to shutdown Chrome to test that theory...
Comment 17 David Cook 2018-02-18 23:05:19 UTC
OK I don't quite know how it's doing it, but it is caching super heavily. I just used the following "convert -size 200x200 xc:none -fill red -draw "circle 20,20 10,10 circle 100,100 50,50" favicon.ico" and that switched the favicon for the tab...and after deleting it from the server, it's still being used in the browser after multiple fresh loads...
Comment 18 David Cook 2018-02-18 23:08:01 UTC
Guess it doesn't really matter how it's doing it. I'll just update the file to be consistent with the other intranet pages. 

Interesting how browsers try to outsmart the servers though...
Comment 19 David Cook 2018-02-18 23:12:58 UTC
Created attachment 71917 [details] [review]
Fix favicon.ico path in installer

This patch corrects the favicon.ico path for the installer.

On git installs, it used to point to ./koha-tmpl/favicon.ico,
and on regular installs, it probably just didn't work.
Comment 20 Fridolin SOMERS 2018-02-27 08:29:49 UTC
Created attachment 72249 [details] [review]
Bug 20173: Clean up koha-tmpl directory

This patch removes some HTML and ico files that are no longer
used in Koha.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 21 Fridolin SOMERS 2018-02-27 08:30:08 UTC
Created attachment 72250 [details] [review]
Bug 20173: Fix favicon.ico path in installer

This patch corrects the favicon.ico path for the installer.

On git installs, it used to point to ./koha-tmpl/favicon.ico,
and on regular installs, it probably just didn't work.

https://bugs.koha-community.org/show_bug.cgi?id=20173
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 22 Fridolin SOMERS 2018-02-27 08:36:25 UTC
I signoff for a dev install : 
DocumentRoot is on /home/koha/src/koha-tmpl
The home page still works.

I have corrected commit message according to https://wiki.koha-community.org/wiki/Commit_messages
Comment 23 Katrin Fischer 2018-02-28 07:07:42 UTC
Favicons still display, can find no references to the files.
Comment 24 Katrin Fischer 2018-02-28 07:08:45 UTC
Created attachment 72285 [details] [review]
Bug 20173: Clean up koha-tmpl directory

This patch removes some HTML and ico files that are no longer
used in Koha.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2018-02-28 07:08:48 UTC
Created attachment 72286 [details] [review]
Bug 20173: Fix favicon.ico path in installer

This patch corrects the favicon.ico path for the installer.

On git installs, it used to point to ./koha-tmpl/favicon.ico,
and on regular installs, it probably just didn't work.

https://bugs.koha-community.org/show_bug.cgi?id=20173
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Jonathan Druart 2018-03-19 18:31:05 UTC
Pushed to master for 18.05, thanks to everybody involved!