Bug 20428 - MARC import fails on Debian Stretch
Summary: MARC import fails on Debian Stretch
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 19676 19898 (view as bug list)
Depends on:
Blocks: 20727
  Show dependency treegraph
 
Reported: 2018-03-15 17:26 UTC by Kyle M Hall
Modified: 2018-10-24 09:13 UTC (History)
15 users (show)

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


Attachments
Bug 20428: Add the option to specify a tmp uploads dir (5.83 KB, patch)
2018-03-29 14:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files (809 bytes, patch)
2018-03-29 14:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Make about.pl inform about missing upload_tmp_path entry (2.50 KB, patch)
2018-03-29 14:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Add the option to specify a tmp uploads dir (6.21 KB, patch)
2018-03-29 14:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files (956 bytes, patch)
2018-03-29 14:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Make about.pl inform about missing upload_tmp_path entry (2.63 KB, patch)
2018-03-29 14:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Add the option to specify a tmp uploads dir (6.21 KB, patch)
2018-05-08 14:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files (877 bytes, patch)
2018-05-08 14:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Make about.pl inform about missing upload_tmp_path entry (2.36 KB, patch)
2018-05-08 14:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Add the option to specify a tmp uploads dir (6.28 KB, patch)
2018-05-08 14:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files (949 bytes, patch)
2018-05-08 14:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20428: Make about.pl inform about missing upload_tmp_path entry (2.43 KB, patch)
2018-05-08 14:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20428: Add the option to specify a tmp uploads dir (6.35 KB, patch)
2018-05-08 16:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files (979 bytes, patch)
2018-05-08 16:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20428: Make about.pl inform about missing upload_tmp_path entry (2.45 KB, patch)
2018-05-08 16:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20428: Added missing parameter --upload-tmp-path (1.75 KB, patch)
2018-05-08 16:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20428: Make upload_tmp a more general tmp directory (10.84 KB, patch)
2018-05-11 17:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Add the option to specify a tmp uploads dir (6.31 KB, patch)
2018-05-11 18:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files (986 bytes, patch)
2018-05-11 18:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Make about.pl inform about missing upload_tmp_path entry (2.47 KB, patch)
2018-05-11 18:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Added missing parameter --upload-tmp-path (1.77 KB, patch)
2018-05-11 18:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Make upload_tmp a more general tmp directory (10.84 KB, patch)
2018-05-11 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Make upload_tmp a more general tmp directory (10.83 KB, patch)
2018-05-11 19:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20428: Add the option to specify a tmp uploads dir (6.44 KB, patch)
2018-05-14 07:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files (1.05 KB, patch)
2018-05-14 07:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20428: Make about.pl inform about missing upload_tmp_path entry (2.55 KB, patch)
2018-05-14 07:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20428: Added missing parameter --upload-tmp-path (1.86 KB, patch)
2018-05-14 07:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20428: Make upload_tmp a more general tmp directory (10.91 KB, patch)
2018-05-14 07:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20428: (QA follow-up) Remove redundant directory level (2.12 KB, patch)
2018-05-14 07:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20428: Display warning if the entry is empty (2.12 KB, patch)
2018-05-14 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-03-15 17:26:42 UTC
It appears that Koha cannot access /tmp on Stretch. We should standardize on each Koha instance having it's own TMPDIR in /var/lib/koha/<instance>/tmp to avoid this as well as avoiding possible file name collisions between instances.
Comment 1 Jonathan Druart 2018-03-28 18:33:01 UTC
If it is confirmed, it's blocker for 18.05
Comment 2 Kyle M Hall 2018-03-29 11:19:11 UTC
(In reply to Jonathan Druart from comment #1)
> If it is confirmed, it's blocker for 18.05

To give a bit more detail, we had two Koha servers that were upgraded to Stretch. Both servers exhibited this behavior. The file upload would work just fine, the files would show up in /tmp as expected, but then Koha was unable to 'see' those files. In the code, if you printed a list of files in /tmp, Koha would see the tmp dir as empty! It's like to Koha, /tmp was a write-only filesystem!
Comment 3 paxed 2018-03-29 11:36:27 UTC
Would this be caused by systemd PrivateTmp?

https://www.maxoberberger.net/blog/2017/10/debian-9-private-tmp.html
Comment 4 Tomás Cohen Arazi 2018-03-29 12:54:58 UTC
(In reply to paxed from comment #3)
> Would this be caused by systemd PrivateTmp?
> 
> https://www.maxoberberger.net/blog/2017/10/debian-9-private-tmp.html

That's the most probable cause! But the solution should be what Kyle said: creating a per-instance temp dir and adjusting the cleanup script to deal with it too.
Comment 5 Tomás Cohen Arazi 2018-03-29 14:13:46 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2018-03-29 14:13:51 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2018-03-29 14:13:55 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2018-03-29 14:19:18 UTC
Created attachment 73429 [details] [review]
Bug 20428: Add the option to specify a tmp uploads dir

This patch adds an option to the koha-conf.xml file for specifying
a temporary uploaded files directory.

The koha-create script is adjusted to handle it and a convenient option
switch is added. If ommited, it will default to
/var/lib/koha/<instance>/uploads_tmp.

koha-create-dirs is patched to create the required directory with the
right permissions.

The docs get the new parameter documented.
Comment 9 Tomás Cohen Arazi 2018-03-29 14:19:23 UTC
Created attachment 73430 [details] [review]
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files

This patch makes Koha::UploadedFile->temporary_directory try to use the
new configuration entry. It will fallback to File::Spec->tmpdir
otherwise.
Comment 10 Tomás Cohen Arazi 2018-03-29 14:19:33 UTC
Created attachment 73431 [details] [review]
Bug 20428: Make about.pl inform about missing upload_tmp_path entry

This patch makes about.pl warn about a missing upload_tmp_path entry in
koha-conf.xml. It also mentions the effectively used tmp dir.
Comment 11 Tomás Cohen Arazi 2018-03-29 14:38:04 UTC
*** Bug 19898 has been marked as a duplicate of this bug. ***
Comment 12 M. Tompsett 2018-04-02 00:42:12 UTC
Comment on attachment 73430 [details] [review]
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files

Review of attachment 73430 [details] [review]:
-----------------------------------------------------------------

::: Koha/UploadedFile.pm
@@ +169,5 @@
>  sub temporary_directory {
>      my ( $class ) = @_;
> +    return (C4::Context->config('upload_tmp_path'))
> +                ? C4::Context->config('upload_tmp_path')
> +                : File::Spec->tmpdir;

Why a trinary when a || File::Spec->tmpdir would work more nicely?
Comment 13 Tomás Cohen Arazi 2018-05-08 14:31:56 UTC
Created attachment 75159 [details] [review]
Bug 20428: Add the option to specify a tmp uploads dir

This patch adds an option to the koha-conf.xml file for specifying
a temporary uploaded files directory.

The koha-create script is adjusted to handle it and a convenient option
switch is added. If ommited, it will default to
/var/lib/koha/<instance>/uploads_tmp.

koha-create-dirs is patched to create the required directory with the
right permissions.

The docs get the new parameter documented.
Comment 14 Tomás Cohen Arazi 2018-05-08 14:32:02 UTC
Created attachment 75160 [details] [review]
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files

This patch makes Koha::UploadedFile->temporary_directory try to use the
new configuration entry. It will fallback to File::Spec->tmpdir
otherwise.
Comment 15 Tomás Cohen Arazi 2018-05-08 14:32:09 UTC
Created attachment 75161 [details] [review]
Bug 20428: Make about.pl inform about missing upload_tmp_path entry

This patch makes about.pl warn about a missing upload_tmp_path entry in
koha-conf.xml. It also mentions the effectively used tmp dir.
Comment 16 Kyle M Hall 2018-05-08 14:53:04 UTC
Created attachment 75162 [details] [review]
Bug 20428: Add the option to specify a tmp uploads dir

This patch adds an option to the koha-conf.xml file for specifying
a temporary uploaded files directory.

The koha-create script is adjusted to handle it and a convenient option
switch is added. If ommited, it will default to
/var/lib/koha/<instance>/uploads_tmp.

koha-create-dirs is patched to create the required directory with the
right permissions.

The docs get the new parameter documented.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2018-05-08 14:53:12 UTC
Created attachment 75163 [details] [review]
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files

This patch makes Koha::UploadedFile->temporary_directory try to use the
new configuration entry. It will fallback to File::Spec->tmpdir
otherwise.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2018-05-08 14:53:16 UTC
Created attachment 75164 [details] [review]
Bug 20428: Make about.pl inform about missing upload_tmp_path entry

This patch makes about.pl warn about a missing upload_tmp_path entry in
koha-conf.xml. It also mentions the effectively used tmp dir.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Marcel de Rooy 2018-05-08 14:53:41 UTC
No access to /tmp? This will probably affect much more than upload?
What about CGI sessions? Several cron jobs ?
We should also look for hardcoded /tmp's in the codebase..
etc.

Or make sure that koha-instance has enough permissions at install/upgrade ?
Comment 20 M. Tompsett 2018-05-08 16:22:33 UTC
Created attachment 75169 [details] [review]
Bug 20428: Add the option to specify a tmp uploads dir

This patch adds an option to the koha-conf.xml file for specifying
a temporary uploaded files directory.

The koha-create script is adjusted to handle it and a convenient option
switch is added. If ommited, it will default to
/var/lib/koha/<instance>/uploads_tmp.

koha-create-dirs is patched to create the required directory with the
right permissions.

The docs get the new parameter documented.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 21 M. Tompsett 2018-05-08 16:22:36 UTC
Created attachment 75170 [details] [review]
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files

This patch makes Koha::UploadedFile->temporary_directory try to use the
new configuration entry. It will fallback to File::Spec->tmpdir
otherwise.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 22 M. Tompsett 2018-05-08 16:22:40 UTC
Created attachment 75171 [details] [review]
Bug 20428: Make about.pl inform about missing upload_tmp_path entry

This patch makes about.pl warn about a missing upload_tmp_path entry in
koha-conf.xml. It also mentions the effectively used tmp dir.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 23 M. Tompsett 2018-05-08 16:22:44 UTC
Created attachment 75172 [details] [review]
Bug 20428: Added missing parameter --upload-tmp-path

Line 463 lists valid parameters, but it was missing
upload-tmp-path: which resulted in the koha-create
saying it is an invalid parameter.

TEST PLAN
---------
1) apply all patches but this.
2) sudo perl ~/misc4dev/cp_debian_files.pl
3) restart_all
4) sudo koha-create --create-db --upload-tmp-path unique-path
awesome-test-name
   -- Fails to run.
