Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Files/Images fields can contain unsaved temp images when output formatting is off #825

Open
Toutouwai opened this issue Mar 5, 2019 · 8 comments

Comments

Projects
None yet
5 participants
@Toutouwai
Copy link

commented Mar 5, 2019

Short description of the issue

Files uploaded to a Files or Images field get a temp status until the page is saved. These temp files may not ever get saved permanently to the page if Page Edit is abandoned before saving.

The issue is that these temp files can be included in the value of a Files/Images field when output formatting is off - either for the whole page or by getting the field value with getUnformatted().

In the screencast below the issue is demonstrated by dumping the count of an images field after some images are uploaded but the page is not saved.

images

Expected behavior

If temp files are not considered valid for the formatted value of a field they shouldn't be valid for the unformatted value either. If there needs to be a way to get the value of a field including temp files maybe this needs some dedicated option/argument so they are not included unintentionally.

Setup/Environment

  • ProcessWire version: 3.0.127
@ryancramerdesign

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@Toutouwai This is exactly how temp files are designed to work, this is an intended behavior.

@Toutouwai

This comment has been minimized.

Copy link
Author

commented Mar 5, 2019

@ryancramerdesign, I guess it just isn't intuitive to me that output formatting would have an impact on which files are considered to belong to a field and so getting these temporary files in the field value is unexpected (for me at least).

In the past when looping over fields in a template I have used getUnformatted() to simplify dealing with Files/Images fields so that their value is always a WireArray regardless of if they are configured as single or multiple value fields. It sounds like this is not a wise thing to do if it potentially adds unexpected files into the field value. So you could say this is just a mistake on my part, but I just wonder if other people could be doing similar things and getting caught out because it doesn't occur to them that the unformatted value includes unsaved temp files.

I was wondering if some more explicit option could be used in places where the core needs to include the temp files in the field value. So that FieldtypeFile::wakeupValue could include some line to the effect of:

if(!$options['includeTemp'] && $pagefile->isTemp()) continue;
@szabeszg

This comment has been minimized.

Copy link

commented Mar 6, 2019

so that their value is always a WireArray regardless of if they are configured as single or multiple value fields.

My sites all rely on this technique, so as far as I can see these temp images could break them all? Luckily I have not yet run into this particular issue but this is because I manage the images of all those site but this will soon change on some sites of mine where editors will not be able to deal with such issues, of course. I think providing the same set of images in both cases is naturally expected no matter what.

I was wondering if some more explicit option could be used

I also have the feeling that the current solution of handling these temporary states is just not robust enough.

@ryancramerdesign

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

This is part of the purpose of having separate unformatted and formatted states, as the unformatted value represents the backend value where things are manipulated, and the formatted value represents the frontend value where things are output. When it comes to files fields, skipping over temporary files not yet confirmed is an output formatting function. However, if you are working with unformatted files fields and want to skip over any potentially temporary files that might be present, you'd want to call that Pagefile::isTemp() method to check on them before using them in any output. Though for any kind of front-end use I'd suggest using the formatted value instead, which will take care of it for you.

@szabeszg

This comment has been minimized.

Copy link

commented Mar 8, 2019

Thanks for the explanation, Ryan! Using Pagefile::isTemp() sounds like an easy fix so I will add it to the hook method I use. Still, would it be possible to add this important bit of information to the API docs? Sure, one can even miss it there but at lease there is some chance of being read.

@teppokoivula

This comment has been minimized.

Copy link

commented Mar 8, 2019

+1 for mentioning temporary items being present in the docs when discussing output formatting. Note that I didn't check yet if it already is mentioned, but if it isn't, it definitely should be.

I get the reasoning there, but I can also see how folks may assume that OF affects just the formatting (i.e. things like textformatters being involved, escaping stuff, and so on), not how many / which items are included – if you get my point.

TL;DR: It's fine that it works like this, but it should be documented properly :)

@netcarver netcarver added the type: docs label Mar 8, 2019

@Toutouwai

This comment has been minimized.

Copy link
Author

commented Mar 9, 2019

Agree with the documentation suggestions - where were you guys thinking the documentation should go? Added to Pagefiles? Or Page::of()?

@ryancramerdesign, I think the community would benefit from an article explaining Output Formatting from the ground up - the rationale behind it, how it relates to setting values to a page, things to watch out for when getting an unformatted value versus a formatted value. A page within the documentation would be ideal, or alternatively a blog post.

Even folks who have been using PW for years have questions regarding output formatting - some discussion in the last three pages of this thread: https://processwire.com/talk/topic/20596-new-post-new-pw-website-ready/?do=findComment&comment=179388

@szabeszg

This comment has been minimized.

Copy link

commented Mar 9, 2019

A page within the documentation would be ideal

How about an article for experienced PW devs as an additional one at https://processwire.com/docs/

Added to Pagefiles? Or Page::of()?

Maybe both with relevant short tips and pointers to each other and the article I referred to above.

Toutouwai added a commit to Toutouwai/FieldtypeSelectImage that referenced this issue May 9, 2019

v0.2.2
Skip temp images when getting image field values: see processwire/processwire-issues#825
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.