Description: If an editable field widget is used to change a field's value while using Firefox (cycling through submission and callback), and subsequently changed in another environment (a different computer or browser), then refreshing the initial Firefox page incorrectly continues to show the values as last set when using that browser rather than the new values (which are confirmed as being stored in that fields database table after changing them in the second environment).
Proposed solution: attach "drupal_add_http_header" as a callback function to the form with more aggressively specified anti-cache directives. See forthcoming patch from _vid for details.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | editablefields-reload-entity-1784160-15.patch | 829 bytes | _vid |
| #10 | editablefields-reload-entity-1784160-10.patch | 852 bytes | megachriz |
| #1 | editablefields.module-browser_side_caching_ajax_values_causes_inaccurate_display-1784160-1.patch | 930 bytes | _vid |
Comments
Comment #1
_vid commentedThe referenced patch is attached.
We added the drupal_add_http_header to the form #attached call back array.
Here's the code:
Thanks to
http://treehouseagency.com/blog/brian-mcmurray/2012/03/01/add-http-heade...
and
http://stackoverflow.com/questions/1971721/how-to-use-http-cache-headers...
Comment #2
dags commentedI tested this on the dev branch and saw the issue occurring for anonymous users and the proposed patch fixes it. Committed to 7.x-1.x.
Comment #3
dags commentedComment #4
dags commentedComment #5
quicksketchI'm requesting this patch be rolled back, or at least you should strongly warn users that adding any editable fields to the front-end display of their nodes will disable ALL PAGE CACHING, including Varnish, Akamai, and the Drupal page cache. As noted in the Treehouse article, you might want to take this approach when displaying a CAPTCHA, however CAPTCHA module explicitly states it disables page caching if you show the captcha to anonymous users.
Firefox specifically provides a feature that keeps current field values when you reload the page via the reload button or command+R, so what's being described here may simply be a browser behavior. If you select the address in the address bar then hit enter to reload the page, Firefox reloads all the fields to use their defaults.
If the behavior being described is simply the Drupal page cache, I would consider this to be expected behavior since all the content on anonymous pages is delayed for the length of the page cache lifetime.
Comment #6
_vid commentedHi quicksketch,
Thanks for catching this.
The goal of the patch was to disable the caching on nodes that contain active editable fields and only those nodes.
After running some tests it appears that the cache has indeed been disabled on every node.
We we're using any caching on our site. So to test this I enabled all drupal caching from the performance page and ran drush cron -v. I'm not sure if that's a sufficient way to test this. But I am seeing the no-cache header on every page.
So it appears that updating $form['#attached']['drupal_add_http_header'] affects every node.
I made the assumption that we were only altering the $form array on editablefields enabled nodes but I don't see evidence of that now.
Looking closer at the Treehouse example, there is a more qualified $form array in use, so perhaps that change would ensure that we're only affecting nodes that use editablefields, like so:
I'll test that now.
If that is the case, I wonder if other code in the editablefields_form() function might benefit from being inside the more qualified $form array. At least the other #attached var:
$form['#attached']['library'][] = array('editablefields', 'editablefields');and maybe the #attributes one as well:
$form['#attributes'] = array('class' => 'editable-field');Comment #7
_vid commentedI've commented out the change on my dev site, cleared the cache, ran cron and I'm still seeing the no-cache header. So I need to do more investigation to see if it's being called from somewhere else as well before my test results can be accurate.
Comment #8
quicksketchThanks @_vid. I doubt the change would affect every page of the site, unless there was some node with an editable field being displayed somewhere on the page. Things in the #attached array only get added to the page when drupal_render() is run on them, so that shouldn't be the source of the problem.
I think generally I'm just saying the page cache should be kept in all situations, even if displaying an editable field with potentially stale data. The fewer ways end-user can shoot themselves in the performance foot the better. If the staleness is a problem, they should look at any number of cache-busting modules out there http://drupal.org/project/cache_control, http://drupal.org/project/purge, etc. The entire site crashing because caching isn't working isn't a fair tradeoff for stale data (which is expected to some extent on all anonymous pages).
Comment #9
dags commentedOops. I guess I was a little premature on that one. Thanks quicksketch, and _vid for your input. The commit has been reverted.
Comment #10
megachrizI got a similar issue, and maybe the solution to that is equal to the solution to this issue.
I have a list of nodes in a View where one of the fields is an editable field. When I take the following steps:
This results in that the state of the complete node gets reverted to the state it had when the View was loaded. Any changes that are done in the other browserwindow are undone.
My use case: One of the fields of the node may be edited in the View. I have an edit link in the View, so the user can fill in additional values of the node. A form for the additional fields will be displayed in a popup (made with Ctools modal).
My solution: when the editablefields form gets submitted, don't retrieve the entity from the cache, but reload it (but only if it has an ID).
Comment #11
jgraham commentedThis sounds pretty similar to the patch in #1719824: $entity loaded from form_state is potentially stale in editable_fields_form_submit() perhaps these are the same issue? Although I was able to trigger the issue in the same browser/environment.
Perhaps this or #1719824: $entity loaded from form_state is potentially stale in editable_fields_form_submit() should be closed as duplicate?
Comment #12
megachrizYes, the patch in #1719824: $entity loaded from form_state is potentially stale in editable_fields_form_submit() is even about the same as the patch posted in #10, except that the patch in #10 checks if the entity ID is not empty.
Maybe the initial problem described here is different, and then the patch in #10 should be posted in the other issue, but I'm not sure. I would like some feedback from _vid or quicksketch about this before we close one of the issues as a duplicate.
Comment #13
_vid commentedAfter reviewing the issues, I don't think this one is a duplicate.
@joseph.muennich and I did experience the same issue that @jgraham and @MegaChriz describe (I'm following that issue now) but beyond that, as @quicksketch helped clarify:
That is the problem that we're trying to combat and differentiates the issues for me.
My attempt at cache-control seemingly solved both issues at once; though after @quicksketch's feedback I'm looking at new ways to approach this.
It would be interesting to see if the patch above (#10) addresses the FF issue. Though I think it unlikely.
In that case, I'd recommend moving the patch #10 to the other issue.
Comment #14
_vid commentedHi all, @joseph.muennich and I are back on this issue for another site using editable fields and my previous no-cache fix didn't do the trick so I applied @MegaChriz's patch #10 and it appears to be working great now. We'll do some more testing after lunch and if all goes well, I'd like to move the status to RTBC.
Comment #15
_vid commentedUpdate; I wasn't able to apply @MegaChriz's #10 patch properly to 7.x-1.0-alpha2. So I made the changes by hand and re-rolled the patch for 7.x-1.0-alpha2. I changed the version of the issue to 7.x-1.0-alpha2 incase the test-bot might come through and try to apply the patch.
Comment #16
john franklin commentedIt looks like a similar patch has already been committed (commit 94aefbe8) for #1405854: Multiple fields not saved.
Comment #17
joelpittetIsn't the
$element['#entity_id']always known if it's being used as a key?Comment #18
megachriz@joelpittet
No, if you pass an entity that is not yet saved to
editablefields_form(), then$element['#entity_id']will be empty. I did that in a custom module so I could "edit" fields for entities that did not exist yet. Use case: I had created a view of question nodes which could be answered on the page itself. So for each question there was a form for an answer entity. I only needed a single field of the answer entity in this context, so I used editablefields for that. But when loading the questions page, some answer entities may exist and some don't. So I needed be able to load an editablefield for an entity that did not exist yet. In these cases$element['#entity_id']was empty.The issue here is that the entity could get out of date and would need to be reloaded. But reloading can only be done if there is an entity ID and there isn't one if you pass an unsaved entity to
editablefields_form().Comment #19
joelpittetJust looking at the value of the else in the patch, the last key is an empty #entity_id in element.
If that were ever TRUE, you'd get an undefined index
Example code to show what I mean:
https://3v4l.org/u5cNk
Comment #20
megachrizHm, I agree that indeed looks weird. But for some reason, the site that I had implemented this on, never reported a undefined index in the logs for this line. Probably because of the following code in
editablefields_form():If a new entity is passed,
$entitywould just be in$form_state['editablefields_entities'][$entity_type][NULL]. That's weird, but apparently it worked.Comment #21
megachrizOr maybe the entity that I passed had
0as an ID? It's been four years since I wrote that custom module...Comment #22
joelpittetI've committed something that does similar to this patch. Closing this because it should be resolved.
Comment #23
jweirather commentedWas there an official resolution to this issue? We are seeing what appears to be a very closely related issue using 7.x-1.x-alpha4.
In Firefox only (apparently) we have a views table with several editable fields in each row. If an editable field is changed on any row, and the page refreshed, the editable fields are not updated properly.
In particular, this is on a ticket system, where users can update the status of tickets using editable fields. If a user updates a ticket to "closed", it should disappear from the page upon reload. In Firefox, after updating a ticket to "closed" and reloading the page, the ticket itself IS disappearing from the view, but the form controls do not update... Meaning the textual content for the tickets shifts as expected, but the form controls for the editable fields do not update with the textual content.
It appears to work properly in Chrome and Edge.
I've tried applying the patch to no avail. Any pointers are appreciated!
Edit: Noting that navigating to a different page and back again appears to update the fields properly.