5) apply this patch
6) repeat 2-4
7) sudo vi /etc/koha/sites/awesome-test-name/koha-conf.xml
   -- the upload_tmp_path entry should have unique-path
      in it.
Comment 24 M. Tompsett 2018-05-08 16:25:50 UTC
I'm going for lunch, but I'll have to test this on Stretch for a MARC import to be sure. Changing back to needs signoff.
Comment 25 Tomás Cohen Arazi 2018-05-08 18:01:15 UTC
This is PQA.
Comment 26 Tomás Cohen Arazi 2018-05-08 18:50:24 UTC
(In reply to Marcel de Rooy from comment #19)
> No access to /tmp? This will probably affect much more than upload?
> What about CGI sessions? Several cron jobs ?
> We should also look for hardcoded /tmp's in the codebase..
> etc.
> 
> Or make sure that koha-instance has enough permissions at install/upgrade ?

You are right about this being a broader problem for Stretch users. Kyle filed bug 20727 to have that conversation as this one only deals with file uploads.
Comment 27 Marcel de Rooy 2018-05-11 06:14:59 UTC
(In reply to Tomás Cohen Arazi from comment #26)
> (In reply to Marcel de Rooy from comment #19)
> > No access to /tmp? This will probably affect much more than upload?
> > What about CGI sessions? Several cron jobs ?
> > We should also look for hardcoded /tmp's in the codebase..
> > etc.
> > 
> > Or make sure that koha-instance has enough permissions at install/upgrade ?
> 
> You are right about this being a broader problem for Stretch users. Kyle
> filed bug 20727 to have that conversation as this one only deals with file
> uploads.

I think koha-conf should contain an entry for an alternative tmp directory.
Upload should then create a subfolder there.
But the config var should be more general and not be upload-tmp-path..
Comment 28 Tomás Cohen Arazi 2018-05-11 13:55:48 UTC
(In reply to Marcel de Rooy from comment #27)
> (In reply to Tomás Cohen Arazi from comment #26)
> > (In reply to Marcel de Rooy from comment #19)
> > > No access to /tmp? This will probably affect much more than upload?
> > > What about CGI sessions? Several cron jobs ?
> > > We should also look for hardcoded /tmp's in the codebase..
> > > etc.
> > > 
> > > Or make sure that koha-instance has enough permissions at install/upgrade ?
> > 
> > You are right about this being a broader problem for Stretch users. Kyle
> > filed bug 20727 to have that conversation as this one only deals with file
> > uploads.
> 
> I think koha-conf should contain an entry for an alternative tmp directory.
> Upload should then create a subfolder there.
> But the config var should be more general and not be upload-tmp-path..

I agree. Will try to provide a sensible follow-up.
Comment 29 Tomás Cohen Arazi 2018-05-11 17:26:54 UTC
Created attachment 75275 [details] [review]
Bug 20428: Make upload_tmp a more general tmp directory

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Tomás Cohen Arazi 2018-05-11 18:48:37 UTC
Created attachment 75279 [details] [review]
Bug 20428: Add the option to specify a tmp uploads dir

This patch adds an option to the koha-conf.xml file for specifying
a temporary uploaded files directory.

The koha-create script is adjusted to handle it and a convenient option
switch is added. If ommited, it will default to
/var/lib/koha/<instance>/uploads_tmp.

koha-create-dirs is patched to create the required directory with the
right permissions.

The docs get the new parameter documented.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 31 Tomás Cohen Arazi 2018-05-11 18:48:44 UTC
Created attachment 75280 [details] [review]
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files

This patch makes Koha::UploadedFile->temporary_directory try to use the
new configuration entry. It will fallback to File::Spec->tmpdir
otherwise.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 32 Tomás Cohen Arazi 2018-05-11 18:48:50 UTC
Created attachment 75281 [details] [review]
Bug 20428: Make about.pl inform about missing upload_tmp_path entry

This patch makes about.pl warn about a missing upload_tmp_path entry in
koha-conf.xml. It also mentions the effectively used tmp dir.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 33 Tomás Cohen Arazi 2018-05-11 18:48:59 UTC
Created attachment 75282 [details] [review]
Bug 20428: Added missing parameter --upload-tmp-path

Line 463 lists valid parameters, but it was missing
upload-tmp-path: which resulted in the koha-create
saying it is an invalid parameter.

TEST PLAN
---------
1) apply all patches but this.
2) sudo perl ~/misc4dev/cp_debian_files.pl
3) restart_all
4) sudo koha-create --create-db --upload-tmp-path unique-path
awesome-test-name
   -- Fails to run.
5) apply this patch
6) repeat 2-4
7) sudo vi /etc/koha/sites/awesome-test-name/koha-conf.xml
   -- the upload_tmp_path entry should have unique-path
      in it.
