Add image uploading to WYSIWYGs through editor.module

I'm working on a Drag'n'Drop (instead of a native or CKEditor dialog) file upload that works with the latest version of Firefox/Chrome/IE. Should it be part of this issue or I publish first a contrib module? In the latter case I'll focus on a D7 version.

@quicksketch, does #1 allow us to make the image form fieldable ala file_entity/media?
Eg taxonomy terms or other fields on the images etc - so the views etc (eg recent uploads etc) could be as powerful and flexible as possible
It's a managed file so I guess we could form_alter in the field_attach_form/submit/validate logic at a later date?

Thanks, I didn't know that module.

@quicksketch - let me know where I can help with this, this will be awesome

#1: general +1 :)

Some remarks/questions:

• You say "file handling for text editors", but isn't "image handling for text editors" more accurate? Or do you want to support non-image files as well? I can see how you would want to support Confluence-like drag-and-drop of e.g. Powerpoint/Keynote files into a WYSIWYG editor and then have it render a preview of that, but that's beyond the MVP, I think. Hence I'm inclined to focus on "image handling for text editors", while still keeping the door open for contrib to provide the Powerpoint/Keynote use case. In Drupal core in general, I think the common practice would be for files other than images to be uploaded through File fields (cfr. Upload.module attachments).
• We'll have to deal with image styles (i.e. after uploading choose an image style, plus the option to have the style link to the original probably), but that should be a follow-up issue.
• How would this #type = filtered_html element work? Which module would call it? This is the most vague part to me, though I think I kind of know what you mean, it's just technically not very clear. You talk as if this exists already, but unless I'm missing something, it does not yet exist. Most likely, you're referring to "any place where text filtering is taking place", but I'm not sure this architecture change is acceptable for everybody. Nor do I even know what the consequences of such an architecture change would be.
• If uploads are configurable in the editor settings, then why is $element['#editor_uploads'] = TRUE necessary at all?
• On file usage entry (in|de)crementing: how to deal with revisions? I.e. revision 1 of a node uses a file, revision 2 doesn't, revision 3 does. Does this count as 1 or 2 entries? I'd say two. Related:
  

  If files were *removed*, it would also decrement the file usage entries by one and then call file_delete() to clean up the file if no uses were remaining.

  The only way to not break previous revisions is by counting file usage counters on the revision level, not the entity level.

• The editor_save_files stuff… that'll be "kind of like a filter but not really". I like the overall concept, but I think this is where it won't fit into the existing architecture. I very much hope I'm wrong :)

Alright, sounds like we're in complete agreement :) I'm glad that your experience with File Field Sources has given you a more holistic background — thus you're in a much better position to judge all this than I am! :)

RE: image styles fiasco — that is actually related to #1863530: Don't generate CDN URLs when using the Insert module by blacklisting file/ajax/* (an issue with the CDN module)… :) It'd be cool to take all of that into account.

Maybe I don't understand how the data-fid will work in general, but I have some doubts regarding the data-integrity...

1) What will happen if I copy & paste content from another Drupal site, with an image that has data-fid set?

Example:

Site "A" has an image "A" with data-fid = "1". I copy some content from Site "B", along with an image "B", with (the same) data-fid = "1" (I am even not aware of this as I just selected something and pasted it).

2) Another (minor) problem to keep in mind - an image is uploaded, data-fid is set automatically, then user changes an URL in the image dialog to a different image.

(I guess this case can be solved quite easily by removing fid in case of an external image, or by updating fid to the correct value if the URL points to a file that has a different fid, to make sure that image will not be lost accidentally some day).

Clarifying issue status; this is postponed on #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms landing.

Site "A" has an image "A" with data-fid = "1". I copy some content from Site "B", along with an image "B", with (the same) data-fid = "1" (I am even not aware of this as I just selected something and pasted it).

That's what UUIDs are for. You're absolutely right that content should not contain references to serial IDs.

Luckily, all entities have UUIDs in D8 already. We can simply use them.

Do we aim to get this done before code freeze? Can this be done after code freeze?

