Imagefield formatters don't respect visibility options

CCK: 6.x-2.1
imagefield: ImageField 6.x-3.0-alpha3, alpha4
permissions for your tmp and files directory: 777
download method: public
imagefield configuration:

Default list value: listed/hidden
How should the list value be handled?: User Configurable/Enforce Default

subscribe

same issue here

If I change the setting to

Default list value: * to hidden and after saving it auto reverts to listed. Same with the enforced/user configurable. It is automatically reverting to the user configurable.

Can anyone explain what this is anyways?

"The list option determines whether files are visible on node views. This will be used as the default value for the list option."

I don't want this List checkbox showing up on my upload form. It's ugly and I don't need it. I want the file showing up no matter what!

video: http://www.utipu.com/app/invited/id/2c881ac5a53f4588a0f8ffa71b96fcb1

amsgwp, i'm not seeing what so hard about it. select "Listed" and then select "Enforce Default". They'll always be listed and uses aren't presented with an option.

I'm not sure why you're settings aren't sticking. That's pretty odd.

I confirm this bug.... I have unchecked the listed for some images and they are shown in the node... if I put "Generic files" as the display mode they are hidden. But I want to use the image preset.

another bug I have is that When I add dynamically more fields with the "add another" button.. the List checkbox is not selected or checked by default.

The list checkbox has been fixed and improved in #388196: Revise the Field Configuration Options.

if I put "Generic files" as the display mode they are hidden. But I want to use the image preset.

This is because ImageCache does not respect the "list" option. We've considered making it respect it, but adding the option now seems like it would likely break a lot of sites that assume all the images are going to show up.

I think the best solution really would be to remove the "list" option entirely from ImageFields and have them always default to listed. We can leave this option in for FileField, but seems like the number of users that actually want this feature are extremely small, it's there currently because it's a side-effect of building on top of FileField.

I really think image cache should respect the list option.
there could be an option in the administration section with a button to toggle all imagefields list option on /off quickly for all fields in database.

Marked #305238: List Check Box behavior as duplicate.

Subscribing. I too would prefer Imagecache respecting the list value. There are scenarios where you want users to be able to choose.

But the current situation is confusing, so if it's not possible to have ImageCache respect the setting (or not desirable for most people), it makes sense not to have the list option for imagefields.

I would prefer imagecache to respect the list value also.

There are numerous use cases where this makes the most sense. Especially with solutions like: http://drupal.org/project/filefield_insert . When you insert an image it shows up from the file/imagefield and in the node body.

At Zivtech we'd be willing to create the patch or whatever it takes to get this working. What roughly needs to be done? Should it remain a patch or be integrated into the module, or a small separate module?

acouch, thanks for volunteering to create a patch. There a couple options available.

We could attempt to move processing of the "list" option from the theme_filefield_item() to filefield_field_sanitize(). This would make a single point of change that would immediately affect all FileField-based modules, including all the formatters provided by ImageField and ImageCache.

Or, we could update the formatters provided by ImageField and ImageCache to do the same check as theme_filefield_item().

Regardless of the approach we take, an _update function must be provided that ensures that files don't suddenly stop (or start) displaying after upgrading to the new version. It's a very dangerous move, since it's unlikely that we'll be able to not break sites considering the lax use of the list option previously. While I agree that it'd be nice for ImageField and ImageCache to obey the "list" option, I don't know if it's feasible to make this change now. The last thing we'd want is for users to update an all the images on their site to disappear because the list option is enabled but unchecked on all their files (a very common combination unfortunately).

Cool. We'll review and let you know if there is an approach that makes the most sense to us.

If an update function sets all imagefiled images to 'listed' then folks would be starting where they left off since all of the images were visible previously. Right?

If an update function sets all imagefiled images to 'listed' then folks would be starting where they left off since all of the images were visible previously. Right?

That's mostly correct. You'll need to check which formatter was being used when displaying the field and set "list" = 1 when any formatter was being used other than the ones provided by FileField.

