Bug 10646 - mod_rewrite should be enabled by the postinst script
Summary: mod_rewrite should be enabled by the postinst script
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-25 19:56 UTC by Tomás Cohen Arazi
Modified: 2015-06-04 23:24 UTC (History)
4 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10646: mod_rewrite should be enabled by the postinst script (2.07 KB, patch)
2013-07-25 20:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10646: mod_rewrite should be enabled by the postinst script (2.14 KB, patch)
2013-07-27 23:02 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 10646 - warn if mod_rewrite is not enabled (1.55 KB, patch)
2013-11-19 03:22 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 10646 - warn if mod_rewrite is not enabled (1.63 KB, patch)
2014-01-09 13:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2013-07-25 19:56:16 UTC

    
Comment 1 Tomás Cohen Arazi 2013-07-25 20:23:17 UTC Comment hidden (obsolete)
Comment 2 Mirko Tietgen 2013-07-27 23:02:15 UTC Comment hidden (obsolete)
Comment 3 Robin Sheat 2013-07-27 23:28:31 UTC
I don't think that we should change system configuration like this. What happens if it breaks something else on the system? 

I'd rather koha-create aborts with an error, or something like that.
Comment 4 Jared Camins-Esakov 2013-07-27 23:42:36 UTC
(In reply to Robin Sheat from comment #3)
> I don't think that we should change system configuration like this. What
> happens if it breaks something else on the system? 
> 
> I'd rather koha-create aborts with an error, or something like that.

I disagree. I think we'll have already broken whatever else could be broken by an Apache configuration change. We've already clobbered the heck out of whatever Apache configuration the user had before by uninstalling the user's existing Apache MPM (probably prefork or worker with Apache 2.2 and I guess event with Apache 2.4) and installing itk in its place, so we might as well just make Koha work at that point. That said, I don't have a particularly strong feeling that this is necessary, since the error message it pretty self-explanatory, I just think the horse has already left the barn.
Comment 5 Robin Sheat 2013-07-28 13:53:51 UTC
(In reply to Jared Camins-Esakov from comment #4)
> I disagree. I think we'll have already broken whatever else could be broken
> by an Apache configuration change. We've already clobbered the heck out of
> whatever Apache configuration the user had before by uninstalling the user's
> existing Apache MPM (probably prefork or worker with Apache 2.2 and I guess

That's all voluntary however, warnings are given.
Comment 6 Robin Sheat 2013-11-19 03:22:52 UTC Comment hidden (obsolete)
Comment 7 Robin Sheat 2013-11-19 03:23:46 UTC
This is, I think, the preferable way of solving this. koha-create will abort with an error if mod_rewrite isn't enabled.
Comment 8 Tomás Cohen Arazi 2014-01-09 13:26:38 UTC
Created attachment 24072 [details] [review]
Bug 10646 - warn if mod_rewrite is not enabled

Koha requires mod_rewrite. If it's not enabled in Apache, then
koha-create will now abort with a helpful error message. Also adds a
warning when not run as root to avoid confusing errors.

Test plan:
 * apply the patch
 * a2dismod rewrite
 * run koha-create without sudo, note the error about being root
 * run koha-create with sudo, note the error about rewrite
 * a2enmod rewrute
 * repeat test above, note that it works

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Works as expected.
Comment 9 Robin Sheat 2014-01-09 21:48:20 UTC
I'm going to pass QA on this because I wrote it, Tomás tested it, and it's packaging related :)
Comment 10 Galen Charlton 2014-01-10 16:13:12 UTC
Pushed to master.  Thanks, Robin!
Comment 11 Fridolin Somers 2014-02-08 09:02:45 UTC
(In reply to Mirko Tietgen from comment #2)
> This patch makes the post install scripts used by the .deb packages
> enable the mod_rewrite module (which is mandatory if you want to run Koha).
The mod_rewrite module is not mandatory anymore in Apache configuration (etc/koha-httpd.conf) since Bug 9683.
Why is it different when using packaged version ?
Comment 12 Robin Sheat 2014-02-09 21:48:43 UTC
(In reply to Fridolin SOMERS from comment #11)
> The mod_rewrite module is not mandatory anymore in Apache configuration
> (etc/koha-httpd.conf) since Bug 9683.
> Why is it different when using packaged version ?

In both the opac and the staff client, the following rewrite rules are applied:

RewriteCond %{QUERY_STRING} (.*?)(?:[A-Za-z0-9_-]+)=&(.*)
RewriteRule (.+) $1?%1%2 [N,R,NE]
RewriteRule ^/bib/([^\/]*)/?$ /cgi-bin/koha/detail\.pl?bib=$1 [PT]
RewriteRule ^/isbn/([^\/]*)/?$ /search?q=isbn:$1 [PT]
RewriteRule ^/issn/([^\/]*)/?$ /search?q=issn:$1 [PT]

I'm not sure if they're ever useful (from looking at bug 9683, I suspect not.) We could make them conditional, or we could drop them totally. I don't really mind either way.
Comment 13 Jared Camins-Esakov 2014-02-10 00:39:07 UTC
(In reply to Robin Sheat from comment #12)
> I'm not sure if they're ever useful (from looking at bug 9683, I suspect
> not.) We could make them conditional, or we could drop them totally. I don't
> really mind either way.

I would vote for making them conditional, because I know libraries that use /bib/ URLs, but I don't have a strong opinion, since that's easy enough to add manually.
Comment 14 Fridolin Somers 2014-02-10 07:53:15 UTC
(In reply to Robin Sheat from comment #12)
> (In reply to Fridolin SOMERS from comment #11)
> > The mod_rewrite module is not mandatory anymore in Apache configuration
> > (etc/koha-httpd.conf) since Bug 9683.
> > Why is it different when using packaged version ?
> 
> In both the opac and the staff client, the following rewrite rules are
> applied:
> 
> RewriteCond %{QUERY_STRING} (.*?)(?:[A-Za-z0-9_-]+)=&(.*)
> RewriteRule (.+) $1?%1%2 [N,R,NE]
> RewriteRule ^/bib/([^\/]*)/?$ /cgi-bin/koha/detail\.pl?bib=$1 [PT]
> RewriteRule ^/isbn/([^\/]*)/?$ /search?q=isbn:$1 [PT]
> RewriteRule ^/issn/([^\/]*)/?$ /search?q=issn:$1 [PT]
> 
> I'm not sure if they're ever useful (from looking at bug 9683, I suspect
> not.) We could make them conditional, or we could drop them totally. I don't
> really mind either way.
Yes but Bug 9683 added a  <IfModule mod_rewrite.c> around those lines. It means that those lines are not active is mod_rewrite if not enabled.
In my opinion, very few libs are using those rules.
But it can be kept has examples.
Comment 15 Tomás Cohen Arazi 2014-02-10 11:57:15 UTC
The thing is, anyone using Debian packages doesn't have (by default) the rewrite rules wrapped in a conditional. Those are the users that need to be taken into account in this specific bug.

This patch just captures the situation where mod_rewrite needs to be enabled by hand by the user, and prints a warning. Another option would be to make it conditional.

I'd be in favour of not changing the current behaviour (which would mean wrapping the rewrite rules), but adding this warning, for the stable branches at least. Maybe for 3.16 we could just have the rewrite rules match those for tar.gz installs.

Regards
To+
Comment 16 Fridolin Somers 2014-02-18 09:01:49 UTC
Patches pushed to 3.14.x, will be in 3.14.4.