Images uploaded to a comment in CKEditor are not marked as permanent

[laryn]

Describe your issue or idea

I recently ran into an issue with the Splashify module where images uploaded into CKEditor were not marked as permanent and were deleted. I modified some code that I found in the core block module for use with Splashify, to mark those files as permanent and it seems to be working. I have noticed that the forum has the same issue where uploaded images are not being saved permanently on replies/comments on a topic. @oadaeh has verified that this is a core comment issue, not a forum-specific issue.

Steps to reproduce (if reporting a bug)

• Enable comments and allow image uploading in comments.
• Make a comment and insert an image into it.
• Save.

Actual behavior (if reporting a bug)

• Image uploads fine but is not marked as permanent so disappears after a time.

Expected behavior (if reporting a bug)

• Image is marked as permanent.

References:

• Images uploaded to CKEditor are disappearing backdrop-ops/forum.backdropcms.org#56
• Files uploaded in CKEditor are not marked as permanent backdrop-contrib/splashify#7

[oadaeh]

I tested this by adding an image to the content of a CKEditor field by clicking on the image button.
These places saved the image data in the file_managed table with a status of 0 (Temporary):

• Comments on both a Post and a Forum
• Splashify module content (contrib)

These places saved the image data in the file_managed table with a status of 1 (Permanent):

• Custom block content
• Page and Post content

[oadaeh]

As an FYI, I removed the fix @laryn made to the Splashify module before I did my testing. I wanted to be sure I was seeing the behavior, and I wanted to see what was common and different.

[herbdool]

I wonder if it can be set in preSave or postSave methods. Unlike the block module, comment is extending the Entity class and using its save method. https://github.com/backdrop/backdrop/blob/1.x/core/modules/comment/comment.entity.inc

[oadaeh]

The Comment module could also override the save() method, and it could be done there.

[herbdool]

That's an option, though I looked at https://github.com/backdrop/backdrop/blob/1.x/core/modules/user/user.entity.inc and it's in the presave method there. I was searching for that function file_usage_add and didn't see it mentioned for nodes so I wonder if it's done differently there.

[laryn]

In the thread on the forum channel, @jenlampton says "We should implement the same fix in comment_save() as we did in node_save() in #1373 "

[oadaeh]

node_save() just calls Node::save(), and like Herb, I didn't see anything in there for saving files.

[herbdool]

It might be because nodes call hook_entity_insert and comments might not?

[jenlampton]

Maybe check filter's implementation of hook entity save?

[herbdool]

From what I can tell, it should work with comments. Comment extends Entity which invokes hook_entity_insert and that is called in filter_entity_insert. And that should make the files permanent. So might be something wrong with _filter_get_file_ids_by_field?

[herbdool]

I narrowed it down to this: comment class needs to set a bundle like: "comment_node_post". Currently it's defaulting to the entity type which is "comment".

[oadaeh]

(While I was writing this up, @herbdool posted a more succinct explanation, but it's not entirely complete, so I'm going to post this anyway. :) )

Well, that was not nearly as quick as it seemed that it would be at the start, but I've figured it out.

When content is saved and the Filter module is allowed to have its way with it, whether through filter_entity_insert() or filter_entity_update(), at some point it calls _filter_get_file_ids_by_field(), which calls _filter_get_processed_text_fields().

In _filter_get_processed_text_fields() there is this line:
$fields = field_info_instances($entity->entityType(), $entity->bundle());

For the Node entity, bundle() looks like this:

  public function bundle() {
    return $this->type;
  }

The Comment entity does not have a bundle function, so the default Entity bundle() method gets called, which looks like this:

  public function bundle() {
    return $this->entityType();
  }

So, when _filter_get_processed_text_fields() is called for Comments, it ends up looking like this:
$fields = field_info_instances('comment', 'comment');

The problem is that there are no 'comment' bundles for Comment entities, and in fact, the Comment entity doesn't really define a bundle or type for itself, and so nothing is returned for $fields. What Comment does define is 'node_type' that it uses as a bundle or type, but it doesn't let the world know about that.

To fix the problem for Comment, all that needs to be done is to add a bundle() method that looks something like this:

  public function bundle() {
    return $this->node_type;
  }

(I don't know what other implications, if any, that would have on the rest of Backdrop.)

However, that won't fix the problem for Splashify or other modules like it that use the WYSIWYG field for things that are not defined as entities. For those, something different needs to be done, but I don't know what. It might be as simple as documenting the fact that they need to save their own files, but that seems odd to have to help out another function that already does it, just because it doesn't handle every situation.

[oadaeh]

I should also add that for modules like Splashify, they would need to manually handle all the file management, including updating usages when editing the content and removing files when they are no longer used.

[herbdool]

Ha! @oadaeh we were on the same track. You solved it where I got stuck. I was trying to figure out how to create $this->type for comment but you're right that $this->node_type is set. I added this to comment.entity.inc:

We should declare node_type in the class to be proper I think:

  /**
   * The comment type (bundle).
   *
   * @var string
   */
  public $node_type;

And

  /**
   * Implements EntityInterface::bundle().
   */
  public function bundle() {
    return $this->node_type;
  }

I tested with a comment and it does mark it properly in file_managed and file_usage.

[herbdool]

@oadaeh for everything other than comments there's already a long thread about that here #1373 so we can probably just focus this issue on setting the bundle. And running tests that there are no side effects of this.

[oadaeh]

#1373 is closed, so nothing will probably happen there unless it is re-opened. Maybe a new issue should be opened.

[herbdool]

@oadaeh perhaps. It would be for any module that overrides the default by setting '#editor_uploads' => TRUE, on an element. There's probably no easy way to come up with something generic so perhaps at least we should have this better documented on the API site.

[herbdool]

@laryn @oadaeh are you able to test my PR?

[laryn]

@herbdool Sure, will try to test on Monday! Thanks.

[quicksketch]

Wow nice work @oadaeh! That is a deeply nested problem! The fix and implementation by @herbdool looks great to me. I'm marking this RTBC from the feedback here and my review. @laryn please move back to needs work if you encounter any issues, otherwise I'll merge this next time I'm going through the RTBC queue.

[laryn]

Looks good to me! Thanks @herbdool and @oadaeh.

[quicksketch]

I've merged backdrop/backdrop#2101 into 1.x and 1.9.x, thanks everyone!