Actually, not only does Imagecache fail to respect the "list" checkbox but imagefield ignores it as well. I have attached patches for each that cause them to respect the the list item but did not yet tackle the problem quicksketch pointed out with creaing an _update function to prevent us from breaking sites because I think it is a bit more complicated than he indicated in the previous comment.

A content type could be set to use generic files in the teaser but full in the body and that doesn't even take into consideration views. Perhaps we could loop through each field on each content type and each view making sure that nowhere in the site is there a conflict and then proceed to set the list value to 1, but even then we'll need some kind of an interface to explain the problem and to inform the user of which nodes have conflicts that need to be resolved. Is it more important to ensure that we don't have a broken interface element in an important module or not to break sites? This is a pretty rare case and the list checkbox is off by default and even then list is enabled by default. I think this probably affects a relatively small group of users. Tough call...

Also, each patch adds a (slightly modified) copy of filefield_file_listed from file field's filefield_formatter.inc. It might be a good idea to move this function into a file that is always included (filefield.module maybe?) to prevent the redundancy.

A couple things that might improve these patches:

- Use the shortcutted operator "||" instead of "|", there's a slight (very, very slight) performance improvement that the second condition is never checked if the first one fails. More importantly it matches code standards.
- ImageField fully depends on FileField, so you can just use filefield_file_listed() instead of making a duplicate function.
- You should use isset() when checking values such as $field['list_field'], to prevent PHP notices.

Of course a update paths are needed also, including display settings stored by CCK and formatters within Views. Unfortunately there's nothing that you can do about code-based default views (a common practice among shops that store views in code for version control) other than give them a warning. But for such circumstances usually just a notice in the release notes is needed.

Thanks for the effort so far, though I'm still not sure what to do with such patches upon completion. As you note it's a risky business, one that's not entirely clear whether the benifits of increased flexibility outweigh potentially breaking existing sites.

I realize that imagefield fully depends on filefield but the filefield_file_listed() function is in filefield_formatter.inc which I believe is only conditionally included by CCK? If I try to call that function directly I get "Fatal error: Call to undefined function filefield_file_listed()".

As for the isset(), that section was copied from filefield_formatter.inc. Should I file a patch to change that as well? I'm guessing that the thinking was that these formatters are always applied to fields created by filefield so it will always be set but perhaps that's too big an assumption?

As for upgrading, I hadn't thought of the issue with code based views. I have to imagine that the number of people relying on images ignoring the list setting is pretty small. These people have to go to the trouble of setting "list field" to enabled and then further uncheck "files listed" to even potentially have a problem.

It seems like the only people that will have trouble with the issue is if someone has two different display types and want the list_field check to work on only one of them (a generic list of select files on the teaser and all of the actual images displayed on the actual node, etc.). These users are likely fairly advanced, could a notice be sufficient for these limited cases?

It seems like a "conflict resolver" script would be a lot of work and legacy code for what might actually be an extremely limited use case.

setting status

These people have to go to the trouble of setting "list field" to enabled and then further uncheck "files listed" to even potentially have a problem.

I changed these default settings in FileField 3.0 beta1 about 3 months ago, it used to be completely opposite and the default was to have the list field enabled and not listed by default. :-P So it's likely there are thousands of sites that have that as the setting, considering it was the default for FileField in Drupal 6 for over a year. An upgrade path is absolutely a requirement for these patches both for CCK formatters and database views.

tizzo,

I realize that imagefield fully depends on filefield but the filefield_file_listed() function is in filefield_formatter.inc which I believe is only conditionally included by CCK? If I try to call that function directly I get "Fatal error: Call to undefined function filefield_file_listed()".

see module_load_include(), e.g. module_load_include('inc', 'filefield', 'filefield_formatter');

Thanks drewish! I hadn't run across that function before! I rerolled the patch using filefield's function.

