When copying an image with the same name from one text editor to another, duplicates of this image appear.
Steps:
- create two images on your computer. Place them in different directories, and give them the same name (e.g. image.png)
- create a new multiple choice question
- In the first HTML editor, use the Upload button to insert the first of your images
- In the second HTML editor, use the Upload button to insert the second of your images
- Select the image in the first editor and copy it (Ctrl + C)
- Paste it into the third HTML editor
- Select the image in the second editor and copy it (Ctrl + C)
- Paste it into the third HTML editor
- Fill in any other minimal fields to continue
- Press the "Save changes and continue editing" button
- Observe:
- The third editor has the first image in it twice
- The third editor does now have the second image at all
Root causes:
- file_copy_file_to_file_area used the new filename to fetch the old file, despite having the correct filename available
- file_merge_draft_areas uses the same filename when calling file_copy_file_to_file_area - e.g. image.png
- file_copy_file_to_file_area only checks if a file with the same filepath exists in the new target, not that the content is the same
- file_replace_file_area_in_text only supports changing the itemid, not any other useful part making it hard to implement a fix from the first two bugs
Additional bugs detected:
- file_replace_file_area_in_text does not handle all possibly file URIs properly - it works by fluke. Not all file URIs have the itemid in them - it is optinoal and sometimes not used
- extract_draft_file_urls_from_text can only ever work with a draftfile.php, and it allows for all URI parts to be specified, however:
- the only possible valid component for this is user - all others will lead to impossible matches
- the only possible valid filearea for this is draft - all others will lead to impossible matches
- for all of these methods (and others) the $forcehttps option is provided, but this is now deprecated as a use-case. Even before this point though, it could have led to some cases where a non-https value was used in the before/after, but not in the after/before
Suggested fixes:
- for stables this will likely be a bit grim and repetitive because there's just no point adding new args to fix the misunderstandings of the original methods
- Fix the bug in file_copy_file_to_file_area by modifying the $fileinfo['filename'] to be the old filename ($file['filename'])
- Fix the bug in file_merge_draft_areas by modifying the $fileinfo['filename'] to generate a unique name based on the old filename ($file['filename']) (e.g. $filename = uniqid("{$pathinfo['filename']}_") . ".{$pathinfo['extension']}";)
- Detect the bug in file_copy_file_to_file_area where the same file properties are used with a different file and throw an exception when found
- Detect incorrect uses of extract_draft_file_urls_from_text and throw \coding_exception for them
- Somehow work around the problem in file_replace_file_area_in_text to allow the new file name to be provided too
- write abundant unit tests for it all
- for the main branch (4.4):
- create a new \core_files\util class which is loaded by DI
- create new methods there:
public function replace_fileurl_in_text(
array $oldfile,
array $newfile,
string $text,
): string;
public function merge_draft_area_files(
int $draftitemid,
string $text,
): string;
public function copy_file_to_file_area(
\stored_file $oldfile,
?string $newcomponent = null,
?string $newfilearea = null,
?int $newitemid = null,
?int $newcontextid = null,
?string $newfilepath = null,
?string $newfilename = null,
): \stored_file;
- deprecate the existing methods
- modify the existing methods to use the new methods
- modify all existing places which called the old methods to make them use the new methods
- consider killing off extract_draft_file_urls_from_text and replacing it with private methods on \core_files\util as this is internal logic of the file API
- write abundant unit tests for it all