http://drupal.org/user/23728 reports that the image field settings are broken: the file is saved to the wrong schema and is not set to permanent. Attached tries to fix both. Tests are needed. I will see what I can come up with test wise...

CommentFileSizeAuthor
#91 default_image_1028092_91.patch6.08 KBchx
PASSED: [[SimpleTest]]: [MySQL] 30,065 pass(es). View
#91 interdiff_90_91.txt695 byteschx
#88 default-image.patch5.82 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 30,016 pass(es). View
#89 default-image.patch5.81 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 30,053 pass(es). View
#87 default_image_save-1028092-87.patch6.29 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 30,060 pass(es). View
#84 default-image.patch5.82 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 30,052 pass(es). View
#82 1028092_82_img_default.patch6.55 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 30,043 pass(es). View
#80 1028092_80_img_default.patch6.55 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#43 1028092_43_img_default.patch6.15 KBscor
PASSED: [[SimpleTest]]: [MySQL] 31,591 pass(es). View
#34 1028092_33.patch6.14 KBchx
PASSED: [[SimpleTest]]: [MySQL] 31,580 pass(es). View
#30 default_image_status-1028092-30.patch6.15 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 31,579 pass(es). View
#29 default_image_status-1028092-29.patch6.19 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 31,588 pass(es). View
#14 default_image_status-1028092-14-eojthebrave.patch6.07 KBeojthebrave
PASSED: [[SimpleTest]]: [MySQL] 31,548 pass(es). View
#7 drupal-1028092.patch3.46 KBSweetchuck
PASSED: [[SimpleTest]]: [MySQL] 31,552 pass(es). View
#5 image_default_image_handling.patch2.64 KBchx
PASSED: [[SimpleTest]]: [MySQL] 31,548 pass(es). View
#4 image_boy_this_is_very_ugly_fapi_code.patch2.07 KBchx
PASSED: [[SimpleTest]]: [MySQL] 31,499 pass(es). View
image_boy_this_is_very_ugly_fapi_code.patch1.97 KBchx
FAILED: [[SimpleTest]]: [MySQL] 31,497 pass(es), 1 fail(s), and 1 exception(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Status: Needs review » Needs work

The last submitted patch, image_boy_this_is_very_ugly_fapi_code.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

If you are wondering whether this is a security issue, we wondered too but neclimdul, davereid, greggles and myself have decided the security issue here is so hard to exploit if at all exists that we decided to fix the issue in public. It can be argued that there is a slight information disclosure if you are set up for private files and upload a default image which gets into public instead of private -- however, Drupal drops an Options None into files disabling DirectoryIndex and so you have a six hours window to figure out a file name without knowing when to do it and also it's not user uploaded data that has this problem only the default. Extremely hard to exploit, extremely rare and the critical data loss bug -- the file not made permanent -- rather needs a lot of eyes and hands and so we decided on making the poor issue public.

The patch above lacks file usage handling and so results in a file validate error, otherwise I can see it working by looking into files directory and the DB.

chx’s picture

Assigned: chx » Unassigned

Also, I unassign lest people think I work on it which is not the case for a day at least.

chx’s picture

FileSize
2.07 KB
PASSED: [[SimpleTest]]: [MySQL] 31,499 pass(es). View

Talked to Dávid more and here is a patch that avoids the validate error. file usage deletion is not yet handled.

chx’s picture

FileSize
2.64 KB
PASSED: [[SimpleTest]]: [MySQL] 31,548 pass(es). View

I took a quick break and added some deletion functionality too.

Dave Reid’s picture

Subscribe

Sweetchuck’s picture

FileSize
3.46 KB
PASSED: [[SimpleTest]]: [MySQL] 31,552 pass(es). View

file_usage fix
#chx: Dávid is my family name, so please call me Andor :-)

chx’s picture

Thanks -- but those functions belong to image.field.inc. Otherwise, looks very promising! Care to write tests? Also, given that you $fid = (is_array($field['settings']['default_image']) ? $field['settings']['default_image']['fid'] : $field['settings']['default_image']); recheck $fid just after this why not

if (is_array($field['settings']['default_image'])) {
  $fid = $field['settings']['default_image']['fid'];
  if ($fid && ($file = file_load($fid))) {
    file_usage_delete($file, 'image', 'default_image', $field['id'], 1);
  }
}
moshe weitzman’s picture

I seem to recall a critical issue 6 months ago or so where files were not marked as permanent and got deleted after 6 hours. Can't find it.

chx’s picture

That was #618654: File and image fields are treated as temporary files and automatically deleted after six hours but that was about the user uploaded images not the default image.

mfer’s picture

Subscribe

carlos8f’s picture

Issue tags: +Needs tests

If I get time I will write some tests.

aaron’s picture

subscribe

eojthebrave’s picture

FileSize
6.07 KB
PASSED: [[SimpleTest]]: [MySQL] 31,548 pass(es). View

Cleaned up the comments and added some tests to make sure that the file status is updated, and that the default image for an image field using private:// gets saved with the private:// scheme and that they still display properly.

Assuming tests pass I like the approach that this takes and it fixes a pretty nasty bug.

chx’s picture

Status: Needs review » Needs work

@eojthebrave thanks for picking this issue up, image_field_settings_uri_scheme_process needs some doxygen. Edit: default images works as expected. <= typo, "image" not "images". Edit2: file_usage_add / delete defaults count to 1 , no need to explicitly wirte it out. Last edit before sleep: + $this->assertRaw($default_output, t('Default image displayed when no user supplied image is present using private:// scheme.')); <= that's misleading , the assert does not check for private:// scheme here.

sun’s picture

+++ modules/image/image.field.inc
@@ -50,7 +50,8 @@ function image_field_settings_form($field, $instance) {
+  ) + element_info('radios');
+  array_unshift($form['uri_scheme']['#process'], 'image_field_settings_uri_scheme_process');

We have element_info_property() for this.

+++ modules/image/image.field.inc
@@ -556,3 +557,15 @@ function theme_image_formatter($variables) {
+function image_field_settings_uri_scheme_process($element, $form_state, &$complete_form) {

Missing phpDoc.

Powered by Dreditor.

AgaPe’s picture

subscribe

byrondover’s picture

Subscribe.

AgaPe’s picture

Hi, sorry but i am not sure - is there a patch for this issue? if so which one is it?

AgaPe’s picture

anybody could help how to apply this patch from #14, i am applying it manualy, don't know what to do with:
+ ) + element_info('radios');
in

-  );
+  ) + element_info('radios');
+  array_unshift($form['uri_scheme']['#process'], 'image_field_settings_uri_scheme_process');