Comment 34 Tomás Cohen Arazi 2018-05-11 18:49:07 UTC
Created attachment 75283 [details] [review]
Bug 20428: Make upload_tmp a more general tmp directory

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Tomás Cohen Arazi 2018-05-11 19:47:10 UTC
Created attachment 75286 [details] [review]
Bug 20428: Make upload_tmp a more general tmp directory

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 36 Tomás Cohen Arazi 2018-05-11 19:47:52 UTC
Marcel: can you review the last changes?
Comment 37 Marcel de Rooy 2018-05-14 06:40:59 UTC
(In reply to Tomás Cohen Arazi from comment #36)
> Marcel: can you review the last changes?

QA: Looking here now
Comment 38 Marcel de Rooy 2018-05-14 07:19:03 UTC
dev-koha@dev:/usr/share/koha/devclone$ ll /var/lib/koha/dev/tmp/uploads/
drwxr-xr-x 2 dev-koha dev-koha 4096 May 14 08:56 koha_dev_upload

Removing this additional level. In an enhancement we can remove the database name from the main upload folder in tmp since it is instance bound now.
Comment 39 Marcel de Rooy 2018-05-14 07:21:02 UTC
Created attachment 75293 [details] [review]
Bug 20428: Add the option to specify a tmp uploads dir

This patch adds an option to the koha-conf.xml file for specifying
a temporary uploaded files directory.

The koha-create script is adjusted to handle it and a convenient option
switch is added. If ommited, it will default to
/var/lib/koha/<instance>/uploads_tmp.

koha-create-dirs is patched to create the required directory with the
right permissions.

The docs get the new parameter documented.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Marcel de Rooy 2018-05-14 07:21:07 UTC
Created attachment 75294 [details] [review]
Bug 20428: Make Koha::UploadedFile use the new config entry for tmp files

This patch makes Koha::UploadedFile->temporary_directory try to use the
new configuration entry. It will fallback to File::Spec->tmpdir
otherwise.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2018-05-14 07:21:12 UTC
Created attachment 75295 [details] [review]
Bug 20428: Make about.pl inform about missing upload_tmp_path entry

This patch makes about.pl warn about a missing upload_tmp_path entry in
koha-conf.xml. It also mentions the effectively used tmp dir.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2018-05-14 07:21:17 UTC
Created attachment 75296 [details] [review]
Bug 20428: Added missing parameter --upload-tmp-path

Line 463 lists valid parameters, but it was missing
upload-tmp-path: which resulted in the koha-create
saying it is an invalid parameter.

TEST PLAN
---------
1) apply all patches but this.
2) sudo perl ~/misc4dev/cp_debian_files.pl
3) restart_all
4) sudo koha-create --create-db --upload-tmp-path unique-path
awesome-test-name
   -- Fails to run.
