Description
Kyle M Hall (khall)
2018-05-08 14:51:37 UTC
Created attachment 75200 [details] [review] Bug 20727: Replace usage of File::Spec->tmpdir with Koha::UploadedFile->temporary_directory As explained in bug 20428 use tmpdir can cause issues and it just makes sense to standardize our temp directory in a universal way. Test Plan: 1) Apply this patch 2) Verify you can still log in and use Koha 3) Verify the web installer still works 4) Verify EDI module can still download files via FTP 5) Verify fines.pl still runs with -o option 6) prove t/db_dependent/Plugins.t 7) prove t/db_dependent/Sitemapper.t 8) prove t/db_dependent/Templates.t +my $dir = Koha::UploadedFile->temporary_directory; I think it should be moved to some more general module? I had thought about that as well. Where would you want to move it? Koha.pm? (In reply to Marcel de Rooy from comment #2) > +my $dir = Koha::UploadedFile->temporary_directory; > > I think it should be moved to some more general module? (In reply to Kyle M Hall from comment #3) > I had thought about that as well. Where would you want to move it? Koha.pm? Yeah. Definitely better.. Note that I commented too on 20428 that imo we need a general koha-conf variable for an alternative tmp directory.. (In reply to Marcel de Rooy from comment #4) > (In reply to Kyle M Hall from comment #3) > > I had thought about that as well. Where would you want to move it? Koha.pm? > > Yeah. Definitely better.. Note that I commented too on 20428 that imo we > need a general koha-conf variable for an alternative tmp directory.. I'll update this patch after bug 20428 has been pushed. (In reply to Kyle M Hall from comment #5) > (In reply to Marcel de Rooy from comment #4) > > (In reply to Kyle M Hall from comment #3) > > > I had thought about that as well. Where would you want to move it? Koha.pm? > > > > Yeah. Definitely better.. Note that I commented too on 20428 that imo we > > need a general koha-conf variable for an alternative tmp directory.. > > I'll update this patch after bug 20428 has been pushed. Ping, 20428 has been pushed :) Created attachment 75626 [details] [review] Bug 20727: Move temporary_directory() to C4::Context Looking here now Created attachment 75667 [details] [review] Bug 20727: Replace usage of File::Spec->tmpdir with Koha::UploadedFile->temporary_directory As explained in bug 20428 use tmpdir can cause issues and it just makes sense to standardize our temp directory in a universal way. Test Plan: 1) Apply this patch 2) Verify you can still log in and use Koha 3) Verify the web installer still works 4) Verify EDI module can still download files via FTP 5) Verify fines.pl still runs with -o option 6) prove t/db_dependent/Plugins.t 7) prove t/db_dependent/Sitemapper.t 8) prove t/db_dependent/Templates.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 75668 [details] [review] Bug 20727: Move temporary_directory() to C4::Context Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 75669 [details] [review] Bug 20727: (QA follow-up) Fix Upload.t and Plugins.t Trivial changes. Adding C4::Context->temporary_directory to subsequent tmpdir call in Plugins. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 75670 [details] [review] Bug 20727: (QA follow-up) Remove a few use statements again We do no longer need "use Koha::UploadedFile" in a few places. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Do not we need to update the occurrence in updatedatabase.pl as well? installer/data/mysql/updatedatabase.pl:my $tmp= File::Spec->tmpdir.'/koha_upload'; Created attachment 75833 [details] [review] Bug 20727: Replace use of tmpdir in updatedatabase.pl Created attachment 75834 [details] [review] Bug 20727: Add use line for File::Spec in C4::Context, sort used modules Created attachment 75835 [details] [review] Bug 20727: Add use line for File::Spec in C4::Context, sort used modules Created attachment 75923 [details] [review] Bug 20727: Replace usage of File::Spec->tmpdir with Koha::UploadedFile->temporary_directory As explained in bug 20428 use tmpdir can cause issues and it just makes sense to standardize our temp directory in a universal way. Test Plan: 1) Apply this patch 2) Verify you can still log in and use Koha 3) Verify the web installer still works 4) Verify EDI module can still download files via FTP 5) Verify fines.pl still runs with -o option 6) prove t/db_dependent/Plugins.t 7) prove t/db_dependent/Sitemapper.t 8) prove t/db_dependent/Templates.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 75924 [details] [review] Bug 20727: Move temporary_directory() to C4::Context Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 75925 [details] [review] Bug 20727: (QA follow-up) Fix Upload.t and Plugins.t Trivial changes. Adding C4::Context->temporary_directory to subsequent tmpdir call in Plugins. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 75926 [details] [review] Bug 20727: (QA follow-up) Remove a few use statements again We do no longer need "use Koha::UploadedFile" in a few places. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 75927 [details] [review] Bug 20727: Replace use of tmpdir in updatedatabase.pl Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 75928 [details] [review] Bug 20727: Add use line for File::Spec in C4::Context, sort used modules Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Awesome work all! Pushed to master for 18.11.x Architectural enhancement, not backporting for 18.05.x series. When I run "perl -c C4/Auth.pm", I get the following: dcook@kohaprod:/kohawebs/dev/dcook/git> perl -c C4/Auth.pm Subroutine psgi_env redefined at C4/Auth.pm line 50. Subroutine safe_exit redefined at C4/Auth.pm line 52. Subroutine get_template_and_user redefined at C4/Auth.pm line 142. Subroutine _version_check redefined at C4/Auth.pm line 715. Subroutine _session_log redefined at C4/Auth.pm line 761. Subroutine _timeout_syspref redefined at C4/Auth.pm line 768. Subroutine checkauth redefined at C4/Auth.pm line 778. Subroutine check_api_auth redefined at C4/Auth.pm line 1387. Subroutine check_cookie_auth redefined at C4/Auth.pm line 1629. Bareword "C4::Context::temporary_directory" not allowed while "strict subs" in use at C4/Auth.pm line 1750. BEGIN not safe after errors--compilation aborted at C4/Auth.pm line 1928. If I change "C4::Context::temporary_directory" to "C4::Context::temporary_directory()", that error goes away and compilation is fine. I'm guessing no one else has experienced this? When I run "perl -c C4/Auth.pm", I get the following: perl -c C4/Auth.pm Subroutine psgi_env redefined at C4/Auth.pm line 50. Subroutine safe_exit redefined at C4/Auth.pm line 52. Subroutine get_template_and_user redefined at C4/Auth.pm line 142. Subroutine _version_check redefined at C4/Auth.pm line 715. Subroutine _session_log redefined at C4/Auth.pm line 761. Subroutine _timeout_syspref redefined at C4/Auth.pm line 768. Subroutine checkauth redefined at C4/Auth.pm line 778. Subroutine check_api_auth redefined at C4/Auth.pm line 1387. Subroutine check_cookie_auth redefined at C4/Auth.pm line 1629. Bareword "C4::Context::temporary_directory" not allowed while "strict subs" in use at C4/Auth.pm line 1750. BEGIN not safe after errors--compilation aborted at C4/Auth.pm line 1928. If I change "C4::Context::temporary_directory" to "C4::Context::temporary_directory()", that error goes away and compilation is fine. I'm guessing no one else has experienced this? (In reply to David Cook from comment #26) > I'm guessing no one else has experienced this? For some reason, when I use the web app, I don't get any errors. I wonder if we're suppressing them unintentionally in whatever is loading C4::Auth? I see C4::Context turns off the warnings for redefined functions when there's a HTTP_USER_AGENT environmental variable, so that explains why the warnings don't show up in the web server logs... I wonder if Modern::Perl is up to something here... That's not it either as I have a newer CGI version than 4.08... HTTP_USER_AGENT=stuff perl -c C4/Auth.pm [Thu Oct 18 11:16:58 2018] Auth.pm: Subroutine psgi_env redefined at C4/Auth.pm line 50. [Thu Oct 18 11:16:58 2018] Auth.pm: Subroutine safe_exit redefined at C4/Auth.pm line 52. [Thu Oct 18 11:16:58 2018] Auth.pm: Subroutine get_template_and_user redefined at C4/Auth.pm line 142. [Thu Oct 18 11:16:58 2018] Auth.pm: Subroutine _version_check redefined at C4/Auth.pm line 715. [Thu Oct 18 11:16:58 2018] Auth.pm: Subroutine _session_log redefined at C4/Auth.pm line 761. [Thu Oct 18 11:16:58 2018] Auth.pm: Subroutine _timeout_syspref redefined at C4/Auth.pm line 768. [Thu Oct 18 11:16:58 2018] Auth.pm: Subroutine checkauth redefined at C4/Auth.pm line 778. [Thu Oct 18 11:16:58 2018] Auth.pm: Subroutine check_api_auth redefined at C4/Auth.pm line 1387. [Thu Oct 18 11:16:58 2018] Auth.pm: Subroutine check_cookie_auth redefined at C4/Auth.pm line 1629. Status: 500 Content-type: text/html <h1>Software error:</h1> <pre>Bareword "C4::Context::temporary_directory" not allowed while "strict subs" in use at C4/Auth.pm line 1750. BEGIN not safe after errors--compilation aborted at C4/Auth.pm line 1928. </pre> <p> For help, please send mail to this site's webmaster, giving this error message and the time and date of the error. </p> [Thu Oct 18 11:16:58 2018] Auth.pm: Bareword "C4::Context::temporary_directory" not allowed while "strict subs" in use at C4/Auth.pm line 1750. [Thu Oct 18 11:16:58 2018] Auth.pm: BEGIN not safe after errors--compilation aborted at C4/Auth.pm line 1928. I'm guessing the redefined warnings come from some circular dependency... but that's a red herring since we'd never run C4::Auth on its own anyway. If I create a script and load "C4::Auth", the script compiles fine. In fact, if I remove the semi-colon from the end of "my $dir = C4::Context::temporary_directory", the Koha web app still loads just fine. But I get the following when I try to compile the module: perl -c "C4/Auth.pm" Subroutine psgi_env redefined at C4/Auth.pm line 50. Subroutine safe_exit redefined at C4/Auth.pm line 52. Subroutine get_template_and_user redefined at C4/Auth.pm line 142. Subroutine _version_check redefined at C4/Auth.pm line 715. Subroutine _session_log redefined at C4/Auth.pm line 761. Subroutine _timeout_syspref redefined at C4/Auth.pm line 768. Subroutine checkauth redefined at C4/Auth.pm line 778. Subroutine check_api_auth redefined at C4/Auth.pm line 1387. Subroutine check_cookie_auth redefined at C4/Auth.pm line 1629. syntax error at C4/Auth.pm line 1751, near "C4::Context::temporary_directory my " Global symbol "$instance" requires explicit package name at C4/Auth.pm line 1751. Global symbol "$dir" requires explicit package name at C4/Auth.pm line 1752. Global symbol "$instance" requires explicit package name at C4/Auth.pm line 1752. BEGIN not safe after errors--compilation aborted at C4/Auth.pm line 1928. Interesting... I always thought that modules were compiled when they were loaded using "use". Not even the BEGIN{} block in the module is run when loaded using "use", which seems contradictory to experience... |