Just wondering what is happening with this - is it too late to get this in? I am not au fait with what is and isnt allowed under API freeze, but this seems like the kind of 'on-top' functionality that doesnt require API change.

New Drupal adopters - who may see image uploading as an integral part of the WYSIWYG experience - might be a little deflated to find it not there. (It won't affect me in the slightest, I just took my first test drive of the latest D8 last week, with a view to putting a site live in the next month, and so far I love and love and love what I am seeing)

Just a suggestion: I note there is an 8.x version of https://drupal.org/project/elfinder - earlier versions were integrated with the CKEditor module, but the latest 8.x probably isn't integrated with the Drupal native dialog yet.

However, it might serve as an imperfect-but-there solution and then work can be done on a more fully integrated experience in D9. Bringing it in as a standalone module wouldn't hold up any other functionality.

#19: I also think this can still go in, it won't require an API change, only an API addition (insofar this will be considered an API).

I agree this is super super important. Unfortunately I won't have time for it. quicksketch promised to work on it at DrupalCon Portland, almost two months ago, but he got sucked into client work so hasn't had the time either. Maybe he has time now? :) Or maybe you have time?

So here is a patch that adds:
- a managed_file field to the image dialog
- upload directory, max filesize, min and max dimensions to the filter config page: these are used to construct validators for the managed_file field

Im not too sure what to do about file saving, ill need a little help here. The patch contains some simple file saving in the submit function of the image class. However, for some reason file_load isn't working on the uploaded file (its also not previewing properly once uploaded, maybe that's a hint)

Hmm. Basically the tests are failing because the CKEditor test doesn't pick up the extra $editor->settings variables. Should I store them somewhere else - $editor->upload perhaps?

New patch: this one actually uploads an image, although the saving probably needs to be abstracted somewhat, the code is still in the form submit function.

The other thing is that uuid's are not created by the managed_file field upon initial upload, is that a bug we need to make another issue for, or is it something that needs to be handled as part of the form submit process?

I also moved the upload settings to $editor->upload - it seems to make sense to have $editor->settings deal just with the particular text editor.

Im not sure how much farther I can take this, it would be good to get another set of eyeballs on it.

Accidentally deleted tags - readding now!!

So this patch loses the minimum dimensions and adds file scheme.

To summarise whats left (as I understand it):
- The uploaded file needs to get itself a uuid as part of the managed_file element upload, and I think that's going to need its own ticket (Let me know, and I'll create it!) We can then add the uuid as a 'data-id' attribute to the HTML.
- Once that is done, move the file saving to an editor_file_save function or $editor object that is called by hook_entity_presave. This scans all attached textarea fields for this 'data-id' attribute, makes the files with those uuids permanent if they aren't already, and increments the file usage tally for those files.
- We need to implement hook_entity_delete to scan textareas in all revisions of that entity (is that going to be a performance drag?), and hook_entity_revision_delete to scan textareas in that particular revision, and decrements the file usage accordingly.
- Add #editor_uploads property to the text_format element in editor_element_info. What does our callback function do if editor_uploads is FALSE? Replace the managed_file field with the original URL field? (sorry, this is the bit I'm least clear about)

UUID issue: #2046809: UUIDs not added when file uploaded using managed_file FAPI element. I guess we can press forward on the rest in the meantime.

Hi, took a look at patch - definitely looks much better :)
- The URL field is shown by default. Once I save the filter format settings page, then the managed_file field appears.
- I had a look at trying to move the file usage code into the entity hooks. What is the easiest way to get the values of all text editor fields for a particular revision ID? Im having a little trouble navigating the docs.

My hands are going to be full over next few days, I can try and come up with something next week, unless you get there first :D

Oh. This is the issue I meant to make major, I think!

Nice progress here! What's left to get this to completion? :)

Functionally, the only thing left to do is to move the file usage incrementing/decrementing somewhere that is aware of revision context, so we can act on entity delete, revision delete, and revision update. Im having hardware issues at moment so i cant be of much help this week, unfortunately.

I'm on it now.

Fixed bugs

