Task #67126

Bug #59473: Inconsistent API for folders

Bug #65305: Folder::getSubfolder() doesn't use the file drivers correctly

Make sure ProcessedFile and LocalImageProcessor use correct FAL API

Added by Frans Saris about 4 years ago. Updated 11 months ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
-
Target version:
-
Start date:
2015-05-25
Due date:
% Done:

100%

TYPO3 Version:
7
PHP Version:
Tags:
Complexity:
medium
Sprint Focus:

Description

ProcessedFile::setName() and LocalImageProcessor::checkForExistingTargetFile() only work for local/'normal' file systems where the file identifier is the full filepath.

As not all file systems use this to build the identifiers of files and folders these functions should use the proper FAL methods to make sure it works in all cases.


Related issues

Related to TYPO3 Core - Bug #67095: unnecessary generation of images Closed 2015-05-21
Related to TYPO3 Core - Bug #75756: Fatal error occurs when sys_file_processedfile table is cleared but processed file is still present Closed 2016-04-18

Associated revisions

Revision efa96ba5 (diff)
Added by Andreas Wolf over 3 years ago

[BUGFIX] LocalImageProcessor cannot reuse remote files

The LocalImageProcessor had a check for the storage type, to only do a
check for existing files on local storages, as it needed to fetch an
existing file from the storage to get metadata from it.

This check is now replaced by a proper usage of the FAL API: Fetch the
generated file for local (read-only) processing, which in case of a
local storage means the file is not moved at all. Then, the image
metadata can be extracted from the local file and the file index record
of the processed file can be updated accordingly.

Change-Id: I9fca326fe1d1743cd53a0a85c674ff81e5a051b9
Releases: master, 7.6
Resolves: #67126
Reviewed-on: https://review.typo3.org/46905
Reviewed-by: Jan Helke <>
Tested-by: Jan Helke <>
Reviewed-by: Benni Mack <>
Tested-by: Benni Mack <>

Revision 9d71c195 (diff)
Added by Andreas Wolf over 3 years ago

[BUGFIX] LocalImageProcessor cannot reuse remote files

The LocalImageProcessor had a check for the storage type, to only do a
check for existing files on local storages, as it needed to fetch an
existing file from the storage to get metadata from it.

This check is now replaced by a proper usage of the FAL API: Fetch the
generated file for local (read-only) processing, which in case of a
local storage means the file is not moved at all. Then, the image
metadata can be extracted from the local file and the file index record
of the processed file can be updated accordingly.

Change-Id: I9fca326fe1d1743cd53a0a85c674ff81e5a051b9
Releases: master, 7.6
Resolves: #67126
Reviewed-on: https://review.typo3.org/47127
Reviewed-by: Benni Mack <>
Tested-by: Benni Mack <>

History

#1 Updated by Markus Klein about 4 years ago

Maybe more places have some special treatment for "Local" driver type. Don't forget to grep them and check all.

#2 Updated by Michael Oehlhof about 4 years ago

I have had a look in the sources and here are my results:

In LocalImageProcessor.php we need the absolute path to the target file. But there is no public function in the driver interface yet. Because we need it only for the ‚Local’ DriverType we can change in the LocalDriver.php the function getAbsolutePath from protected to public and use this function.
To use the function getAbsolutePath we also must get the current driver in the function checkForExistingTargetFile, for this we need to change the function getDriver of ResourceStorage.php from protected to public.

In the function setName of ProcessedFile.php the property ‚identifier‘ is set directly. That did not work with drivers that are creating the identifiers themselves in a special way. We have to use the function addFile of the storage to add the file and then use the identifier of the added file.

Any comments welcome.

#3 Updated by Frans Saris about 4 years ago

The LocalImageProcessor can also be used by non Local storages. So only adding this to the local driver isn't enough.

#4 Updated by Michael Oehlhof about 4 years ago

In LocalImageProcessor the "new" function is only called for the local storage, so adding this to the local driver is enough.
Line 106: if (.... && $storage->getDriverType() === 'Local')

#5 Updated by Frans Saris about 4 years ago

Had a look at the code and now I get your intention.

But IMO we should find a solution that fits all type of drivers. The $storage->getDriverType() === 'Local' check is added because this works only for hierarchical path based storages and there is currently no way to check for this other then checking for type == local

#6 Updated by Michael Oehlhof about 4 years ago

For the processed files we need direct access to the file which is than used by getImageDimensions.
Do we need a solution for drivers which filesystems are not locally accessible?

#7 Updated by Philipp Wrann almost 4 years ago

We are currently working on a FAL Driver implementation for our own media database and ran into that problem, processedFile generation (backend preview for example) is not possible because of conditions like:

if ($processingFolder->hasFile($task->getTargetFileName()) && $storage->getDriverType() === 'Local')

How are the chances for this to be fixed in 7 LTS?

#8 Updated by Daniel Hölbling-Inzko almost 4 years ago

I just ran into this issue with a custom FAL Driver I was writing. Unfortunately this severely limits the usability of the FAL in our case as we where going to connect Typo3 to an external Image-Repository through it. And having reprocessed files on every request slows the system down to a crawl.

Anything I can do to help to fix this issue?
Looking through the code it seems the FAL Driver and Storage is missing some methods to retrieve local filenames for processing am I right? Because most code from `checkForExistingTargetFile` should imo move into storage/driver.

greetings Daniel

#9 Updated by Martin Kleofasz over 3 years ago

@Daniel Hölbling-Inzko

Have you work out a solution for this problem?
We encountered the same problem with our custom FAL driver and also with the EXT:fal_ftp from the extension repository, if the storage is flagged readonly.

If we can help to fix this issue. Contact me please.

#10 Updated by Gerrit Code Review over 3 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/46905

#11 Updated by Gerrit Code Review over 3 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/46905

#12 Updated by Gerrit Code Review over 3 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/46905

#13 Updated by Gerrit Code Review over 3 years ago

Patch set 1 for branch TYPO3_7-6 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/47127

#14 Updated by Anonymous over 3 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#15 Updated by Benni Mack 11 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF