Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-80408

Text Editor image duplication

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Major Major
    • None
    • 3.11.16, 4.1.6, 4.2.4, 4.3.3, 4.4
    • Files API
    • MOODLE_311_STABLE, MOODLE_401_STABLE, MOODLE_402_STABLE, MOODLE_403_STABLE, MOODLE_404_STABLE

      When copying an image with the same name from one text editor to another, duplicates of this image appear.

      Steps:

      1. create two images on your computer. Place them in different directories, and give them the same name (e.g. image.png)
      2. create a new multiple choice question
      3. In the first HTML editor, use the Upload button to insert the first of your images
      4. In the second HTML editor, use the Upload button to insert the second of your images
      5. Select the image in the first editor and copy it (Ctrl + C)
      6. Paste it into the third HTML editor
      7. Select the image in the second editor and copy it (Ctrl + C)
      8. Paste it into the third HTML editor
      9. Fill in any other minimal fields to continue
      10. Press the "Save changes and continue editing" button
      11. Observe:
        1. The third editor has the first image in it twice
        2. The third editor does now have the second image at all

      Root causes:

      1. file_copy_file_to_file_area used the new filename to fetch the old file, despite having the correct filename available
      2. file_merge_draft_areas uses the same filename when calling file_copy_file_to_file_area - e.g. image.png
      3. 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
      4. 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:

      1. 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
      2. 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:
        1. the only possible valid component for this is user - all others will lead to impossible matches
        2. the only possible valid filearea for this is draft - all others will lead to impossible matches
      3. 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:

      1. 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
        1. Fix the bug in file_copy_file_to_file_area by modifying the $fileinfo['filename'] to be the old filename ($file['filename'])
        2. 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']}";)
        3. 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
        4. Detect incorrect uses of extract_draft_file_urls_from_text and throw \coding_exception for them
        5. Somehow work around the problem in file_replace_file_area_in_text to allow the new file name to be provided too
        6. write abundant unit tests for it all
      2. for the main branch (4.4):
        1. create a new \core_files\util class which is loaded by DI
        2. 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;
          

        3. deprecate the existing methods
        4. modify the existing methods to use the new methods
        5. modify all existing places which called the old methods to make them use the new methods
        6. 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
        7. write abundant unit tests for it all

        1. 01.png
          01.png
          99 kB
        2. 02.png
          02.png
          57 kB
        3. 03.png
          03.png
          65 kB
        4. 04.png
          04.png
          60 kB
        5. 05.png
          05.png
          62 kB
        6. 06.png
          06.png
          62 kB
        7. 123.mp4
          9.75 MB

            Unassigned Unassigned
            romanenkovladimir Romanenko Vladimir
            Votes:
            0 Vote for this issue
            Watchers:
            12 Start watching this issue

              Created:
              Updated:

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 2 hours
                2h

                  Error rendering 'clockify-timesheets-time-tracking-reports:timer-sidebar'. Please contact your Jira administrators.