looks like it should be

) element_info('radios');
array_unshift($form['uri_scheme']['#process'], 'image_field_settings_uri_scheme_process');

which does not look like a valid thing..

it hurts its still an issue... :)

RoboPhred’s picture

Do not remove the second + on the first line, that adds in the array returned by element_info. Only the first + or - in the line is part of the patch syntax, the rest is code.

AgaPe’s picture

thanks. patch resulted in image not working and
"Inactive fields are not shown unless their providing modules are enabled. The following fields are not enabled: * Serie Picture (field_media_picture) field requires the Image widget provided by image module"
error

JamesSharpe’s picture

Subscribe

demonhjack’s picture

Status: Needs work » Needs review
marcingy’s picture

Status: Needs review » Needs work

Patch in #14 was already reviewed and set to needs work

Dragan Eror’s picture

Subscribe

dixon_’s picture

Assigned: Unassigned » dixon_

I'm sitting in the Mississippi room on DrupalCon and working on this. I'll basically work on the patch from #14 and make it up to speed with the reviews from #15 and #16.

dixon_’s picture

Assigned: dixon_ » Unassigned
FileSize
6.19 KB
PASSED: [[SimpleTest]]: [MySQL] 31,588 pass(es). View

I have now re-rolled and corrected the patch from #14 with most of the issues mentioned in #15 and #16. What I haven't done:

  • misleading assertion that chx mentioned in #15
  • implement element_info_property() that sun mentions in #16 (I don't fully understand how we should implement that)

So, still needs some work... But I won't work on this for a little bit.

dixon_’s picture

Status: Needs work » Needs review
FileSize
6.15 KB
PASSED: [[SimpleTest]]: [MySQL] 31,579 pass(es). View

I think I've made the assertion description a little bit more correct now. Giving this a test run.

dboulet’s picture

sub

Dragan Eror’s picture

I manually added code from the patch #1028092-30: Default image is not set to permanent and saved to the wrong schema and it seems to works fine.
Thanks @dixon_

