Created attachment 19952 [details] [review] Bug 10646: mod_rewrite should be enabled by the postinst script 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). It tests the module isn't already enabled, and enables if necesary, and reloads Apache configuration. To test: - Install current koha-common packages on a fresh setup. Make sure you have mod_rewrite disabled (its the default if you didn't have apache and it was just pulled by koha-common at least on Ubuntu 12.04). Run $ a2dismod rewrite to make sure it is disabled on your test box. Create a new koha instance like this: $ koha-create --create-db name You'll get an error like this: Syntax error on line 76 of /etc/koha/apache-shared.conf: Invalid command 'RewriteEngine', perhaps misspelled or defined by a module not included in the server configuration Action 'configtest' failed. The Apache error log may have more information. ...fail! , build your own packages, make sure you have mod_rewrite disabled (which is the default on Ubuntu 12.04 at least), and install the package. Create a new instance issuing: - Apply the patch, build your packages, repeat: No apache warning about the rewrite module. Regards To+ Sponsored-by: Universidad Nacional de Cordoba
Created attachment 19969 [details] [review] Bug 10646: mod_rewrite should be enabled by the postinst script 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). It tests the module isn't already enabled, and enables if necesary, and reloads Apache configuration. To test: - Install current koha-common packages on a fresh setup. Make sure you have mod_rewrite disabled (its the default if you didn't have apache and it was just pulled by koha-common at least on Ubuntu 12.04). Run $ a2dismod rewrite to make sure it is disabled on your test box. Create a new koha instance like this: $ koha-create --create-db name You'll get an error like this: Syntax error on line 76 of /etc/koha/apache-shared.conf: Invalid command 'RewriteEngine', perhaps misspelled or defined by a module not included in the server configuration Action 'configtest' failed. The Apache error log may have more information. ...fail! , build your own packages, make sure you have mod_rewrite disabled (which is the default on Ubuntu 12.04 at least), and install the package. Create a new instance issuing: - Apply the patch, build your packages, repeat: No apache warning about the rewrite module. Regards To+ Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
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.
(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.
(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.
Created attachment 23013 [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
This is, I think, the preferable way of solving this. koha-create will abort with an error if mod_rewrite isn't enabled.
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.
I'm going to pass QA on this because I wrote it, Tomás tested it, and it's packaging related :)
Pushed to master. Thanks, Robin!
(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 ?
(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.
(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.
(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.
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+
Patches pushed to 3.14.x, will be in 3.14.4.