Default image is not set to permanent and saved to the wrong schema

Subscribe

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

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.

Subscribe

If I get time I will write some tests.

subscribe

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.

+++ 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.

subscribe

Subscribe.

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

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... :)

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.

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

Subscribe

#14: default_image_status-1028092-14-eojthebrave.patch queued for re-testing.

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

Subscribe

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.

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.

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

sub

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_

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.

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

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

+++ 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.

Subscribe

Sorry for noise. Subscribe

subscribe

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).

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

subscribe: waiting for D7 patch

subscribe

subscribe

subscribe

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.

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

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...

subscribe: waiting for D7 patch

Buxyman, patch in #43 worked for me on D7

subscribe

subscribe

subscribe

subscribe

when will this patch be committed?

subscribe

subscribe

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

Subscribe.

Subscribe.

Subscribe

Subscribe.

subscribe

subscribe

Need it for D7

subscribe

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

Because it isn't fixed yet

@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.

Subscribe

Subscribe.

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

Subscribe

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

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.

+++ 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.

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.

Sorry - ignore this patch.

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

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

"needs backport to D7"

It's all in the tag...

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

@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.

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.

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."

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

Working here as well

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

Committed to 7.x and 8.x.

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?

@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.

Many thanks :)

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.

As per https://drupal.org/comment/4603338#comment-4603338 it was committed to d7.

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