TYPO3 Core

I think this whole method is a bit problematic:

1. The order of operations is wrong (I’m currently on correcting this, see below)
2. It emits a signal that is only used when the file is actually about to be added (PreFileAddSignal).
3. The folder check is unnecessary IMHO as the $targetFolder cannot be passed as NULL
4. Considering the original reason this method was added, I don’t see why a simple call to hasFileInFolder() did not suffice.

As for the signal, an API user might rely on the file only being emitted in addFile()—that implies that the file will actually be added moments later. If the file is not added at all in the current (AJAX) request, then things might break. There is documentation on that, but I’d rather have a dedicated signal than abusing a totally unrelated signal.

Concerning the order of operations: The first thing to do is let others alter the filename (via the signal), then the driver should tell if the filename is allowed (or correct it – sanitizeFileName()) and only then we must check the permissions.

Andreas Wolf wrote:

4. Considering the original reason this method was added, I don’t see why a simple call to hasFileInFolder() did not suffice.

The whole issue is that hasFileInFolder() doesn't sanitize the input and also the signal can change the file name. So to check if a file already exists, f.i. when uploading a new file, you first need to sanitize the filename just like the normal addFile() procedure would do. And this involves the driver->sanitize() and the PreAddFile signal.

As for the signal, an API user might rely on the file only being emitted in addFile()—that implies that the file will actually be added moments later. If the file is not added at all in the current (AJAX) request, then things might break. There is documentation on that, but I’d rather have a dedicated signal than abusing a totally unrelated signal.

Maybe a new signal would indeed make sense, but current signal is currently documented in the code with:

// We do not care whether the file exists yet because $targetFileName may be changed by an
// external slot and only then we should check how to proceed according to $conflictMode

That's why we went for adjusting the current signal.

So your proposal is to go for a SanitizeFileName Signal?

I'm not convinced that order is wrong. IMO sanitizeFileName() should be called before the signal. Else the Signal gets the wrong filename.

Of Course the Signal could return an file that is not save. But that's the responsibility of the developer. When the filename is really passed to the driver to be created/added the driver sanitizes the filename one last time before creating the file and returns the filename.

Also the signal has a reference to the $driver and is able to call $driver->sanitizeFileName().

So the order of operations is correct.

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

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

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

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

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

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

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