Make VideoHandler support optional #1439
Conversation
There was a problem hiding this comment.
Looks good to me with respect to the reported issue.
Maybe we should wrap the whole if-then-else-branch into the try-block? This would also allow to upload regular photos which are too broken to generate thumbs but maybe still viewable by a browser. But I don't know if that case really exist.
We should also have a test which tries to upload a video with FFMpeg disabled and which asserts that the upload still succeeds, that no thumbnail has been generated and a log entry has been made. But I can also write the test tomorrow independently of this PR.
This comment was marked as resolved.
This comment was marked as resolved.
|
@nagmat84: I made some changes along the lines you suggested. Logging works now. An existing testcase ( I also added a testcase for uploading a video file without ffmpeg (or exiftool for that matter, to ensure no video metadata). It works, though maybe it's not as tight as you would like (I don't believe it actually checks if there are no thumbnails?). |
|
|
||
| Configs::set(self::CONFIG_HAS_FFMPEG, $hasFFMpeg); | ||
| Configs::set(self::CONFIG_HAS_EXIF_TOOL, $hasExifTool); | ||
| } |
There was a problem hiding this comment.
Maybe we should additionally assert that the expected error message, e.g. "FFMpeg is disabled by configuration", can be found in the logs?
To this end we could clear the log table at the beginning of the test with DB::tables('logs)->delete(). At the end of the test we do a count-query and assert that we found an entry, e.g. something like
$numErrorMsg = DB::tables('logs)->where('message', 'like', '%FFMpeg is disabled by configuration%')->count();
self::assertEquals(1, $numErrorMsg);But maybe this would make the test too strict? I am unsure.
There was a problem hiding this comment.
I wouldn't do it myself (testing for log messages seems a little overly prescriptive to me) but feel free to add a commit to that effect to this PR if you want.
Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de>
* add JetBrain Open Source Community Support (#1442) * add JetBrain Open Source Community Support Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de> Co-authored-by: Martin Stone <1611702+d7415@users.noreply.github.com> * Make VideoHandler support optional (#1439) * Make VideoHandler support optional * Changes recommended by nagmat84 * Add a testcase for video upload without ffmpeg * Fix new testcase * Fix a typo * Update tests/Feature/PhotosAddHandlerTestAbstract.php Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de> * Check for error message that FFmpeg is disabled * Fixed Google Motion Photo test Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de> * Apply fixes from PHP-CS-Fixer Co-authored-by: Benoît Viguier <ildyria@users.noreply.github.com> Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de> Co-authored-by: Martin Stone <1611702+d7415@users.noreply.github.com> Co-authored-by: Kamil Iskra <kamil.01482@iskra.name> Co-authored-by: qwerty287 <80460567+qwerty287@users.noreply.github.com> Co-authored-by: github-actions[bot] <action@github.com>
* Support API tokens per user * Do not require Accept and Content-Type headers * Fix 500 on user creation * Recompile * Fix some reviews * Undo Content-Type and Accept change * Fix phpstan * Fix phpstan * Fix phpstan 3 * add support for disabling the token completely rather than letting it set as a random value * disable-enable FK * add tests * remove unused variable * Translate and add strings * Fix security issue during migration that gives admin perms to api_key * Fix locale * Sync frontend * Suggestions * Apply suggestions from code review Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de> * Apply suggestions * Recompile frontend * Fix perms * Fix lint * Improve check * Simplify middleware * Assert JSON * fix format * Apply fixes from PHP-CS-Fixer (#1444) * add JetBrain Open Source Community Support (#1442) * add JetBrain Open Source Community Support Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de> Co-authored-by: Martin Stone <1611702+d7415@users.noreply.github.com> * Make VideoHandler support optional (#1439) * Make VideoHandler support optional * Changes recommended by nagmat84 * Add a testcase for video upload without ffmpeg * Fix new testcase * Fix a typo * Update tests/Feature/PhotosAddHandlerTestAbstract.php Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de> * Check for error message that FFmpeg is disabled * Fixed Google Motion Photo test Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de> * Apply fixes from PHP-CS-Fixer Co-authored-by: Benoît Viguier <ildyria@users.noreply.github.com> Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de> Co-authored-by: Martin Stone <1611702+d7415@users.noreply.github.com> Co-authored-by: Kamil Iskra <kamil.01482@iskra.name> Co-authored-by: qwerty287 <80460567+qwerty287@users.noreply.github.com> Co-authored-by: github-actions[bot] <action@github.com> * Hash tokens * Sync frontend * Tmp commit to find out why it's failing * Second try to find failure * I'm pretty stupid at all * Fix db errors * Fix phpstan * Fix phpstan 2 * Fix phpstan 3 * Fix test * Add tests and fix issues as guest * sync frontend * fix phpdoc * Update locales and sync frontedn * Fix duplicated keys * Sync frontend * Add getAuthenticatedUser test * Don't send token with user, fix locales, synced frontend * Make PHPStan happy about something toally unrelated * Sync frontend * fix sync * Remove HasUserMiddleware * Fix tests * Added custom guard to handle session or token * Avoid unintended login as admin * Keep token-based authentication stateless + Fix tests * Added test for logout * Don't drop table * Recompile frontend * Revert "Recompile frontend" This reverts commit 039d72f. * Only migrate if not exists * Sync frontend to master * Undo change Co-authored-by: ildyria <beviguier@gmail.com> Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Benoît Viguier <ildyria@users.noreply.github.com> Co-authored-by: Martin Stone <1611702+d7415@users.noreply.github.com> Co-authored-by: Kamil Iskra <kamil.01482@iskra.name> Co-authored-by: github-actions[bot] <action@github.com>
Fixes #1438
This is a semi-draft PR:
VideoHandleris also used for Google Motion Pictures. I left the latter as-is as I don't understand how that part works and what (if anything) we can do about it.