search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
145 stars 40 forks source link

Taxonomy terms "Description" field: "The "Insert image" dialog does not provide the option to upload an image #5007

Open klonos opened 3 years ago

klonos commented 3 years ago

Steps To Reproduce

To reproduce the behavior:

  1. On a vanilla installation of Backdrop, head to the "Tags" vocabulary, to add a taxonomy term -> /admin/structure/taxonomy/tags/add
  2. Click the "Insert image" button on the CKEditor toolbar for the "Description" field...

Actual behavior

The "Insert image" dialog only allows selecting an existing image (no option to upload new):

image

Expected behavior

The "Insert image" dialog should be allowing to upload a new image, same as when clicking the same button from a node body field, or an image field:

image

indigoxela commented 3 years ago

Confirmed, the filter setting to allow image uploads seems to get ignored.

Fun fact: if I add a "real" textarea field to terms (via Field UI), then this field does allow image uploads as expected, so I suspect, that this has something to do with the "pseudo field".

indigoxela commented 3 years ago

Sort of related or not: we have an open issue to convert this field to a regular field

As the regular field doesn't show that faulty behavior, switching to a real field would also fix the image upload problem. I'm just not sure how complicated this would be considering backwards compatibility.

indigoxela commented 3 years ago

It turned out to be a one-liner fix. A PR is available for testing and review.

indigoxela commented 3 years ago

Oops, there's a problem: With that change a file can get uploaded, but the file usage doesn't get set, so the file stays in temporary status - which means it will get deleted.

klonos commented 3 years ago

Oops, there's a problem: With that change a file can get uploaded, but the file usage doesn't get set, so the file stays in temporary status - which means it will get deleted.

When I filed this issue here, I also started creating a new one for this bug, with extended set of steps to reproduce (but then I closed my browser tab and lost everything 🤦🏼 ). I also believed that the image added to the description would get deleted if you delete a re-used image file - but it doesn't! 🤷🏼

klonos commented 3 years ago

...but then I closed my browser tab and lost everything 🤦🏼

Give me a few mins to get re-motivated and retype all that again...

klonos commented 3 years ago

...my point is that the thing about the image file being added to the description field and not getting a file usage IS pre-existing, and I think it should be a separate issue. This one-liner fix is just fine 👍🏼

indigoxela commented 3 years ago

...my point is that the thing about the image file being added to the description field and not getting a file usage IS pre-existing, and I think it should be a separate issue.

I see. It feels a bit weird to provide a PR, that makes things worse, but an extra issue for file usage handling in this pseudo field might make more sense.

klonos commented 3 years ago

@indigoxela here's the issue: #5016 ...while retyping the whole thing, I discovered that the same issue applies to images added in the WYSIWYG of custom blocks.

ghost commented 3 years ago

Do we need to allow for the (rare, possibly made-up) situation where a site might not want editors to upload images here, but would rather just let them select from existing ones? Then if we 'fix' this, suddenly editors can start uploading their own images... Or am I just being pedantic?

indigoxela commented 3 years ago

The thing is that uploading or not is still handled by the filter configuration, for instance on /admin/config/content/formats/filtered_html

Currently the setting there is just ignored for that pseudo field. That's a bug IMO. With the PR change this setting is respected.

Or am I just being pedantic?

I wouldn't say "pedantic", you're cautious. :smile:

quicksketch commented 3 years ago

I think this issue should either be combined or depend on https://github.com/backdrop/backdrop-issues/issues/5016. The lack of registering a file usage results in new files being deleted after 6 hours because they were never marked permanent (part of file_usage_add(). I don't think we can commit a change that will knowingly cause data loss.

klonos commented 3 years ago

During this week's meeting, @quicksketch mentioned that images added via this field will suffer from the same data loss issue that the "Block content" fields in custom blocks used to suffer. I would like us to test and confirm that.

indigoxela commented 3 years ago

During this week's meeting, @quicksketch mentioned that images added via this field will suffer from the same data loss issue that the "Block content" fields in custom blocks used to suffer.

That's correct, they do.

Our situation with fixing this might be slightly easier with taxonomy terms, as terms are entities with numeric IDs. (Blocks don't have, but the field in db table file_usage expects a numeric id.)

One thing to consider, though: putting effort into migrating this pseudo field to a real field could eventually be more efficient. Or maybe not...

klonos commented 3 years ago

...migrating this pseudo field to a real field could eventually be more efficient.

I'm leaning towards that too.

indigoxela commented 3 years ago

I decided to close the related PR, because it would introduce even more trouble. IMO it's essential to first fix the usage count problem.

As there's no PR anymore, I also remove the milestone. :wink: