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

Item added to Repeater field despite page not being saved #760

Closed
Toutouwai opened this issue Dec 6, 2018 · 8 comments
Closed

Item added to Repeater field despite page not being saved #760

Toutouwai opened this issue Dec 6, 2018 · 8 comments

Comments

@Toutouwai
Copy link

Toutouwai commented Dec 6, 2018

Short description of the issue

If a new item is added to a Repeater field and then the page is not saved (either by browser refresh or abandoning of Page Edit), that item persists as an unpublished page in the field value. The unpublished page is invisible in the Repeater inputfield.

Steps to reproduce the issue

  1. Add a new item to an empty Repeater field.
  2. Do not save the page but just refresh Page Edit.
  3. Check the unformatted value of the Repeater field.

Screencast

In the screencast below, the field is genuinely empty to begin with, and the count of the unformatted value is 0 as expected. After an item is added and Page Edit refreshed without saving, the Repeater appears to be empty but actually contains an invisible unpublished item. The count of the unformatted value is now 1.

rep

Expected behavior

I expected any new, unpublished items in a Repeater field not to persist in the field value unless the page/field is saved. And if a Repeater does contain an unpublished item I expected that to be visible in some way within the Repeater inputfield - otherwise there is no clue within Page Edit that the item exists and it's difficult to distinguish genuinely empty Repeater fields from those containing unpublished items.

Setup/Environment

  • ProcessWire version: 3.0.120
@adrianbj
Copy link

adrianbj commented Dec 6, 2018

I expected any new, unpublished items in a Repeater field not to persist in the field value unless the page/field is saved.

I completely agree with your analysis here - this is confusing for sure. It's not quite the same thing, but an uploaded image is also kept even if the page isn't saved after upload. The difference with images is that they remain visible in the admin and are effectively automatically published. I think there was a discussion about this somewhere, but can't find it right now.

Back to the repeater issue at hand - if there is going to be something done on this front, it might be time to revisit this issue: #36 - those bugs regarding addStatus and removeStatus really should be dealt with.

@ryancramerdesign
Copy link
Member

When you add a repeater item, it has to create a new page to accommodate the item. ProcessWire doesn't support editing of pages without an ID whether in repeaters our on their own. That's because some Fieldtypes require a page ID in order to operate.

When it comes to repeaters, ProcessWire will often maintain one or more "ready" pages for the repeater. This is basically a placeholder for a page that can be edited, when/if one is added. If you add a repeater item, but don't save it, it remains as a ready page and will be used the next time you want to add an item to the repeater. This is an intended behavior, we don't like to waste page IDs.

It's not quite the same thing, but an uploaded image is also kept even if the page isn't saved after upload. The difference with images is that they remain visible in the admin and are effectively automatically published.

This is not the case unless you've specifically enabled the "Overwrite existing files option", which also outlines the behavior in the field description. Most of the time you would not have this option enabled. Other than that option, when you upload an image or file, it has what's called "temp" status. The temp status is removed once the page is saved with the image file on it. An image/file uploaded on a page that isn't followed with a save, is removed automatically. Unlike with repeaters, there's nothing that can be re-used later with an image that wasn't saved, so it can only be deleted.

@adrianbj
Copy link

adrianbj commented Dec 7, 2018

This is not the case unless you've specifically enabled the "Overwrite existing files option", which also outlines the behavior in the field description. Most of the time you would not have this option enabled. Other than that option, when you upload an image or file, it has what's called "temp" status. The temp status is removed once the page is saved with the image file on it. An image/file uploaded on a page that isn't followed with a save, is removed automatically.

Turns out it was my Custom Upload Names module that was causing the problem here. When it uses $pagefile->rename() that removes the "temp" status. Do you think this is something that should be changed in the core rename method? Otherwise I think I would need to hack something into my module. BTW is the temp status just the setting of created to 1969-12-31 16:00:10 or is there something more to it?

@Toutouwai
Copy link
Author

Toutouwai commented Dec 7, 2018

Thanks for the explanation Ryan. I can understand why this is handled the way that it is, but I still think it's a bit of a problem.

When dealing with a fieldtype that has a WireArray value I think the standard way to identify an empty field is via count. So it would be very common for devs to distinguish between empty and populated Repeater fields with a condition like:

if($page->repeater->count) {
    // This field is populated...
}

If this is done in a context where output formatting is off then a repeater field that contains only an invisible "ready" item is going to be treated as if it were populated when for all intents and purposes it should be considered empty. So if we can say that virtually nobody wants to treat a repeater that contains only ready items as populated then there needs to be some easy way to check for empty repeaters that excludes ready items, and devs need to know about this and use it as the standard test. So ideally it should be something obvious and transparent (for instance, not something that requires an understanding of bitwise flags).

What do you consider to be the right test to identify empty repeater fields, including those that contain only "ready" items? And could this test be implemented behind the scenes for repeaters, so properties/methods such as count will automatically exclude ready items unless specifically told to include them?

Edit: there is also the use of count in PageFinder selectors to consider. For example, how will "ready" items affect attempts to find pages with an unpopulated repeater field:

$unpopulated = $pages->find("repeater.count=0");

@adrianbj
Copy link

adrianbj commented Dec 8, 2018

I know I am way off topic here now, but just wanted to close out my comparison with this and images by saying that I fixed the issue in Custom Upload Names by doing $pagefile->isTemp(true); after $pagefile->rename

@netcarver
Copy link
Collaborator

@ryancramerdesign Ryan, bumping this to see if you have any further thoughts on Robin's comments above? Is this issue a candidate for closure soon?

@ryancramerdesign
Copy link
Member

ryancramerdesign commented Mar 8, 2019

@Toutouwai @netcarver As for an alternate to $page->repeater->count() when dealing with an unformatted page, I would suggest doing a $page->getFormatted('repeater')->count() to get the count of published items in the repeater.

As for a page finding selector, it looks to me like $pages->find("repeater.count=0"); should work, as there is literally a "count" property on repeater fields, and it stores the count of published, non-hidden items in the repeater. Though because it's a literal property in the DB table (and repeaters do not consume more than one row in its DB table since they are not a FieldtypeMulti instance), it's possible there might need to be a row present in order to match that selector, so you might instead use something even simpler: a $pages->find('repeater=') selector, which should match both the situation of there being no rows and there being a row with an empty data column. I don't think it would be necessary but you could also try (repeater=''), (repeater.count=0) if for some reason the above does not work in your particular case.

@Toutouwai
Copy link
Author

Thanks @ryancramerdesign, $pages->find("repeater.count=0"); does work as expected, even for pages with repeaters containing "ready" items.

Regarding the unformatted value, I understand now that this is another case like File/Image fields where the actual contents of the field (not just textformatters, etc) can be different depending on output formatting state. Now I know this it's fine, I just think it needs to be documented somewhere and hopefully covered in a blog post explaining output formatting (as I suggested in another thread today).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants