Should images uploaded with IMCE be managed?

Usage should be registered by the applications that use IMCE as a file browser. For instance, file field integration of IMCE registers the usage before adding the file to the widget.

I would appreciate some clarification on this also. I have just tried a migration from D6 to D8 and although the files came across ok, they are also marked as being unused. I notice that the D8 file system settings allow "orphaned" files to be deleted - which suggests that IMCE files marked as not used are going to be deleted by the file system

I also think this question would benefit from some more discussion. Specially because I think integration with drupalimage like #2664068: CKEditor image and link dialog integration a.k.a. "Browse server" button is a great way forward and IMCE is a nice module to improve drupal file management UX.

Right now, drupalimage offers two alternative ways to inlilne an image: an upload widget and a src text field. The file registration behaviour (managed vs unmanaged) changes depending on what widget you use.

The src text field is just a hardcoded reference to a file location, while the upload widget creates a managed file via ajax and returns an file entity that drupalimage uses to populate data-entity-uuid. Later on entity insert/update de editor module parses this uuid attribute and registers the file usage.

IMCE is integrating with drupalimage through this second option, the src text field, but it kind of blurs the line between this two widgets. On one hand it allows for upload and management of files, but does not create entity files and just returns a url to populate the image src.

I would like IMCE to be able to enhance the upload widget and offer an option to have file entities created and returned, but that might be just out of scope.

Note: some of the behaviour described above, might not be accurate, I just had a quick look at the code this evening.

I've come up with this quick (and probably horrible) hack to have the images inserted with the data-entity-type and data-entity-uuid attributes populated so the editor module can register their usage.
EDIT: now using entityquery

modules/imce/src/ImceFM.php

  public function getFileProperties($uri) {
    $properties = array('date' => filemtime($uri), 'size' => filesize($uri));
//PATCHSTART
    if ($uuid = $this->getUuidFromUri($uri)) {
      $properties['type'] = 'file';
      $properties['uuid'] = $uuid;
    }
//PATCHEND
    if (preg_match('/\.(jpe?g|png|gif)$/i', $uri) && $info = getimagesize($uri)) {
      $properties['width'] = $info[0];
      $properties['height'] = $info[1];
    }
    return $properties;
  }

//PATCHSTART
  public function getUuidFromUri($uri) {
    $nids = \Drupal::entityQuery('file')->condition('uri', $uri)->execute();
    $file = \Drupal\file\Entity\File::load(reset($nids));
    return $file ? $file->uuid() : NULL;
  }
//PATCHEND

modules/imce/js/plugins/ckeditor/imce.ckeditor.js

    sendto: function (File, win) {
      var imce = win.imce;
      var editor = CKEDITOR.instances[imce.getQuery('ck_id')];
      if (editor) {
        var i;
        var text;
        var lines = [];
        var selection = imce.getSelection();
        var is_img = imce.getQuery('type') === 'image';
//PATCHSTART
        var entity_data = '';
//PATCHEND
        for (i in selection) {
          if (!imce.owns(selection, i)) {
            continue;
          }
          File = selection[i];
//PATCHSTART
          if (File.uuid) {
            entity_data = '" data-entity-type="' + File.type + '" data-entity-uuid="' + File.uuid;
          }
//PATCHEND
          // Image
          if (is_img && File.width) {
//PATCHSTART
//            lines.push('<img src="' + File.getUrl() + '" width="' + File.width + '" height="' + File.height + '" alt="' + File.formatName() + '" />');
            lines.push('<img src="' + File.getUrl() + entity_data + '" width="' + File.width + '" height="' + File.height + '" alt="' + File.formatName() + '" />');
//PATCHEND
          }
          // Link
          else {
            // Use the selected text/image for the first link
            text = !lines.length && CKEDITOR.imce.getSelectedHtml(editor) || File.formatName();
//PATCHSTART
//            lines.push('<a href="' + File.getUrl() + '">' + text + '</a>');
            lines.push('<a href="' + File.getUrl() + entity_data + '">' + text + '</a>');
//PATCHEND
          }
        }
        editor.insertHtml(lines.join('<br />'));
      }
      win.close();
    },

And since file usage is registered, uploaded files do not have to be permanent by default.

modules/imce/src/Plugin/ImcePlugin/Upload.php

      foreach (array_filter($files) as $file) {
        // Set status and save
//PATCHSTART
//        $file->setPermanent();
//PATCHEND
        $file->save();

I made a patch of this but for the dev branch. Should work on 8.x-1.2 too.

This patch just contains Karol Haltenberger's work, but reformatted as a patch. I also had to fiddle with the JS a bit.. was getting some errors about stray commas. Fixed that.

I changed this to 8.x-1.x-dev because I believe that is where patches should be committed, not on release branches.

The patch in #10 adds the data attributes for tracking the files' UUID for file management purposes. Keep in mind it is not retroactive; existing nodes with inserted inline images that don't have these data attributes will not automatically start being counted in the "Used in" column on /admin/content/files . The dirty solution is to re-edit the affected nodes manually.. only viable for small numbers of nodes. Separate issue from this though.

Actually this part should not be removed without checking that the editor profile has usage tracking enabled.

$file->setPermanent();

So, the above patch works to an extent, uploading a file using the IMCE image button will return the file's uuid and upon saving the content its usage will be registered.

But there are two issues still:

First, if the image is uploaded using the "Open File Browser" link in the popup window of the "normal" image button, the inserted image will NOT get the uuid, and I can't figure out why.

---

Second, if a new image is uploaded, the "normal" image button can not be used to modify it (caption, alignment, alt) immediately, submitting the popup form will return with an error message, caused by the Drupal file manager checking for file usage.
ManagedFile::validateManagedFile()
The description of this code is not clear to me, but it expects a registered usage for the file.

Usage could be registered by IMCE upon upload (and resize?), but we need to know that we are editing an entity and we should have its type and id too.

\Drupal\imce\Plugin\ImcePlugin\Upload::opUpload()

foreach (array_filter($files) as $file) {
        // Set status and save
        $file->setPermanent();
        $file->save();
        if ($entity) {
           \Drupal::service('file.usage')->add($file, 'editor', $entity->getEntityTypeId(), $entity->id());
        }
        // Add to the folder and to js response.
        $name = $fs->basename($file->getFileUri());
        $folder->addFile($name)->addToJs();
      }

see: \Drupal\file\FileUsage\DatabaseFileUsageBackend::add()

Although this means, that if the image is again removed without saving the entity first, the editor module will not know that it was ever registered and will not clean up this usage.

First, if the image is uploaded using the "Open File Browser" link in the popup window of the "normal" image button, the inserted image will NOT get the uuid, and I can't figure out why.

That is because the logic of the patch has been implemented on the CKEditor plugin javascript file, imce.ckeditor.js, and if we use the "normal" image button, we are not using that file at all. We should implement the uuid assigning somewhere within the file browser to make it work in all cases, not just when using the imce plugin directly.

Karol, you're right;

"First, if the image is uploaded using the "Open File Browser" link in the popup window of the "normal" image button, the inserted image will NOT get the uuid, and I can't figure out why."

I noticed this as well, when dealing with PDFs or other non-image files uploaded with Open File Browser, but the same would apply to images uploaded that way. I have not looked at this in some time and am not sure what to edit. Anyone have ideas?

"We should implement the uuid assigning somewhere within the file browser to make it work in all cases, not just when using the imce plugin directly."
Very true.

I have tested on my end and thought I was able to reproduce the issue, but now I can't. Will circle back to this.

@escaples explained the problem very well in #4:

Right now, drupalimage offers two alternative ways to inlilne an image: an upload widget and a src text field. The file registration behaviour (managed vs unmanaged) changes depending on what widget you use.

The src text field is just a hardcoded reference to a file location, while the upload widget creates a managed file via ajax and returns an file entity that drupalimage uses to populate data-entity-uuid. Later on entity insert/update de editor module parses this uuid attribute and registers the file usage.

IMCE is integrating with drupalimage through this second option, the src text field, but it kind of blurs the line between this two widgets. On one hand it allows for upload and management of files, but does not create entity files and just returns a url to populate the image src.

That's the reason why if IMCE integrates with drupalimage through the src text field form, it won't return more than a URL and data-entity-uuid will be empty. The solution depends on core drupalimage single URL form, it could be extended to register relative URLs as file entities. That's what I've proposed in this issue and patch: https://www.drupal.org/node/2828048. It's working well, although there is still a discussion about a more appropriate approach to acomplish that.

Karol, re "First, if the image is uploaded using the "Open File Browser" link in the popup window of the "normal" image button, the inserted image will NOT get the uuid, and I can't figure out why." This is actually working fine for me, using my patch. It has some differences from the code you posted. I am able to insert files and images using both the Open File Browser dialog and the IMCE image dialog, and they both get UUID attributes.

@juanolalla, thanks, that is most interesting. Am I to understand that IMCE would then need to leverage the core imagedialog functions? And what about non-image files uploaded via "Open File Browser"? Those also need UUID attributes. I am not familiar enough with the inner workings of EditorImageDialog.php to know if it deals only with images or also files generally, but its name suggests it's for images only.

And what about non-image files uploaded via "Open File Browser"?

I think we would need to implement or leverage another editor plugin to upload/select files and return a link to the file. A plugin which could integrate IMCE as file browser, and could register the files as entities.

This patch and the core patch, and turning image uploads off (using Open in Filebrowser) is a working combination for us.

Awesome discussion!
On the site I'm working on, every user uploaded file is placed in the private file system, so I need to have the file_usage table always up-to-date.

The patch above works fine for images and files, although I had to update the text filtering option to allow data-entity-type and data-entity-uuid on links. I had to update drupal to 8.2.7, to include the following fix on the editor module, that was wrongly getting the node ids when checking file references (see https://github.com/drupal/drupal/commit/bc0b00e254804e2a00ac5df1ee344fde...)

I rerolled the patch on #10 using a relative path to the module itself, so you can apply it using composer as well.

    "extra": {
        .....
        "patches": {
            "drupal/imce": {
                "Fixes IMCE upload issues without data-entity-uuid and data-entity-type": "https://www.drupal.org/files/issues/imce-managed-2762473-24.patch"
            }
        }
    }

Re-rolled. Not sure how to test, another developer used this patch and it didn't apply any more :/

Not sure why this was added and was skipped. Would need a re-roll if this is important ;)

-        $file->setPermanent();
         $file->save();

I tested the patches from #24 (did not apply cleanly) and #25 (applied cleanly but no UUIDs were being inserted in my HTML). I reviewed #25 and re-rolled it with a small syntax fix to the entity_data JS variable; this solved the missing UUIDs issue on my end. I am running Drupal 8.4.5 for what it's worth. See comment 27 below for the patch. Sorry, I can't seem to figure out how to attach a patch to the correct comment number. Wish we could just attach files to comments directly!

New patch coming.

I re-rolled my patch because I was having some trouble applying it. This one applied.

Thanks for the discussion, I found while testing D8.5.3 and also found that images uploaded via IMCE do not have the data-entity-type and data-entity-uuid set and without this there is no chance to edit the attributes of the image later using context menu (right click). IMCE also does not work together with the module lnline_responsive_images which is a suitable way to scale the images in the editor. So my impression is that D8 inline image handling is still not consistent. In D7 I liked using IMCE for beeing able to use an image at different places and organize the files in directories

Gerhard, have you tried any of the patches? We're using my patch on several production sites.
I don't use inline_responsive_images much, but I do believe there are patches to make IMCE work with that.

Casper, thanks. I now tried patch #30. But I am sorry to say: I cannot bring it to work. The changed imce.ckeditor.js is invoked (When entering a syntax error the complete ckeditor is gone) but the sendto: function seems not to be invoked or overwritten by sone other code. Whatever I enter in the
lines.push Statement the result is the same as before the code change. (Before each test I cleared the cache). So at the moment I wonder what is wrong.

Gerhard, I don't know what to tell you other than it's worked for me. I'm applying patch #30 with Composer and don't even need to clear caches (with Chrome) to see it start working.

Casper, thanks. I have no idea how to apply a patch with composer.
I edited the files manual and see the code in firefox debug

The composer.json file should have a section called "extra". In there you could place the patch, like this;

"extra": {
        "installer-paths": {
            ... some code here.... ignore this and see 'patches' below;
        },
        "patches": {
            "drupal/imce": {
                "Make IMCE work with managed files": "https://www.drupal.org/files/issues/2018-03-20/imce-managed-2762473-30.patch",
        	"IMCE image caption support - https://www.drupal.org/project/imce/issues/2850277#comment-12540407": "https://www.drupal.org/files/issues/2018-03-23/2850277-IMCE-caption-support-12.patch"
            }
        }
    }

In my example above I am also adding caption support to IMCE images.

After saving the composer.json file, run "composer install" from the command line and it should patch IMCE.

Casper, thanks you for showing how to implement a patch.
But meanwhile I see what is the problem. It is the original imce dialogue which is changed (also with me) not the D8 dialogue with imce upload integrated. Thats unchanged and imce uploaded files are not filled with uuid and cannot be changed afterwords
So I see several problems for our authors:
- You need to use 2 different buttons: First upload the files with the original imce upload to get them managed and be changed (second) with attribute like orientation or all plugin enhanced attributes like style or width by inline_responsive_images or others
- Files are managed now (registered use of the file within the node) but never deregistered when removing the file from the node. I have now 7 places where the image is registered in my node but at the end my node does not have images any longer and I cannot delete the image
- When -testwise - migrating our D7 site all files are not managed. So they cannot be changed by dialoges in the editor
- no idea whether your patch becomes officially integrated by the maintainers of imce.
So my impression that D8 inline image handling is a mess is affirmend and the whole idea of managing files and having files manged to change them later is put into question. I would like to have the D7 functionality again and have a rich dialogue for changing image attributes without bothering with file management.

'Files are managed now (registered use of the file within the node) but never deregistered when removing the file from the node.'
That would be a problem; I have not tested for that; my only interest in all this was just being able to insert images and edit their properties; I don't really care so much about file management specifically, and it sounds like neither do you.

Adding this issue to the list on https://www.drupal.org/project/drupal/issues/2821423#comment-12666525

Note about the last patch, you need to use a use statement for \Drupal\file\Entity\File:

$file = \Drupal\file\Entity\File::load(reset($nids));

// Change to:

use \Drupal\file\Entity\File;

$file = File::load(reset($nids));

I will create a followup patch for this. Ignore the patch #40 here (I can't seem to fully hide it). It doesn't work.

I tried making a patch that alters ImceFM.php to use a usestatement, but the patch failed for some reason;

"missing header for unified diff at line 5 of patch
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?"

I'm not sure what that means but maybe I shouldn't have generated my patch using "git diff > patch.txt" .. I don't know. Maybe someone else can try rolling that patch?

Hi @caspervoogt, note that the use statement goes at the top of the file below the namespace. See http://php.net/manual/en/language.namespaces.importing.php

Ignore this "mypatch.patch" please. Wish I could just delete it.

I tried rolling a new patch, because I was testing Inline Responsive Images in conjunction with this issue. I couldn't get that to work. I found that disabling the "Track images uploaded via a Text Editor" filter fixed it though. This was also the case for the built-in image icon, not just the IMCE one.

Patch #30 works for me--UUIDs are now assigned. Per the comments in #36, patch #12 is no longer needed.

Second @ufku this patch is still not secure.
More details on: #2850277: IMCE image caption support

wtf is going on?

#46 just patch x not y needed
#47 change status, randomly talk about something unrelated
#48 close without comment

Hello @Alan, the problems described look different, but if you look at the patch you will see that they are moving towards the same solution. So I found it better to centralize our actions in #2850277: IMCE image caption support, where @ufku already said what should be done to make the patch commit.

Comments on #47.

Too easy, it's a dup then :)

I did not dial as dup so people would not apply an unsecured patch.

If I insert a file uploaded via IMCE into a WYSIWYG field, it doesn't indicate that it is "Used". Is that related to this issue? Is this issue still being worked on, or should I open a new ticket?