Closed (fixed)
Project:
Ubercart
Version:
6.x-2.7
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
31 Dec 2008 at 19:46 UTC
Updated:
30 Jul 2013 at 17:21 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
cha0s commentedWoot! I picked this one up again cause I got an inspiration from doing some other code with $form_state['storage']. I thought that'd solve the problem, but it didn't.
Turns out my code before just wasn't rendering the hidden form element on the edit page, so it wasn't getting that info on the rebuild, always using time(), so it wouldn't catch the errors. Patch to test follows. =)
Comment #2
cha0s commentedEr, oooops, I left in a change that I made while testing, hehe...
On a side note, do you think we should just do a drupal_render($form) on the end of the specific form elements we want to render. It could have saved me a lot of time here, and also that order_uid element never even gets rendered (which is why I was messing with it, tmo see what'd happen (nothing, it's dead code as far as I can tell)).
Comment #3
Island Usurper commentedYou could make it #type => 'value', so it doesn't need to be rendered. But it is odd that there isn't a drupal_render($form) call at the end. It's probably to keep a bunch of stuff from spilling off the end of the form outside of the panes. We can make another issue for it if.
Comment #4
cha0s commentedI don't think it can be changed to work without rendering, actually. Passing the client the time is the only way I could find to be able to tell when the client actually loaded the form originally, to know if it was before or after the latest order modification.
If you know of another way that works, you should submit another patch...
FWIW, adding the render($form) does add a lot of garbage... yeesh! Should we be using ['#access'] to turn that stuff off? (Wondering before I start on any patch in that direction)
Comment #5
Island Usurper commentedDoing it this way is probably the simplest. It does the job without too many headaches, so it's committed.
Comment #7
Anonymous (not verified) commentedHi, my Drupal installation run on a server cluster.
Unfortunatly the timestamp of the different server are not sincronized.
This cause a validation problem because the rendered time may be higher of the "order modified time" time.
I solved this catching the order last modified date in the form when i render the page instead of the time().
The result is the same and catching the timestamp from the db maybe it's most secure.
Can you commit this patch?
Comment #9
Island Usurper commentedI feel like this is just one of many possible problems that can arise from not having clustered servers with synchronized times. Off the top of my head, cache invalidation becomes an even trickier problem, and even $order->modified can't be trusted because you don't know which server it came from.
There's no excuse for a web server not to have access to any number of time synchronization services.
Comment #10
Anonymous (not verified) commentedSure, however $order->modified comes from the DB and so it is a fixed and secure value to use during the validation.
We can exclude a possible problem from the module and make a better validation just committing this code.
Comment #11
longwaveYou cannot set your own patches to RTBC. I am leaning towards agreeing with Island Usurper, though; if your server times are far enough out that this is causing issues, that is *your* problem to solve.
Comment #12
Anonymous (not verified) commentedYES, THE TIMES ON MY SERVER ARE A PROBLEM ONLY FOR ME.
That's clear.
Can we agree on the fact that control the "last modified order time" is better than control a time() value??
Comment #13
Island Usurper commentedNo, I don't agree with that. I think they're functionally equal in this situation, since the value is stored in the form when it is built and then compared to $order->modified during validation. As long as the stored value is less than the actual modified time, then you can make changes, and it doesn't matter what the value is. So it's easier to not change the code.
Comment #14
longwaveReviewed this again, and $order->modified is just as valid, if not moreso, than time() to check whether the order was modified. Committed to both branches.