5) apply this patch
6) repeat 2-4
7) sudo vi /etc/koha/sites/awesome-test-name/koha-conf.xml
   -- the upload_tmp_path entry should have unique-path
      in it.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2018-05-14 07:21:23 UTC
Created attachment 75297 [details] [review]
Bug 20428: Make upload_tmp a more general tmp directory

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2018-05-14 07:21:29 UTC
Created attachment 75298 [details] [review]
Bug 20428: (QA follow-up) Remove redundant directory level

Upload creates its own upload folder in the temp folder, so there is no
need to add another level in temporary_directory.
Removing the creation of this folder in koha-create-dirs too.

Also removing the use Koha::UploadedFiles in about.pl. No longer needed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2018-05-14 07:24:03 UTC
I would recommend release notes and a db rev warning to alert people to move existing installs in this direction.
Comment 46 Jonathan Druart 2018-05-14 13:08:11 UTC
(In reply to Marcel de Rooy from comment #45)
> I would recommend release notes and a db rev warning to alert people to move
> existing installs in this direction.

Maybe on the about page?
Comment 47 Tomás Cohen Arazi 2018-05-14 13:15:52 UTC
(In reply to Marcel de Rooy from comment #45)
> I would recommend release notes and a db rev warning to alert people to move
> existing installs in this direction.

I agree there should be an IMPORTANT: notice in the release notes for people in Stretch. And I think it is not the only one on this release.
Comment 48 Marcel de Rooy 2018-05-14 13:16:10 UTC
(In reply to Jonathan Druart from comment #46)
> (In reply to Marcel de Rooy from comment #45)
> > I would recommend release notes and a db rev warning to alert people to move
> > existing installs in this direction.
> 
> Maybe on the about page?

Yeah, there is an about warning already on the new tmp config var. A print warning from updatedatabase would be extra.
Comment 49 Jonathan Druart 2018-05-14 14:13:28 UTC
Created attachment 75305 [details] [review]
Bug 20428: Display warning if the entry is empty

And not only if the entry is missing. We should do the same for other
entries.
Also, use Koha::UploadedFile->temporary_directory to know the fallback
value.
Comment 50 Jonathan Druart 2018-05-14 14:14:30 UTC
(In reply to Marcel de Rooy from comment #48)
> (In reply to Jonathan Druart from comment #46)
> > (In reply to Marcel de Rooy from comment #45)
> > > I would recommend release notes and a db rev warning to alert people to move
> > > existing installs in this direction.
> > 
> > Maybe on the about page?
> 
> Yeah, there is an about warning already on the new tmp config var. A print
> warning from updatedatabase would be extra.

I do not think it is necessary.
Comment 51 Jonathan Druart 2018-05-14 14:20:23 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 52 Narcis Garcia 2018-05-24 18:42:25 UTC
PHP uses parameter "upload_tmp_dir", usually set at php.ini or webserver site profile. Doesn't Perl have some similar to not make necessary to check filesystem paths and permissions?

I have a Koha 17.11 running in Debian 9 (Stretch) without problem about uploads. How does Koha 17.11 deal with this?
Comment 53 Jonathan Druart 2018-05-24 18:55:15 UTC
(In reply to Narcis Garcia from comment #52)
> PHP uses parameter "upload_tmp_dir", usually set at php.ini or webserver
> site profile. Doesn't Perl have some similar to not make necessary to check
> filesystem paths and permissions?
> 
> I have a Koha 17.11 running in Debian 9 (Stretch) without problem about
> uploads. How does Koha 17.11 deal with this?

Did you see comment 4?
Comment 54 Narcis Garcia 2018-05-25 07:13:45 UTC
Is "template_cache_dir" parameter affected by this issue?
I have Koha 17.11 running on Debian 9 without this directive. Should I enable it when following performance recommendations?
Comment 55 Jonathan Druart 2018-05-25 13:11:30 UTC
(In reply to Narcis Garcia from comment #54)
> Is "template_cache_dir" parameter affected by this issue?

No

> I have Koha 17.11 running on Debian 9 without this directive. Should I
> enable it when following performance recommendations?

Yes
Comment 56 Pongtawat 2018-07-13 15:15:07 UTC
*** Bug 19676 has been marked as a duplicate of this bug. ***
Comment 57 Mason James 2018-07-20 04:08:14 UTC
(In reply to Narcis Garcia from comment #52)
> PHP uses parameter "upload_tmp_dir", usually set at php.ini or webserver
> site profile. Doesn't Perl have some similar to not make necessary to check
> filesystem paths and permissions?
> 
> I have a Koha 17.11 running in Debian 9 (Stretch) without problem about
> uploads. How does Koha 17.11 deal with this?

fyi: i've hit this problem on deb9 with 17.11.6 + plack
Comment 58 Mason James 2018-08-07 20:28:26 UTC
(In reply to Jonathan Druart from comment #51)
> Pushed to master for 18.05, thanks to everybody involved!

hi Frido, would you accept this patchset for the 17.11 release?