Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't remove temp file on fail when creating office preview #33232

Merged
merged 1 commit into from Oct 19, 2018

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Oct 19, 2018

Description

Whenever an office file preview is created, in case of failure it would
try to delete the input path. In some cases the input path points to a
temporary file and in some other cases it's the original file. Deleting
the original file results in data loss in case the PDF processor had an
error.

The line that deletes the input file was removed now because there is no
need to remove temporary files because the TempManager will have all
temporary files cleant up in the shutdown handler at the end of the PHP
request.

Related Issue

Fixes #33160

Motivation and Context

See issue

How Has This Been Tested?

  • TEST: temporary file mode: verify that temp file is removed at the end
  • TEST: real file mode: verify that real file still exists
  • TEST: temporary file + PDF failure mode: verify that temp file is removed at the end and real file still exists
  • TEST: real file + PDF failure mode: verify that real file still exists

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added => not testable in unit tests
  • Acceptance tests added => not worth it especially with code removal
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

Whenever an office file preview is created, in case of failure it would
try to delete the input path. In some cases the input path points to a
temporary file and in some other cases it's the original file. Deleting
the original file results in data loss in case the PDF processor had an
error.

The line that deletes the input file was removed now because there is no
need to remove temporary files because the TempManager will have all
temporary files cleant up in the shutdown handler at the end of the PHP
request.
@PVince81
Copy link
Contributor Author

I've ticked the test boxes.
By default on openSUSE Tumbleweed, Imagick refuses to read PDFs as per policy, so it was easy to reproduce the original bug and verify. For temp files I had to use the debugger to find the path.

Then to make it work I had to fix the policy as per https://stackoverflow.com/a/52863413 so that Imagick allows reading from the PDF file, in which case the preview works and the cleanup works fine.

@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #33232 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #33232      +/-   ##
============================================
+ Coverage        64%      64%   +<.01%     
  Complexity    18190    18190              
============================================
  Files          1178     1178              
  Lines         68726    68725       -1     
  Branches       1271     1271              
============================================
  Hits          43990    43990              
+ Misses        24366    24365       -1     
  Partials        370      370
Flag Coverage Δ Complexity Δ
#javascript 52.88% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.3% <ø> (ø) 18190 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
lib/private/Preview/Office.php 0% <ø> (ø) 14 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b6c1e1...e3fe74b. Read the comment docs.

@PVince81
Copy link
Contributor Author

stable10: #33234

@davitol davitol mentioned this pull request Jan 24, 2019
39 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Office-files get deleted
3 participants