Widget##getVariable inconsistent with Widget##hasVariable

[flibbles]

Hello,

Small possible issue here.

Widget##hasVariable checks for variables by first looking in its own variable member before iterating up its parent chain. This makes sense to me.

Widget##getVariable looks for a variable in its parentWidget and relies on the javascript prototype mechanism to search all ancestors, like in so:

Widget.prototype.getVariable = function(name,options) {
	. . .
	parentWidget = this.parentWidget;
	// Check for the variable defined in the parent widget (or an ancestor in the prototype chain)
	if(parentWidget && name in parentWidget.variables) {
		var variable = parentWidget.variables[name],
			value = variable.value;
		. . .
	}
	. . .
};

Sure, but why is it starting on its parentWidget and not itself? It results in a case where hasWidget can return true if a variable exists in the called widget, but can't return that variable through getWidget.

This issue has no effect on anyone who's simply using Tiddlywiki, since no widgets rely on variables that they themselves defined, but it does come up for people who design plugins, and testing environments for those plugins (like me).

Wouldn't a more logical implementation be something like:

Widget.prototype.getVariable = function(name,options) {
	options = options || {};
	var actualParams = options.params || [];
	// Check for the variable defined in this widget (or an ancestor in the prototype chain)
	if(name in this.variables) {
		var variable = this.variables[name],
			value = variable.value;
		// Substitute any parameters specified in the definition
		value = this.substituteVariableParameters(value,variable.params,actualParams);
		value = this.substituteVariableReferences(value);
		return value;
	}
	// If the variable doesn't exist in the parent widget then look for a macro module
	return this.evaluateMacroModule(name,actualParams,options.defaultValue);
};

It this change is acceptable, and there isn't some reason for the current method that I'm unaware of, I can create a pull request with the change (and test it too).

Thank you, Flibbles

[Jermolene]

Hi @flibbles apologies for the delayed response.

It's actually hasVariable() that is wrong. Originally, both getVariable() and hasVariable() both started looking in their own variables, and then up the ancestor stack. I changed getVariable() in order to permit widgets to be able to (e.g.) change the currentTiddler value while still referencing it in attributes etc. and getting the parent value. As far as I can see, I failed to change hasVariable() in the same way.

So, ideally for consistency we'd change hasVariable() to just look through the ancestors from the parent. (At the moment the only code in the core that uses hasVariable() is the transclude widget, which wouldn't be impacted by this change).

[flibbles]

Sounds good. If you'd like me to make the PR, I'd be happy to. If not, feel free to close this issue out.

[pmario]

@flibbles ... Did you create a PR, that I've missed?

[flibbles]

@pmario, I guess this one slipped me by. I can still create a PR for it, but I think my personal solution had been to switch to using getVariable rather than use a deprecated internal method. That way I didn't have to deal with the backward compatibility issues.

I can create a PR for this which would be something like:

Widget.prototype.hasVariable = function(name, options) {
   return this.getVariable(name, options) !== undefined;
}

Or whatever the correct implementation would be. Nothing in core uses hasVariable. None of my stuff does either. No reason to bother being efficient. The only risk is if someone plugin developer out there is using hasVariable and expecting the widget to look at its own variables.

Edit: I guess the old hasVariable behavior could easily be maintained while still simplifying hasVariable. It might be a good idea to deprecate hasVariable too.