I didn't realize unlisted had been the default. Would a fair update path be to loop through the display formatters for each field on each content type and on each view and set the list option to 1 if there is no conflict (all of the formatters used are either imagefield or imagecache) and then alert the user if there are conflicts while taking no action? I suppose that could break a number of sites though less.

The only alternative I see is to then set a variable in the variables table that a conflict has occurred and check for that before respecting the setting. This would require some kind of interface for conflict resolution (listing the node title in the table along with each file and a checkbox whether to list it or not)?

What is the situation with D7? With filefield and imagecache going into core will this problem be moved into core? Can we apply a patch to core without an upgrade fix? Or would that be too big an obstacle to add to the core upgrade path?

subscribing

Sorry for the delay.

Here is a patch with at least a first swipe at upgrade scripts. It errors on the side of showing files by looping through every content type and every view looking at each field in each and checking to see if the display field is != default (generic file). If it is a filefield and is displayed as something other than a generic file anywhere on the site all entries for that field are updated to list = 1.

I also added a variable_set and variable_get so that each module can see whether the other has run this update and abort if it has. This way if someone makes changes to the list settings after making these list options actually do something they will not loose their changes by applying another update.

Thoughts?

Giving this a bump, I think these patches solve the problem roughly as we've discussed it should be solved.

The use of variable_get/set is extremely odd here. Instead you should be using drupal_get_schema_versions($module) and checking the schema number if necessary. Otherwise we're permanently adding variables that will never be properly cleaned up. In addition, the patch still isn't quite following coding standards. Start comments with capital letters, and end with periods. Put a space between logic constructs and parenthesis if ($condition) not if($condition). Brackets are missing around table names in updates. Rather than using content_types and trying to manually assemble table names and field names, you should use content_types_install() to get a list of fields.

subscribing

subscribing

subscribing

subscribing

Hi,

any news on this issue?

For us who inline images in the body (I do that with #394682: CCK fields (filefield,imagefield,emfield) and fieldgroups for Inline API), this change is very important.

Thanks,
Antonio

I think it's unlikely that this bug will be fixed, since the Drupal 7 version of Image module doesn't even have the "Display" option, so this ability will be lost when upgrading to Drupal 7 anyway.

But there will be another way to control visibility of a field in D7, right? So an upgrade path could still handle this. And considering D6 will stay there for a while this issue could still have sense.

A question I have, can an upgrade procedure be interactive? I mean, could a module propose to the user two alternative upgrade paths to choose? (Display checked for all vs. Display unchecked for all).

Thanks,
Antonio

Having the visibility("list") option on the imagefield is VERY useful.

I am using file field images, one use is to allow the user to add images to a slideshow on their node by checking the "list" option next to the images when editing.

To workaround this bug:

• I turned off display of my image_field in the CCK
• I used Contemplate Module to override the body template with:

This checks the "list" property of each image before displaying it with the defined "view". And I output the body beforehand w/o this field.

But there will be another way to control visibility of a field in D7, right?

D7 will have the Content Permissions module (or more likely renamed to "Field permissions"), which allows per-field access. However D7 will not have the ability to show/hide individual images when uploaded through ImageField.

D7 will have the Content Permissions module (or more likely renamed to "Field permissions"), which allows per-field access. However D7 will not have the ability to show/hide individual images when uploaded through ImageField.

Ahhh, this doesn't sound very good :(
Slightly OT: What about Filefield? Will the per-item visibility option be removed there?

Slightly OT: What about Filefield? Will the per-item visibility option be removed there?

FileField still has per-item visibility and it will have an upgrade path (for both FileField and Upload module) from D6.

FileField still has per-item visibility and it will have an upgrade path (for both FileField and Upload module) from D6.

Then, forgive me, I cannot see why the asymmetry with Imagefield, being it in a IS-A relationship with Filefield.
I can understand the practical difficulties in upgrading the "List" option for current installations of Imagefield, but I still hope these will be tackled and resolved someway.

bump