getVariable can fail

[EricTheMagician]

Problem statement

Bug Report:
In the getVariable function of variable.js (

blockly/core/variables.js

Line 533 in 09853c3

Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) {

),

If an id is specified, but a variable is not found, it does not fallback to the opt_name + opt_type.

Expected Behavior

Return the variable by opt_name + opt_type if they are specified

Actual Behavior

Returns undefined.

Steps to Reproduce

I don't know what's the simplest way for someone else to reproduce it, but this what we do in our setup.

1. In our toolbox, we have a block that has a FieldVariable dropdown default with name "aChannel" and type orsChannel.
2. We drag the block from the toolbox and drop it in to the main workspace.
3. We save the xml representation of the workspace.
4. We reload blockly and clear the browser cache (for development purposes), though I think clearing the blockly workspace is sufficient.
5. We try to load the workspace from the xml representation in step 3.
   This generates the stack trace below.

Essentially, the getOrCreateVariablePackage calls getVariable by id, name and type.
Here, because of the toolbox, we have a variable called "aChannel", but it has a different id then the one we saved. Thus, when getVariable is called, by id, it doesn't find the one created by the toolbox. So it tries to create a new variable, but it then fails because, it technically already exists.

Stack Traces

Uncaught (in promise) Error: Variable "aChannel" is already in use and its id is "^%=4elLAJI}I@xY|apaL" which conflicts with the passed in id, "K$7zZt~4c?ZYE-d6~*9-".
    at Blockly.VariableMap.createVariable (variable_map.js:177)
    at Blockly.WorkspaceSvg.Blockly.Workspace.createVariable (workspace.js:321)
    at Blockly.WorkspaceSvg.createVariable (workspace_svg.js:1094)
    at Object.Blockly.Variables.createVariable_ (variables.js:557)
    at Object.Blockly.Variables.getOrCreateVariablePackage (variables.js:490)
    at Object.Blockly.Xml.domToFieldVariable_ (xml.js:809)
    at Object.Blockly.Xml.domToField_ (xml.js:841)
    at Object.Blockly.Xml.domToBlockHeadless_ (xml.js:699)
    at Object.Blockly.Xml.domToBlockHeadless_ (xml.js:713)
    at Object.Blockly.Xml.domToBlockHeadless_ (xml.js:713)

Operating System and Browser

• Desktop Chrome

Additional Information

I can provide a PR, but it's not clear how to format it with respect to the style guide.

[rachel-fenichel]

Interesting. So you want the else if (opt_name) to become an if. The if (id) part would run, and return if it found anything, and then fall through to the other path on failure. Is that accurate?

P.S. Sorry for the delay in responding, and thanks for the clear write-up.

[EricTheMagician]

@rachel-fenichel
Yes, exactly.

[EricTheMagician]

@rachel-fenichel
Thanks!