Description
Jesse Weaver
2016-05-13 22:24:37 UTC
Created attachment 51493 [details] [review] Bug 16520: Allow per-VirtualHost environment variables with Plack This allows MEMCACHED_*, OVERRIDE_SYSPREF_* and others to work properly. Test plan: 1) Add the following line to your plack.psgi (near the bottom, just above "mount ..."): enable "+Koha::Middleware::Plack"; 2) Load the OPAC advanced search page (under Plack). The title should read "Koha online catalog" (or whatever your LibraryName syspref contains). 3) Add the following to your Apache configuration: RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName Potato\, Potato" 4) Restart Apache. 5) Refresh. The title should now read "Potato, Potato online catalog". Created attachment 51494 [details] [review] Bug 16520: Initialize the sysprefs Koha::Cache on demand This is needed so that the cache singleton doesn't get created before the MEMCACHED_* environment variables are set. Created attachment 51495 [details] [review] Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files This was not added to the old etc/koha-httpd.conf because that file has no infrastructure for Plack anyway. Created attachment 51496 [details] [review] Bug 16520: Fix the Apache Plack config to load the main page through Plack Previously, the ProxyPass would not pass the OPAC/staff homepage to Plack. This fixes this problem (which is necessary for the other changes on this bug to show on the main page). Created attachment 51576 [details] [review] Bug 16520: Allow per-VirtualHost environment variables with Plack This allows MEMCACHED_*, OVERRIDE_SYSPREF_* and others to work properly. Test plan: 1) Add the following line to your plack.psgi (near the bottom, just above "mount ..."): enable "+Koha::Middleware::Plack"; 2) Load the OPAC advanced search page (under Plack). The title should read "Koha online catalog" (or whatever your LibraryName syspref contains). 3) Add the following to your Apache configuration: RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName Potato\, Potato" 4) Restart Apache. 5) Refresh. The title should now read "Potato, Potato online catalog". Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 51577 [details] [review] Bug 16520: Initialize the sysprefs Koha::Cache on demand This is needed so that the cache singleton doesn't get created before the MEMCACHED_* environment variables are set. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 51578 [details] [review] Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files This was not added to the old etc/koha-httpd.conf because that file has no infrastructure for Plack anyway. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 51579 [details] [review] Bug 16520: Fix the Apache Plack config to load the main page through Plack Previously, the ProxyPass would not pass the OPAC/staff homepage to Plack. This fixes this problem (which is necessary for the other changes on this bug to show on the main page). Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 51580 [details] [review] Bug 16520: (followup) use __MEMCACHED_NAMESPACE__ where appropriate Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I tested this patches and I verified they correctly make the ENV variables reach the plack backend. Thus, the OVERRIVE_SYS* work again, and the memcached variables are correctly set. To verify the latter, I wrote a small patch that would go to another bug to display memcached configuration/status in about.pl. Jesse, It does not work for me, could you please have a look at the test plan and make sure all tests are listed? For instance, it seems that debian/templates/apache-shared-intranet-plack.conf and debian/templates/apache-shared-opac-plack.conf need to be copied (I did but still does not work). Code review: Should not we test if the headers mod is enabled in debian/templates/apache-site.conf.in? (In reply to Jonathan Druart from comment #11) > Jesse, > It does not work for me, could you please have a look at the test plan and > make sure all tests are listed? s/tests/steps Jonathan, what i did for testing was applying the changes manually to my kohadevbox instance. I agree the headers module should be correctly checked. The patched config files worked for me. (In reply to Tomás Cohen Arazi from comment #13) > Jonathan, what i did for testing was applying the changes manually to my > kohadevbox instance. I agree the headers module should be correctly checked. > The patched config files worked for me. Yep I have double checked everything and it now works. I have to admit I am a bit lost with all the apache config files, but should not we move the RequestHeader add X-Koha-SetEnv lines to apache-shared-[intranet|opac]-plack.conf files? In C4::Context->preference, the lc $var has been moved after the OVERRIDE_SYSPREF_* check, I don't think it's expected. 108 # We also add the MEMCACHED_ settings to the actual environment, to make sure any early 109 # initialization of Koha::Cache correctly sets up a memcached connection. 110 foreach my $special_var ( qw( MEMCACHED_SERVERS MEMCACHED_NAMESPACE ) ) { 111 $ENV{$special_var} = $setenvs{$special_var} if defined $setenvs{$special_var}; 112 } Jesse, could you explain in which cases this is useful? (In reply to Jonathan Druart from comment #15) > I have to admit I am a bit lost with all the apache config files, but should > not we move the RequestHeader add X-Koha-SetEnv lines to > apache-shared-[intranet|opac]-plack.conf files? I'm inclined to agree; at first I was thinking to have an example of the new syntax in the configuration files, but given that this should be wrapped in an <IfModule>, it should be moved there. (In reply to Jonathan Druart from comment #16) > In C4::Context->preference, the lc $var has been moved after the > OVERRIDE_SYSPREF_* check, I don't think it's expected. This is in tune with the documentation and how this feature has been historically used; see http://manual.koha-community.org/3.22/en/administration.html#globalsysprefs . (In reply to Jonathan Druart from comment #17) > 108 # We also add the MEMCACHED_ settings to the actual environment, to > make sure any early > 109 # initialization of Koha::Cache correctly sets up a memcached > connection. > 110 foreach my $special_var ( qw( MEMCACHED_SERVERS MEMCACHED_NAMESPACE > ) ) { > 111 $ENV{$special_var} = $setenvs{$special_var} if defined > $setenvs{$special_var}; > 112 } > > Jesse, could you explain in which cases this is useful? C4::Auth, in my testing, loads up some system preferences early on that cause Koha::Cache to be initialized before the Plack environment is copied to %ENV (presumably by the Plack CGI wrapper?). I was poking at this, and one issue with moving those sysprefs is that the apache-*-plack.conf files are not .in files, so __MEMCACHED_NAMESPACE__ is not replaced in them. I could rejigger that, but any strenuous objections to leaving the RequestHeader lines where I currently have them? Created attachment 56309 [details] [review] Bug 16520: Allow per-VirtualHost environment variables with Plack This allows OVERRIDE_SYSPREF_* and others to work properly. Test plan: 1) Add the following line to your plack.psgi (near the bottom, just above "mount ..."): enable "+Koha::Middleware::Plack"; 2) Load the OPAC advanced search page (under Plack). The title should read "Koha online catalog" (or whatever your LibraryName syspref contains). 3) Add the following to your Apache configuration: RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName Potato\, Potato" 4) Restart Apache. 5) Refresh. The title should now read "Potato, Potato online catalog". Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 56310 [details] [review] Bug 16520: Initialize the sysprefs Koha::Caches on demand Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 56311 [details] [review] Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files This was not added to the old etc/koha-httpd.conf because that file has no infrastructure for Plack anyway. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 56312 [details] [review] Bug 16520 (Follow up) Rebase fixes Created attachment 56397 [details] [review] Bug 16520: Allow per-VirtualHost environment variables with Plack This allows OVERRIDE_SYSPREF_* and others to work properly. Test plan: 1) Add the following line to your plack.psgi (near the bottom, just above "mount ..."): enable "+Koha::Middleware::Plack"; 2) Load the OPAC advanced search page (under Plack). The title should read "Koha online catalog" (or whatever your LibraryName syspref contains). 3) Add the following to your Apache configuration: RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName Potato\, Potato" 4) Restart Apache. 5) Refresh. The title should now read "Potato, Potato online catalog". Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Pushed to Master - Should be in the November 2016 release. Thanks! Created attachment 56410 [details] [review] Bug 16520: Allow per-VirtualHost environment variables with Plack This allows OVERRIDE_SYSPREF_* and others to work properly. Test plan: 1) Add the following line to your plack.psgi (near the bottom, just above "mount ..."): enable "+Koha::Middleware::Plack"; 2) Load the OPAC advanced search page (under Plack). The title should read "Koha online catalog" (or whatever your LibraryName syspref contains). 3) Add the following to your Apache configuration: RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName Potato\, Potato" 4) Restart Apache. 5) Refresh. The title should now read "Potato, Potato online catalog". Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 56411 [details] [review] Bug 16520: Initialize the sysprefs Koha::Caches on demand Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 56412 [details] [review] Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files This was not added to the old etc/koha-httpd.conf because that file has no infrastructure for Plack anyway. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 56413 [details] [review] Bug 16520 (Follow up) Rebase fixes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I would have liked to take a look at this one before pushing it. Especially because it modifies the syspref cache code. (In reply to Jonathan Druart from comment #30) > I would have liked to take a look at this one before pushing it. Especially > because it modifies the syspref cache code. Would have been good to launch the tests: prove t/db_dependent/Context.t t/db_dependent/Context.t .. 1/? Can't call method "set_in_cache" on an undefined value at /home/vagrant/kohaclone/C4/Context.pm line 533. Caused by commit 174769e382dfd5205595a5b9ae7b58c8ebf5d850 Bug 16520 (Follow up) Rebase fixes Please fix it ASAP Created attachment 57372 [details] [review] Bug 16520 - (Follow up) Restore retrieval of syspref_cache Created attachment 57383 [details] [review] Bug 16520 - (Follow up) Restore retrieval of syspref_cache Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Followup pushed to master thanks Nick! Reading this bug, I thought I had to do thos step in order to be able to set per-OPAC variables in my Apache config: Add the following line to your plack.psgi (near the bottom, just above "mount ..."): enable "+Koha::Middleware::Plack"; But there is no such thing as Koha::Middleware::Plack, so adding the line made this error show up in the Plack error log: Error while loading /etc/koha/sites/x/plack.psgi: Can't locate Koha/Middleware/Plack.pm in @INC (you may need to install the Koha::Middleware::Plack module) (@INC contains: /usr/share/koha/lib /usr/share/koha/installer /usr/share/koha/lib/installer /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.20.2 /usr/local/share/perl/5.20.2 /usr/lib/x86_64-linux-gnu/perl5/5.20 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.20 /usr/share/perl/5.20 /usr/local/lib/site_perl .) at /usr/share/perl5/Plack/Util.pm line 23, <DATA> line 309. Turns out Koha::Middleware::Plack is not needed, all you need is already in the out-of-the-box .psgi file. Not sure why. Maybe Koha::Middleware::Plack was just a typo for Koha::Middleware::SetEnv all along? http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=1e6df24d6cbf44c865805712e6a15ecc601273aa |