drewish’s picture

Status: Needs review » Needs work

Talked to chx about this a bit. I wonder if using an #after_build would be a little cleaner looking. If not we should try restructuring this to be more like:

  $form['uri_scheme'] = array(
// ...
  );
  // TODO: EXPLAIN WHY WE'RE DOING THIS.
  $form['uri_scheme'] = + element_info('radios');
  array_unshift($form['uri_scheme']['#process'], 'image_field_settings_uri_scheme_process');

Also some of the comments in the tests seem redundant.

chx’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
6.14 KB
PASSED: [[SimpleTest]]: [MySQL] 31,580 pass(es). View

I worked with drewish on this, he suggested after_build but then I have thought value_callback and look how pretty the form api code turned out to be. We fixed comments too.

drewish’s picture

Status: Needs review » Reviewed & tested by the community

I'm very happy with the way this has turned out.

dboulet’s picture

Patch in #34 seems to have fixed the problem for me, thanks!

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/modules/image/image.field.inc
@@ -56,7 +56,11 @@ function image_field_settings_form($field, $instance) {
+    // value callback that knows about the uri scheme element.

@@ -64,6 +68,17 @@ function image_field_settings_form($field, $instance) {
+  // Set the uri scheme as possibly set fresh by the user.

"URI" should always be all-uppercase, no?

+++ b/modules/image/image.field.inc
@@ -64,6 +68,17 @@ function image_field_settings_form($field, $instance) {
 /**
+ * A #value_callback for the image field settings form.
+ *
+ * Makes sure that the correct upload location is set.
+ */

1) I think we can merge the description into the phpDoc summary, as in "#value_callback to ensure correct upload location in image field settings form."

2) Can we add a @see file_managed_file_value()?

+++ b/modules/image/image.field.inc
@@ -64,6 +68,17 @@ function image_field_settings_form($field, $instance) {
+function image_field_settings_default_image_value_callback(&$element, $input = FALSE, $form_state = NULL) {
+  // Set the uri scheme as possibly set fresh by the user.
+  $element['#upload_location'] = $element['#uri_scheme_element']['#value'] . '://default_images/';
+  return file_managed_file_value($element, $input, $form_state);
+}

I wonder what happens if the submitted value for URI scheme is invalid?

Ideally, of course, the uploaded default image would/should be kept in a temporary upload location until form validation passed without errors, but I guess exploring that path would circle back to starting the solution attempt from scratch. Hence, I won't obsess on this edge-case and merely mention the possibility of it. A simple solution might be to simply throw away the upload (not invoke file_managed_file_value()) in case of a form validation error on the uri_scheme element; should be a rare edge-case either way. If we do that, let's make sure to explain it in an inline comment.

Lastly, I'm not sure I understand why we're assigning the uri_scheme element by reference here and attempting to use its #value, if we have $form_state readily available? I can only guess it's because of having to mangle and munge with #parents, but in the end, I wonder whether the #parents traversal isn't the current "standard" approach throughout core? Note that we can keep the reference construct, just wanted to point out that it's different from other code, and a short inline comment might help to explain.

And finally, I think this construct of dependent form element values will always fail if someone or something happens to reorder the elements in the form, so the file upload widget comes before the uri_scheme element, and so the input handling for the file upload element won't find a value for the URI scheme. That, we can note in the phpDoc description -- but OTOH, we could also skip the file upload entirely in that case, just like in the case of a form validation error on the uri_scheme.

Powered by Dreditor.

chx’s picture

I wonder how do you plan to figure out where in $form_state that element is. Good luck. I rather not try. #parents is not yet filled in. You want to take our own #array_parents in #value_callback and iterate $form_state['complete form'] based on that? Yeah. That's the ugly version :D readily available in a number of versions above, but this is the pretty version. And we use reference so that we can actually grab the #value. It can't be invalid, the option validation makes sure of that. That's why we have FAPI in core, the option validation.

The rest I can do.

chx’s picture

Oh lord, this is before validation. Need to run our own options checker. (And remove this madness of saving in value callback in D8)

Alexander Matveev’s picture

Subscribe

robertom’s picture

Sorry for noise. Subscribe

Stretsh’s picture

subscribe

scor’s picture

Status: Needs work » Needs review
FileSize
6.15 KB
PASSED: [[SimpleTest]]: [MySQL] 31,591 pass(es). View

Tested patch #37 and it works. rerolled with doc changes suggested by sun in #37. Not sure if we still need to look at the validation issue (last bit of #37).

chx’s picture

Status: Needs review » Needs work

Yes we need to look at the validation issue as the value callback is before validation where the option checker fires and so we need to validate that the option is valid ourselves.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +needs backport to D7

This needs to go into 8.x then be backported to 7.x

Alexander Matveev’s picture

raoul.leysen’s picture

subscribe: waiting for D7 patch

mmilano’s picture

subscribe

bryancasler’s picture

subscribe

olafveerman’s picture

subscribe

aspilicious’s picture

subscribe, btw, this is a very anoying bug. You can't set a default image on anything. The default image I put on an article get deleted. So hopefully someone will tackle the last part.

antmanvolpe’s picture

Not sure why this issue has not been created in Drupal 7 issues queue rather than waiting for a backport from D8.

I created an issues in the D7 queue to be looked into. This is a rather major bug that needs to be addressed.
http://drupal.org/node/1127140

robertom’s picture

Not sure why this issue has not been created in Drupal 7 issues queue rather than waiting for a backport from D8.

this issue was created in drupal 7 queue, but is moved in D8 by catch (see comment #45).
This issue have a tags "needs backport to D7" for don't forget :).

For more information about this "strange" workflow, you can read #1050616: Figure out backport workflow from Drupal 8 to Drupal 7

I created an issues in the D7 queue to be looked into. This is a rather major bug that needs to be addressed.
#1127140: Default images dissappearing

I think #1127140: Default images dissappearing could be closed because is a duplicate of this issue...

Buxyman’s picture

subscribe: waiting for D7 patch

bryancasler’s picture

Buxyman, patch in #43 worked for me on D7

keeldog’s picture

subscribe

garrettrayj’s picture

subscribe

rwohleb’s picture

subscribe

ilya.bezdelev’s picture

subscribe

calvintennant’s picture

when will this patch be committed?

dptronz’s picture

subscribe

chris.leversuch’s picture

subscribe

jbrown’s picture

This patch locks the 'Upload destination' to 'Public files' - it can't be changed.

Cottser’s picture

Subscribe.

geerlingguy’s picture

Subscribe.

khiminrm’s picture

Subscribe

dystopianblue’s picture

Subscribe.

ClaudeS’s picture

subscribe

joostvdl’s picture

subscribe

Need it for D7

eremenko’s picture

subscribe

JCL324’s picture

Why was this bug not fixed/included in yesterday's 7.2 release??

aspilicious’s picture

Because it isn't fixed yet

carlos8f’s picture

@JCL324, the patch needs work still. Once committed to 8.x branch, it can be backported to 7.x and get in the next release.

As I understand it, the problem with the patch is that it uses a value callback to save the file, which gets the schema from &$form['uri_scheme'] (radio element). The option checker validation has not run yet, which means someone could inject an arbitrary schema into the download location. @chx said we might have to manually validate the value.

xenyo’s picture

Subscribe

Reza Fathi’s picture

Subscribe.

MustangGB’s picture

Wow this is super crippling me at the moment
Hope the Drupal demigod's can figure this one out soon

pillarsdotnet’s picture

beejeebus’s picture

subscribe.

i'll chase up chx in IRC about what #44 means exactly.

adamwhite’s picture

Subscribe

beejeebus’s picture

Status: Needs work » Needs review
FileSize
6.55 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

updated patch to add the validation chx asked for in #44.

Status: Needs review » Needs work

The last submitted patch, 1028092_80_img_default.patch, failed testing.

beejeebus’s picture

Status: Needs work » Needs review
FileSize
6.55 KB
PASSED: [[SimpleTest]]: [MySQL] 30,043 pass(es). View

without the stupid this time.

jbrown’s picture

Status: Needs review » Needs work

There is still the problem I mentioned in #63 - with the patch applied, it isn't possible to change the upload destination.

jbrown’s picture

Status: Needs work » Needs review
FileSize
5.82 KB
PASSED: [[SimpleTest]]: [MySQL] 30,052 pass(es). View

A further problem with this approach is that it doesn't work well with AJAX - if you upload the file via AJAX and then change the upload destination before submitting the form, the file is stored in the original scheme.

The attached patch takes a much simpler approach - if the scheme is changed, then the file is moved to the new scheme in the field update hook.

This also avoids the horrible FAPI hack.

pillarsdotnet’s picture

Status: Needs review » Needs work
+++ b/modules/image/image.module
@@ -413,6 +413,58 @@ function image_image_style_delete($style) {
+  // If the upload destination chanaged, then move the file.

Spelling error.

chx’s picture

Hrm, that might even actually be correct. Nice trick! Maybe a comment that by the time we get to the settings form the field actually exists and whatever the user does is just updating the field. See field_ui_field_overview_form_submit() and field_ui_field_settings_form_submit() and field_ui_field_edit_form_submit.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
6.29 KB
PASSED: [[SimpleTest]]: [MySQL] 30,060 pass(es). View

Fixed spelling error.

Maybe a comment that by the time we get to the settings form the field actually exists and whatever the user does is just updating the field. See field_ui_field_overview_form_submit() and field_ui_field_settings_form_submit() and field_ui_field_edit_form_submit.

Are you referencing places where such comments already exist or places where they should be added?

CNR for bot only.

jbrown’s picture

FileSize
5.82 KB
PASSED: [[SimpleTest]]: [MySQL] 30,016 pass(es). View

Sorry - ignore this patch.

jbrown’s picture

FileSize
5.81 KB
PASSED: [[SimpleTest]]: [MySQL] 30,053 pass(es). View

"If there is an old file?" => "Is there an old file?"

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community

nice work! much cleaner, test covers the bug, RTBC.

chx’s picture

FileSize
695 bytes
6.08 KB
PASSED: [[SimpleTest]]: [MySQL] 30,065 pass(es). View

Sigh. As noone added the comment I asked for I needed to. Still RTBC, interdiff attached.

beejeebus’s picture

thanks chx, sorry for the oversight.

MustangGB’s picture

What a fantastic present to wake up to this morning <3
Possible to backport?

aspilicious’s picture

"needs backport to D7"

It's all in the tag...

MustangGB’s picture

What I mean to say so we don't need to wait for a d8 commit before figuring out how to apply this to d7

catch’s picture

@akamustang, the patch may well apply to Drupal 7 cleanly since the code bases haven't really diverged much yet. Try http://drupal.org/patch/apply for instructions.

bryrock’s picture

I applied this patch to a D7 site just minutes ago. So far, so good. That is, the patching took place with no problems.

I've got one content-type with a default image that has been disappearing daily. I've uploaded the default image again, since applying the patch, and will report back here tomorrow on whether or not my nodes of that content type are still displaying their default image.

bryrock’s picture

I'm happy to report that the patch in #91 appears to also solve this problem in Drupal 7. It's been over 24 hours since I applied it to a D7 production site and the default images that have been disappearing daily all continue to appear where they should be. So I say, "thumbs up."

dboulet’s picture

Working for me as well. Would love to see this included in 7.3.

bryancasler’s picture

Working here as well

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome. Thanks SO much for the great work and all the testing in this issue!

Committed to 7.x and 8.x.

swilsondesign’s picture

Subscribe

danbohea’s picture

I'm a little confused as to how to apply this patch since the patches change on May 31st.

I've tried both git apply -v [patchname.patch] and patch -p0 < example.patch from the root of my site and neither seem to work. Any pointers?

beejeebus’s picture

dixhuit - this patch has already been committed to drupal core, so there's nothing to do :-)

scor’s picture

@dixhuit: this fix is included in the latest snapshot of Drupal 7, so if you need that fix for your site, try to use that package if you can. Or if you are using git, just do git pull.

danbohea’s picture

Many thanks :)

Status: Fixed » Closed (fixed)
Issue tags: -needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.

jan.ptacek’s picture

Issue summary: View changes
Status: Closed (fixed) » Needs work

Sorry, this needs a backport do D7 and it seem that being closed (for 1.5year now) it will never get the love a critical issue (in current version of Drupal everyone downloads) deserves.

Sorry if this is against agreed flow.

marcingy’s picture

Status: Needs work » Closed (fixed)
jan.ptacek’s picture

ops, sorry, i am on 7.26 and currently seeing my default images are saved with status 0 and thus disappearing after cron. will investigate more where this regression stems from