Step-through debug of a clean unconfigured site running from git:-

Unconfigured server (tropo/twillio) configuration form, calls VoipVoice::getDefaultVoice()->getVoiceId().

In getDefaultVoice() there is no variable_get('voipcall_voice_id', ''); set. So $voipvoice_voice = self::getVoice($voice_id); get's called with an empty arg. So getVoice gets called and 

 83     else if (empty($voice_id)) {
 84       $voice_id = variable_get('voipcall_voice_id', ''); # no variable set $voice_id = ''
 85     }
 87     $voipvoices = self::getVoices()
/**
Array
(
    [voip_log] => Array
        (
            [language] => en
            [voice] => man
            [description] => Log - VoIP Drupal Log voice
        )

)
**/
 88     $voipvoice = $voipvoices[$voice_id]; # not set
 89     if (empty($voipvoice)) { #empty
 90       //If wrong voice_id
 91       watchdog('voipcall', 'VoipVoice::getVoice called with non-existing id: '. $voice_id, NULL, WATCHDOG_ERROR);
 92       //Load default voice instead
 93       $voipvoice = self::getDefaultVoice(); # hence calls getDefaultVoice() again and we loop.
 94     }

Seems to be an issue for configurations as above since beta7.

Will just a set of defaults in the variables solve it as desired?

CommentFileSizeAuthor
#8 voipvoice.inc_.patch841 bytessrevilak

Comments

tamerzg’s picture

tamerzg commented
Status: Active » Fixed

variable_get('voipcall_voice_id', '');

was unintentionally left empty it should of been:

variable_get('voipcall_voice_id', 'log');

We just fixed it in latest dev.

ekes’s picture

ekes commented
Status: Fixed » Needs work

VoipTropo doesn't return [log] as a key to the hook_getvoices() (by the way shouldn't that be in namespace?); so running 6.x-1.x from git at the moment I'm still getting the loop. May the servers could offer up a [voipdrupal_default] or something to make it easier?

tamerzg’s picture

tamerzg commented
Status: Needs work » Fixed

You will need to reinstall the voipcall module and clear the caches in order for variable 'voipcall_voice_id' to get deleted, as with previous version it might get stuck with empty value so that would cause the loop again.
It doesn't matter if Tropo or any other server doesn't return log as a key, because in first install value of voipcall_voice_id variable is not set so it will get it from default value provided in variable_get() function.

ekes’s picture

ekes commented

Yes if I have no voipcall_voice_id variable set then now 'log' is used as the default. This happens when you first set the Server to Tropo. But voiptropo_getvoices gets called and doesn't return anything keyed as 'log'. Hence the loop.

ben.bunk’s picture

ben.bunk commented
Status: Fixed » Needs work

I'm also experiencing this. If you set Tropo as the default server before configuring it you'll still get the error. It might not make a lot of sense to enable the server before it's configured but I do it myself and I think many others will do it as well.

It looks like if the log server is going to be the default voice for all servers then it's hook_getvoices() needs to be modified to always return a voice - this is what I came up with: 

function voip_getvoices($language, $gender, $is_local) {
  $voice = new VoipVoice('log', 'man', 'en');
  $voices['log'] = $voice;
  return $voices;
}

Alternatively, you could also disable the radio buttons for all servers except the log server so that the user can still click on the configure link but they can't enable the server yet - 

if (variable_get('voipcall_voice_id', 'log') === 'log') {
  // Disable radios
}
leoburd’s picture

leoburd commented

- Added benbunk fix to voip_getvoices().

That seems to work for me. Does that solve the problem for everyone else?

barrett’s picture

barrett commented

The solution in #5 appears to have worked for me as well.

srevilak’s picture

srevilak commented
StatusFileSize
new voipvoice.inc_.patch841 bytes

I'd like to submit a patch that works around the infinite loop.

The patch takes the first element from getVoices(), and uses that for the default voice_id.

freescholar’s picture

freescholar commented

The patch in post #8 works for me!
Thanks srevilak!

leoburd’s picture

leoburd commented

Hi guys,

If the goal is to provide a fallback default value for getDefaultVoice(), I don't see what the proposed patch adds to what we already had in #6. Would anyone clarify that for me?

Thanks,

.L.

freescholar’s picture

freescholar commented

One benefit I see is that the patch uses the first voice listed in the script as the default. This removes the confusion around the voice named 'log' - as a newbie php programmer, it was not initially evident that 'log' was a voice name.

Is there a significance to the user named 'log'? Does it need to be set as the default voice for some reason?

I didn't try the solution of blocking the radio buttons, because it removes a bit of flexibility on the admin's part.

ben.bunk’s picture

ben.bunk commented

The patch in #8 fails on a clean install.

The reason #8 fails is because on a clean install there aren't any default voices to choose from in the list. For patch #8 to work correctly it would need to make significant assumptions about the voice configurations and requires changes to all of the voip provider modules. The patch in #5 assumes the log server is the default which has no voice and eliminates a lot of code changes.

freescholar’s picture

freescholar commented

Thanks for the clarity - I will try the patch in #5 again and let you know how it goes!
I am new to "patching" and may have applied it incorrectly on first attempt.

leoburd’s picture

leoburd commented
Status: Needs work » Needs review

Hello all,

Would you mind testing the new dev version? The issue reported above prompted us to significantly change the way voices are handled by the VoIP servers in the system. We believe that the bug is now solved. What do you think?

.L.

freescholar’s picture

freescholar commented

The dev version works great - no sign of the bug.
6.x-1.x-dev tar.gz (120.47 KB) | zip (142.99 KB) 2012-Mar-03
I installed it on a fresh installation of D6 without any issues!
I also updated a D6 site that did have the bug and it is now working as planned.
Thanks Leo!!!

leoburd’s picture

leoburd commented
Status: Needs review » Closed (fixed)

Glad that this is solved!

leoburd’s picture

leoburd commented
Issue summary: View changes

Done the clean install; it's the same.