Settings passed as wrong type [#945084]

I discovered a number of issues when trying to use Views Slideshow: Galleria's interface to Galleria's extend option. See #936480: Some Galleria options expect JavaScript functions, Drupal.settings gives string for more information. One of these issues is that some options are not passed in the correct type. I'm purposely ignoring the options that are supposed to be output as JavaScript functions: I'll deal with these in another issue as they are more complex.

A list of the options that I am aware are not outputting with the correct data type are:

debug (true works, but false is output as 0)
image_crop (true works, but false is output as 0)
keep_source (true works, but false is output as 0)
popup_links (true works, but false is output as 0)
preload (all is output as 0)
thumbnails (both true and false are output as integers)
thumb_quality (both true and false are output as integers)

All of these can be demonstrated by following the steps to reproduce in #936480-3: Some Galleria options expect JavaScript functions, Drupal.settings gives string and changing that particular option in step 10.3.

Comment	File	Size	Author
#6	views_slideshow_galleria-945084.patch	11.54 KB	mparker17
#4	views_slideshow_galleria-945084.patch	6.68 KB	mparker17
#3	views_slideshow_galleria-945084.patch	6.68 KB	mparker17

Comments

Comment #1

mparker17

he/him

English

UTC-5

commented 18 October 2010 at 18:00

Inspection of the code shows that there are conditional statements around certain variables to ensure that they are output only when they differ from the default. However, these conditional statements don't seem to work — they all output 0 when the current setting matches the default, which results in a number "0" being output in the settings array, rather than not outputting that variable altogether (which would seem to be what the author intended).

While I totally like the idea of keeping things clean; I can see a potential maintenance problem with this method of doing things: we're making assumptions about what the defaults are. If Galleria's defaults are changed in a future version of Galleria, this module will start exhibiting unexpected behaviour.

For example, if the Galleria maintainers decide that a default of keep_source: true would be easier for developers to understand, the current code will not output false, even if that's what a particular developer wants. Someone would need to change the module.

Also note that this module doesn't enforce which version of Galleria is being used. In my fictional example, if someone updated the module to only output keep_source: false (in reaction to the change to galleria) users who did not update Galleria would start to get unexpected results. In other words, the only way to update the module without side-effects would be to force it to output both keep_source: true and keep_source: false — in other words, we'd have to drop the conditionals anyway.

I want to argue that this is a good reason to drop the conditionals for (at least) the simple options (simple options = ones that don't expect an array or function).