Enhance compatibility with ajax handling. [#2038051]

Problem/Motivation

Currently it can happen that the JS settings for the optionsets are messed up if ajax_command_settings() is used with merge enabled. Instead of having the single options in the optionset the options are converted to arrays what makes flexslider very unhappy.
Further we should figure out a way to make the instance id's unique over multiple requests.
Currently a static variable is used to increase the id within the same request. As this starts over within an ajax request we can end up with two flexslider-1 items on the same page.

Proposed resolution

I've used the already prepared cache to set the optionset only once within the same request - that prevents the merge issue.
Related to the id issue, the best solution I could think of was to add the REQUEST_TIME to the id. I think this should work fine even with caching. A possible issue could be that a normal browser request and the ajax request happen in exact the same second, what again would cause colliding id's.
How likely is that?
To reduce the probability of such a collision we could switch to microsecond(TRUE) instead REQUEST_TIME.

Remaining tasks

Reviews needed

User interface changes

None

API changes

No real API changes, but the format of the id's has changed. If someone has custom code that deals with them it's likely that this will break.

Comment	File	Size	Author
#2	flexslider-enhancements-for-ajax-2038051-2.patch	1.01 KB	das-peter
#2
	flexslider-enhancements-for-ajax.patch	2.14 KB	das-peter

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

minorOffense CreditAttribution: minorOffense commented 12 August 2013 at 14:27

For ajax pages, having a custom ID generated vs having the user configure a static ID is something we'll have to mull over.

Comment #2

das-peter CreditAttribution: das-peter commented 5 January 2014 at 11:41

Issue summary:

View changes

File	Size
flexslider-enhancements-for-ajax-2038051-2.patch	1.01 KB

Just struggled again over this. Re-rolled the patch.
To avoid creating "unpredictable" id's we could change to an approach where we use an unique css class (based on the id) to do the initialization (_flexslider_init()).
That way we could ensure that the initialization works even if an id is set manually and the code isn't aware of the ajax issue.

Comment #3

minorOffense CreditAttribution: minorOffense commented 22 January 2014 at 14:52

Status:

Needs review

» Needs work

That won't work. You could have two requests come in at the same time once again giving you your ID conflict.

Not to mention it would make theming a specific flexslider instance pretty well impossible by ID (even though you aren't really supposed to use ids for theming people still do).

Comment #4

minorOffense CreditAttribution: minorOffense commented 22 January 2014 at 14:52

The cache patch seems ok though. I'd be willing to put that in.

Comment #5

das-peter CreditAttribution: das-peter commented 22 January 2014 at 20:01

The topic of collision was already mentioned in the summary:

I think this should work fine even with caching. A possible issue could be that a normal browser request and the ajax request happen in exact the same second, what again would cause colliding id's.
How likely is that?
To reduce the probability of such a collision we could switch to microsecond(TRUE) instead REQUEST_TIME.

And I've tried to outline another approach to tackle the issue in #2:

To avoid creating "unpredictable" id's we could change to an approach where we use an unique css class (based on the id) to do the initialization (_flexslider_init()).
That way we could ensure that the initialization works even if an id is set manually and the code isn't aware of the ajax issue.

If we change to this approach we could use random id's I think.
However, before I start working towards another solution I'd like to have feedback first.