1. The logic for UUID parsing was completely broken.
2. Uploading an image for a new entity (i.e. an entity that doesn't yet have an ID/UUID) failed, precisely because $entity->id() returned NULL.
3. Mark the editor module, not the file module as the owner/user in the file_usage table.
4. Make it work with entity deletion, revision deletion, etc. Including proper handling for modifying revisions (i.e. don't create a new revision, but still add and remove file references).
5. The $editor->settings['image_upload'] stuff would always be set, even if the DrupalImage button was not being used, plus #editor_uploads was not text format-specific, so this effectively meant you still had no per-text format control over this. I've added a image_upload.status config key, with a corresponding "Enable image uploads" checkbox. Once you check this, image uploads are enabled for this text format, and at the same time this means we can now hide all the image upload settings unless this is checked (using #states). This allowed me to remove the #editor_uploads plus data-editor-uploads plus uploadsEnabled hacks/passing through all over the place.
   The original reason for having #editor_uploads was this:

   Because uploads through the WYSIWYG are only going to be stored as temporary files. The module will need to implement a saving mechanism to mark the files as permanent.

   But now we implement this saving mechanism through entity hooks, so there is (fortunately!) no need anymore for a custom callback that every module has to call.

6. Core's editor.editor.(full_html|basic_html).yml files weren't updated yet to have image uploading enabled by default.
7. Test coverage!

Fixed design flaws

1. data-file-uuid implies it is used/owned by file.module, so I renamed it to data-editor-file-uuid.
2. The schema.yml files hadn't been updated yet (see https://drupal.org/node/1905070). Worse, even, the approach taken in previous patches in this issue was fundamentally incompatible with schema.yml files, because $editor->settings contains text editor plugin-specific settings, but this issue is breaking that. That's why I moved the settings from $editor->settings['image_upload'] to $editor->image_upload.
3. As quicksketch said in #31 (and this is true since that version of the patch): the image upload settings form was owned by the DrupalImage plugin rather than editor.module. So other text editors would need to recreate this form. I fixed that. editor.module now provides a function to generate the subform necessary for image upload settings, so that other text editor plugins can just reuse this.

Follow-ups filed

1. Handle uninstallation of editor.module: all file usages should be deleted. Sadly, the file usage service's API prevents this from being done properly. Filed an issue: #2060405: Impossible to uninstall any module using the file usage service, because it is impossible to delete all file usages by a module.
2. allow an image style to be selected in the image dialog, which gets stored in a data-imagestyle attribute, and is handled by a yet-to-be-added imagestyle filter. However… it would probably be even better if this would be integrated with the picture module instead, so the images are responsive. Everything remains analogous: instead of selecting an image style, you'd select a picture mapping. See #2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5.

Remaining flaws

1. _editor_get_processed_text_fields() essentially hardcodes text.module's field types. I'm not sure how we can get around that. I can think of work-arounds that will enable Drupal 8 contrib to inject more field types, but I cannot think of a way to have them being found automatically; the entity/field system metadata is not rich enough for that AFAIK. In the grand scheme of things, I think this is only a small flaw though, and one that is not caused by or inherent to this issue.

Noteworthy remarks

• I noticed that hook_entity_revision_delete() was not present in entity.api.php, so I fixed that too.
• It is now much easier to insert images that are owned by the Drupal site. But it is now impossible again to insert external images. Considering that the "Basic HTML" text format disallows external images anyway, it is not a problem for the majority of users: only users with access to the "Full HTML" text format are allowed to use external images, and if they have access to that, then it is probably okay to assume they know how to manually write an <img> tag. Furthermore, hotlinking external images is generally a bad practice anyway (URL might change, resource might cease to exist, hotlink protection might break it, inherently worse front-end performance, and so on). Finally, by installing https://drupal.org/project/filefield_sources, you would simultaneously update the "insert image" experience in the WYSIWYG editor, which is highly desireable.
• It does not work with Views. We could add explicit support for that, but that's problematic too. Let me explain. I got rid of the hacks in TextProcessed::setValue() (which was utterly broken because when that is called, no entity ID has been generated yet for new entities), and replaced that with hook_entity_(insert|update|delete|revision_delete)() implementations. All is well for nodes and other fieldable entities. However, this does not work for Views: we have no way of knowing where in a View (Entity) object textual information lives that uses a text format: because it is not a fieldable entity, it does not implement getPropertyDefinitions(), which is what we use for all other entities to figure out where we need to record file usages. That being said, I think this is very much an edge case, and as such it should be handled in a follow-up. The >95% use case is to be able to use this on nodes and other fieldable entities.
  What's more, is that in the two locations where Views uses #type = text_format, it actually explicitly sets #wysiwyg = FALSE, to instruct http://drupal.org/project/wysiwyg to not apply a WYSIWYG editor. (I discovered this while trying to figure out a way to make it work regardless of the above reasoning on why not to do it.) Because Drupal 8 requires it to use #editor = FALSE, that isn't happening yet. I'm not sure why they did this, but I'm sure it's with good reason, so I think it's better for all of us to just not worry about this for now.

Screenshots

This whole thing is less than ideal. Ideally, we'd have a media library in Drupal core, and allow the user to upload files to the media library, and select files from the media library. We don't have that. The current UX is bad. This patch makes the UX much more in line with what most of us expect to be able to do. It's technically less elegant than it should be, but technical elegance can't stand in the way of better UX.
But … the overall approach taken here, with a data- attribute containing the file's UUID, also paves the way for an easier upgrade path in Drupal 9 to ship Drupal with a built-in media library!

MADNESS!

This probably needs usability review.

#38: editor_image_uploads-1932652-38.patch queued for re-testing.

#wysiwyg = FALSE sounds like porting leftover...

#43: yep, exactly :)

Okay, the failure is legitimate. Reproduced locally. When running Drupal\node\Tests\NodeRevisionsAllTestCase:

A fatal error occurred: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '))' at line 2: SELECT base_table.fid AS fid, base_table.fid AS base_table_fid
FROM 
{file_managed} base_table
INNER JOIN {file_managed} file_managed ON file_managed.fid = base_table.fid
WHERE  (file_managed.uuid IN  ()) ; Array
(
)

WTF.

I must say I don't yet see what causes this.

Found & fixed.

Also: FML. I'd written full test coverage for editor.module's entity hooks but failed to git add the file. Now it's gone. Rewriting it at 22:20 on a Friday night. :/

Reproduced the full test coverage from memory, fueled by http://palm.be.

The follow-up #2060405: Impossible to uninstall any module using the file usage service, because it is impossible to delete all file usages by a module has been fixed — yay!

Can we now get this issue moving forward too? Anybody up for reviews? Anybody who wants to trade patch reviews?

I looked at the config changes vs. the schema changes and they all look good.

Thanks! Now we still need PHP, JavaScript and usability review!

Reviewed the PHP code as well. Generally good. Some notes!

@@ -315,6 +315,18 @@ function hook_entity_delete(Drupal\Core\Entity\EntityInterface $entity) {
+function hook_entity_revision_delete(Drupal\Core\Entity\EntityInterface $entity) {
+  // @todo: code example
+}

IMHO feel free to put in the same code as the one you implemented :)

Also, is this hook already there but was undocumented? I miss a change in the patch where this is invoked.

@@ -0,0 +1,131 @@
+    '#title' => t('Width'),
...
+    '#placeholder' => 'width',
...
+    '#title' => t('Height'),
...
+    '#placeholder' => 'height',

It is strange to see the placeholder being the same as the label on the field. What additional information does this add (beyond being noise?). If it does, t()-ing it would be needed too, no? It is user facing text.

@@ -384,3 +385,185 @@ function editor_pre_render_format($element) {
+  $text_field_types = array(
+    'field_item:text',
+    'field_item:text_long',
+    'field_item:text_with_summary'
+  );

Should this be extensible somehow? Contrib can add more text-like fields that would work with ckeditor possibly, no? Not sure if I have a use case from the top of my head though.

Maybe we should just look at the text processing setting, and assume if it is on, then regardless of type, the field is text. :)

@@ -42,16 +44,50 @@ public function buildForm(array $form, array &$form_state, FilterFormat $filter_
+    // If the editor has image uploads enabled, show a managed_file form item,
+    // otherwise show a (file URL) text form item.
+    if ($editor->image_upload['status'] === '1') {
+      $form['attributes']['src']['#access'] = FALSE;
+    }
+    else {
+      $form['fid']['#access'] = FALSE;
+    }
+

Is this extensible to eg. an image gallery module with regular form alter? I assume so being in a buildForm(), but wanted to make sure this is taken into account.

@@ -0,0 +1,118 @@
+ * Definition of \Drupal\editor\Tests\EditorFileUsageTest.

This should now be Contains \Drupal... no?

@@ -0,0 +1,118 @@
+use Drupal\simpletest\DrupalUnitTestBase;
+
+
+/**

Too much whitespace.

Crosspost.

Looking at this, I see no usability issues. The only thing that marked me as odd is that the file field is now above the body? That shouldn't be like that. I also expected this would add a file, to the file upload field (in its listing) - but perhaps thats just previous conversations shaping the expected behaviour.

The files listings and modal dialog are still horrible in terms of UX, but that is not part of this issue :)

#53:

IMHO feel free to put in the same code as the one you implemented :)

I first did that, but all other examples are WRT some example_entity, so an example in line with those should be thought of.

Also, is this hook already there but was undocumented? I miss a change in the patch where this is invoked.

Yes, it is an existing hook.

It is strange to see the placeholder being the same as the label on the field. What additional information does this add (beyond being noise?). If it does, t()-ing it would be needed too, no? It is user facing text.

This is done because the form element label (#title) is hidden (#title_display => invisible), so that we can have a <width> x <height> pixels form with the two form fields inline. That's also why we want/need #placeholder.

That being said, yes, it should've been t()'d. Fixed.

Should this be extensible somehow? Contrib can add more text-like fields that would work with ckeditor possibly, no? Not sure if I have a use case from the top of my head though.

Maybe we should just look at the text processing setting, and assume if it is on, then regardless of type, the field is text. :)

Yes, it should be extensible. It is listed as remaining flaw number 1 in #38.

Great suggestion: implemented! :)

This should now be Contains \Drupal... no?

You're right. Bad copy pasta.

All fixed now!

#54:

The only thing that marked me as odd is that the file field is now above the body? That shouldn't be like that.

That's also not part of this issue. It's been like that in Drupal 8 for a long, long time. As long as I can remember, actually.

I also expected this would add a file, to the file upload field (in its listing) - but perhaps thats just previous conversations shaping the expected behaviour.

That's intentionally not the case, and indeed probably previous conversations shaping the expectations :)

Thanks for the usability review!

A placeholder that is just the same like the title does not add any helpful info. Aside of this the placeholder is not written ucfirst. However I'm strongly for removing this as it does not add anything helpful. A "number" as example placeholder may makes sense here and is helpful.

I don't see anything wrong here, the selectors are a bit gross, but that's inherent to how the name attribute is built.

@hass: the label is not visible, that was the point. Maybe a screenshot would be helpful for you? :)

Well, a screenshot can help a lot :-). Than we have the ucfirst issue only in placeholder text.

#59: there already is a screenshot in #38, but I'll repeat it here for you:

It will look very weird/crappy/broken if we change width x height pixels to Width x Height pixels. I veto against that. Automatically lowercasing that is also inherently broken, e.g. in German it'd need to be Breite x Höhe Pixel, but would then become breite x höhe. Isn't the proper solution to add a translation context?

Hmm... translation contexts are generally used for giving semantic meaning to a word or string. I don't really see why that's needed, though. Following your example:
we just do t('width') and the German translation of 'width' (lowercase) is 'Breite' (uppercase) because 'breite' (lowercase) is not a proper word in German. KISS?

I don't really see why that's needed, though. Following your example:
we just do t('width') and the German translation of 'width' (lowercase) is 'Breite' (uppercase) because 'breite' (lowercase) is not a proper word in German. KISS?

I agree! That's exactly what I have now :) But … hass is arguing that that is bad, see #56. I don't see a cleaner solution either, so unless hass comes up with a better solution, I think the current patch is good to go. I don't want this issue to derail even more than it already has, so if hass feels very strongly about this, we can change it in a follow-up.

So, now we've had a schema.yml (#50), usability (#54), PHP (#52) and JS (#57) review.

Does that mean this issue is now RTBC?

I'm coming from #421118: [Meta] Standardize capitalization on actions that may has a misleading title regarding the issue here, but we are changing all this words for the reason of context. If we have English lcfirst we should not translate ucfirst in German. I fear that we run into the same issues like #421118: [Meta] Standardize capitalization on actions and wished we change all the single/two words strings to ucfirst. I'm not a native english speaker, but why is Width x Height pixels weird/crappy/broken? Add/Delete/Change looks not weird/crappy/broken, but Width x Height pixels does? I'd like to understand... :-)

I must say that I'm sort of torn about t('width') vs. t('Width'). The former looks weird in code, the latter looks weird in the UI (both arguably), so I guess the latter would be my tentative choice.

However,

If we have English lcfirst we should not translate ucfirst in German

is simply not correct. "width" is a proper English word with the translation "Breite". That's just the difference in capitalization in the two languages. You can never say "breite" in Germany, while you can say both "Width" and "width" in English.
Again, that's not to say that we should definitely stick to lowercase, that's just to say that we don't have a problem with translatability either way.

#63, #64: please start a new issue — this discussion is very much offtopic. The code under scrutiny already is part of Drupal core, so you can discuss and fix it in a separate issue: http://drupalcode.org/project/drupal.git/blob/394b9d158f8491dbd4f1c2733d...

I'll remove the t()-wrapping that was introduced in this patch.

Ahh, I see. I did not realize that. In that case, you are of course correct. That totally belongs in another issue. Opened #2067119: 'width' and 'height' HTML placeholder attributes in image dialog are not translated, yet should be lcfirst for that.

NW because I should reroll it to exclude the translation hunks being discussed in #56–#66.

I'm fine with removing the translation code and discussing it separately, since it looks like an unexpectedly contentious issue.

As per #67: removed the contentious translation hunks, which are now being dealt with at #2067119: 'width' and 'height' HTML placeholder attributes in image dialog are not translated, yet should be lcfirst.

Related but not blocking, dialog placement calculations don't currently take displacing elements like the Toolbar into account when determining the position:

#2067263: Drupal dialog placement must take displacing viewport elements, like the Toolbar, into account when calculating placement

#69: editor_image_uploads-1932652-69.patch queued for re-testing.

Let's get label naming consistent:

This is the current admin form for the plugin:

Here's what I suggest as label changes:

The intent is to make consistent through the UI that this feature concerns inline image placement.

I noticed a CSS issue with in-place editing while testing. I've logged it as a bug: #2067285: The entity in-place editing toolbar appears above the Drupal dialog.

If we get this committed soon, it'll be my responsibility to make sure it works with the in-place editing aural UI flow: #1844220: Make in-place editing keyboard and aurally accessible.

Other than the label changes in #73, the JavaScript code looks fine. I read through the PHP code and it seems sane and straightforward. The translation issue has been shifted to the right queue, rather than being debated here. It's almost RTBC, once the labeling issues are addressed.

#73: I like what you're trying to do, and I agree that the "Drupal image" tab name is rather … arcane. However … the tab names are generated automatically from the CKEditor plugin names. And you've guessed it, the CKEditor plugin name is "Drupal image".
We can't rename the CKEditor plugin to "inline images", that'd be wrong. I can rename it to just "Image" though.

Furthermore, I *can* add a title to the vertical tabs container: "CKEditor plugin settings", just like we have "Filter settings" further down on that page. I think that'll help in clarifying what this is all about.

Finally, "Allow inline images" is similarly wrong, it implies that if you don't check that box, the user won't be able to insert images at all. But all it is intended to do is precisely what the previous checkbox said: it enables image uploads. Changing it to "Allow image uploads" is fine though.

So, combining all this, I end up with this:

I hope that you'll find this a good compromise.

Forgot to attach interdiff, which is tiny:

Can we change "Allow image uploads" to "Enable image uploads", please? Allow sounds like a permission where enable is the normal wording if we enable features.

#77: That's what I had. I changed it based on jessebeach's feedback in #73. Please find consensus with her.

Updated issue summary.

Added #2067833: Add image alignment to WYSIWYG to related cases as this may should be part of this issue, but isn't.

Straight reroll of #75 after #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity landed.

Darn, I missed a spot :/ Will reroll.

.

The summary of the VT should probably include whether its allowed. Thats the most important setting.

I'm fine with "Enable image uploads". No sense blocking an important issue on inanity.

Yay! So I will:

1. change back to "Enable image uploads"
2. expand the Vertical Tab summary

Then this should be RTBC!

1. That sounds good.

Voila, done!

1. +++ b/core/modules/editor/editor.admin.inc
   @@ -0,0 +1,131 @@
   +    '#description' => t('A directory relative to Drupal\'s files directory where uploaded images will be stored.'),
   

   I think the standard is to use "" when the string contains a '

2. +++ b/core/modules/editor/editor.module
   @@ -384,3 +385,181 @@ function editor_pre_render_format($element) {
   +    $instance = field_info_instance($entity->entityType(), $field, $entity->bundle());
   

   field_info_instance is deprecated, use Drupal\field\FieldInfo::getInstance() instead.

3. +++ b/core/modules/editor/lib/Drupal/editor/Form/EditorImageDialog.php
   @@ -120,6 +156,16 @@ public function validateForm(array &$form, array &$form_state) {
   +      $form_state['values']['attributes']['src'] = str_replace($_SERVER['DOCUMENT_ROOT'], '', $stream->realpath());
   

   Does this work if Drupal is installed in a subdirectory?

#90:

1. IDK about that. It's easy to find lots of other examples in core where it's also like this. Anyway: changed it.
2. Good catch, changed!
3. That is a very very good question, sir! I didn't touch that code (it's @nirbhasa's code), but I should definitely have noticed that myself while working on this. Now converted to using file_create_url(), as it should!

Jesse Beach found a huge bug.

1. upload + insert an image
2. replace it with another image
3. it will appear as having changed in the CKEditor iframe, but if you switch to source mode, you'll notice that it actually hasn't changed

Ignore #92, this bug is not being introduced here, but was introduced in #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms. Opened a new issue for that: #2071957: Cannot change existing images or links through Text Editor's image/link dialogs.

UI before:

The only way to reference an image is via copy/pasting a URL. So my workflow is:

1) Type some stuff into the WYSIWYG editor.
2) Try and add a picture.
3) Go "(*&(@#" because I don't have a URL to the picture I want. :P~
4) Close the dialog, scroll *up* to "Image" and hit "Browse..."
5) Select the picture from my desktop or whatever.
6) Click upload.
7) Right-click to copy the URL.
8) *Re*-click the "add picture" icon in the WYSIWYG editor.
9) Paste the URL.
10) Click save.

Annoying.

With the patch:

Steps:

1) Type some stuff into the WYSIWYG editor.
2) Click the "Add picture" icon.
3) Hit "Browse..."
4) Select a picture from my desktop or whatever.
5) Click "Save."

Muuuuuch better!

Oh. Except for:

Wah-Wah. What happened there? I assume this has to do with the "secure image" filter (or whatever it's called) that's enabled on Basic HTML, because it works on Full HTML, albeit with the following errors:

Notice: Array to string conversion in editor_entity_update() (line 422 of core/modules/editor/editor.module).

That's a pretty big deal, so needs work for that.

A few code-y things, though I'm running out of steam atm so this might not be everything...

+++ b/core/includes/entity.api.php
@@ -315,6 +315,18 @@ function hook_entity_delete(Drupal\Core\Entity\EntityInterface $entity) {
+function hook_entity_revision_delete(Drupal\Core\Entity\EntityInterface $entity) {
+  // @todo: code example
+}

Sorry, docs gate. We can't let this patch go in without this.

+++ b/core/modules/ckeditor/ckeditor.module
@@ -55,6 +55,20 @@ function ckeditor_library_info() {
+  $libraries['drupal.ckeditor.drupalimage.admin'] = array(

Typically we don't smooshwordstogether. Is this a CKEditor convention? If not, "drupal.image" or "drupal-image" would be better.

+++ b/core/modules/editor/editor.admin.inc
@@ -0,0 +1,131 @@
+function editor_image_upload_settings_form(Editor $editor) {

AFAIK this should be using the new FormBase class now.

Notice: Array to string conversion in editor_entity_update() (line 422 of core/modules/editor/editor.module).

I tried everything I could think of and could not reproduce this. Furthermore, this has test coverage, and zero exceptions are raised there (I tried again locally).

If you're still seeing this, please provide steps to reproduce.

Sorry, docs gate. We can't let this patch go in without this.

No.

Whomever introduced hook_entity_revision_delete() (it is NOT being introduced in this issue) just forgot to add it to entity.api.php. There's another hook in there missing a detailed example. It's not my responsibility to come up with a nice example that's consistent with the rest. I just wanted to make sure we don't forget to do it in the future. The alternative is to just remove this hunk.

Typically we don't smooshwordstogether. Is this a CKEditor convention? If not, "drupal.image" or "drupal-image" would be better.

The plugin names already are "drupallink" and "drupalimage" in the directory structure, so if we want to change this, that's out of scope.

AFAIK this should be using the new FormBase class now.

No.

FormBase/FormInterface are for "full", "stand-alone" forms, with their own validate and submit handlers. This is not a full form. It has no validate and submit handlers of its own This is a subform that other forms may embed. DrupalImage::settingsForm() embeds it.

From #38:

As quicksketch said in #31 (and this is true since that version of the patch): the image upload settings form was owned by the DrupalImage plugin rather than editor.module. So other text editors would need to recreate this form. I fixed that. editor.module now provides a function to generate the subform necessary for image upload settings, so that other text editor plugins can just reuse this.

Since none of the criticism in #94 seems to be accurate or reproducible, setting back to RTBC.

$added_files = array_diff($uuids_by_field[$field], $original_uuids_by_field);

That's the line. array_diff() only works on arrays that only have scalar values. PHP 5.3 just silently casts arrays to the string 'Array' and compares that.

I'm on 5.3.18, so that'd explain it. Thanks!

EDIT: Too late.

Upgraded to PHP 5.4, reproduced, fixed. Thanks again, berdir! :)

(The updated code makes more sense and it now puzzles me how this ever worked — but it did work! You're so weird, PHP.)

Hm. It's still showing a broken image to me once saving the node form. :(

Steps to reproduce:

- Apply the patch and drush si.
- Go to node/add/article.
- Enter a title.
- Click the image button on the WYSIWYG
- Browse to a picture from your desktop or whatever.
- Click Save. You'll see the image appear in the WYSIWYG editor.
- Click "Save and publish" on the form.
- Node page shows up and... red X.

However, I can confirm that the notices are gone when I switch the text format to "Full HTML" so there's that. :)

I could reproduce the notice earlier, but not the image not showing. It showed up for me.

:(

And your input format is set to "Basic HTML"? (or whatever the default is?)

This fails consistently for me every time. I'm drush si-ing in between. Not sure what's wrong.

git pull
sudo cp sites/default/default.settings.php sites/default/settings.php
chmod ug+w sites/default/settings.php
sudo rm -rf sites/default/files/*
mysql -u root -p[password] -e 'SHOW TABLES FROM drupal8' | while read t; do mysql -u root -p[password] -e 'DROP TABLE drupal8.'$t; done
curl https://drupal.org/files/editor_image_uploads-1932652-100.patch | git apply

I install with the interactive installer, so maybe it's a Drush issue with permissions on the files directory?

I go to Add content, Article, fill in title, fill in body, click the CKEditor image button, browse for a file (tried both with and without clicking the upload button before clicking save), click save in the dialog window, click save and publish.

Ha, ok. After some further debugging, this is the problem... #2076847: FilterHtmlImageSecure incorrectly flags images coming from site URLs with ports in them as insecure.

I guess I will have to take your guys's word for it that this works. :)

Committed and pushed to 8.x. Thanks!

#106: indeed, that's the cause of that problem!

And WOOT! :D

Glad to see a happy quicksketch in the D8 issue queue :)

Yay! Kudos to all who contributed to this patch!