Improve file handling #1309
Improve file handling #1309
Conversation
nagmat84
force-pushed
the
improve_file_handling
branch
from
f7c2c3e to
f87300e
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
| use App\Models\Album; | ||
| use App\Models\Configs; | ||
|
|
||
| class AddStrategyParameters | ||
| { | ||
| public ImportMode $importMode; | ||
|
|
||
| // Information about intended parent album | ||
| /** @var Album|null the intended parent album */ |
There was a problem hiding this comment.
Why the /** @var Album|null ? Isn't the type in php enough ?
There was a problem hiding this comment.
If the question is meant as "should it be theoretically sufficient to add the type hint to the variable in an optimal world?" Yes, I agree. 😀 Giving the type as a type hint in the code and as an type annotation in the comment introduces unnecessary redundancy.
Unfortunately, this is how PHPdoc expects it. I did not want to loose the original comment and attach it to the variable. So I changed to // into a /**. But in order to make the comment actually appear in the documentation, I had to add the @var keyword.
If one looks at https://docs.phpdoc.org/3.0/guide/references/phpdoc/tags/var.html#var how the syntax is defined, all three components of the @var tag - type, name, description - seem to be optional. But if you use the @var keyword and want to use the description you must add the type and the name, too. Otherwise, PHPdoc thinks that the first word of the description is the type and the second one the name.
Examples:
Does not work:
/** intended parent album */
public ?Album parentMakes PHpdoc falsely assume that "intended" is the actual type, "parent" the name and "album" the description. 🙄
/** @var intended parent album */
public ?Album parentActually works, but occupies too many lines for my personal taste. This tiny, little variable gets more space in the source code than it deserves 😀
/**
* Intended parent album
*
*@var
*/
public ?Album parentThis works, too, is a succinct one-liner but at the costs of redundancy
/** @var Album|null Intended parent album */
public ?Album parentHence, I decided to got with the last solution, especially as my IDE keeps the type hint and the type annotation in sync for me. 😋
Intro and Final Goal
This is the first of a series of four planned PRs which ultimately clean up the way how the MIME type of media files is handled.
That is this PR.
This PR is already in the pipeline, but not yet ready, see https://github.com/LycheeOrg/Lychee/tree/use_filestreams if curious
This does not change any functionality but it is/will be necessary.
This was discussed on Gitter recently.
All these PRs together will also eventually and "accidentally" enable the option to host media files on a remote location such as AWS S3. 🎉 🎊 I decided to split the PRs into several ones to make reviews faster/easier.
This PR
This first PR basically aims at two aspects
Aspect 1: Stop passing around file paths as strings
This is crucial in order to get rid of file paths eventually and make the transition to data streams possible. To this end, I introduced a couple of "file classes" (
FlysystemFile,NativeLocalFile, etc.) which provide a unified interface for writing to and reading from files. The interface is inspired by Flysystem. Currently, the file paths sill remain to be used under the hood, because our image handlers, ffmpeg extractor, etc., need them, but this will change with the 2nd PR. This first step makes the rest of Lychee using proper file objects as method parameters.Aspect 2: Concentrate checks whether a file type is supported in one place
Previously, there has been an unfortunate trait
Actions\Photo\Extensions\Constantswhich collects some of the supported MIME types and file extensions. Some classes used this trait, but in different and inconsistent ways. These constants have been moved to a new central pointImage\MediaFile.phpwhich is the new parent class for all file classes. The traitConstantshas been nuked.Moreover, a long standing asymmetry has been resolved. Previously, there were constant arrays for supported EXIF image types (but not for videos) and for supported MIME types for videos (but not for images). Now there are arrays for both. This also changes the way how a file is recognized as raw or as a photo. Previously, we replaced the MIME type of files which had been identified as raw by the special keyword
'raw'upon upload. Then a file was recognized as a photo, if it was neither a video nor raw. Now, as we have a list of MIME types for videos and for photos, videos and photos are identified as such, if there MIME type is included in the respective array. Everything else is raw. This means we don't use the special keyword'raw'anymore and don't loose the information about the real MIME type.Addon: Type-safe EXIF extractor
The previous
Extractorused a non-type-safe array to transport EXIF data. This has been changed, too. The EXIF data are typed properties, now. Accidentally, this fixed a bug which seemingly nobody noticed so far. Matching the video and photo part of an Apple Live photos cannot have worked previously. The extractor used the attributelivePhotoContentID, but we tried to savelive_photo_content_id'into the DB. This kind of bugs cannot happen anymore.