Page MenuHomePhabricator

AbuseFilterComputeVariableHook parameter $result type seems incorrect
Open, Needs TriagePublic

Description

Should $result really be a nullable string?

interface AbuseFilterComputeVariableHook {
	/**
	 * Hook runner for the `AbuseFilter-computeVariable` hook
	 *
	 * Like AbuseFilter-interceptVariable but called if the requested method wasn't found.
	 * Return true to indicate that the method is known to the hook and was computed successful.
	 *
	 * @param string $method Method to generate the variable
	 * @param VariableHolder $vars
	 * @param array $parameters Parameters with data to compute the value
	 * @param ?string &$result Result of the computation
	 * @return bool|void True or no return value to continue or false to abort
	 */
	public function onAbuseFilter_computeVariable(
		string $method,
		VariableHolder $vars,
		array $parameters,
		?string &$result
	);
}

Then the usage in LazyVariableComputer...

		return $result instanceof AFPData ? $result : AFPData::newFromPHPVar( $result );

And then AFPData::newFromPHPVar uses

	/**
	 * @param mixed $var
	 * @return AFPData
	 * @throws InternalException
	 */
	public static function newFromPHPVar( $var ) {

Event Timeline

Reedy renamed this task from AbuseFilterComputeVariableHook parameter $result seems incorrect to AbuseFilterComputeVariableHook parameter $result type seems incorrect.Aug 17 2022, 3:52 PM

Indeed, it should allow AFPData, any scalar type, lists, and null. So the correct union type would be AFPData|string|int|float|bool|list|null. The list case should really only allow scalars, null, or other lists with the same rules as elements, but I don't think this recursive definition can be captured in a docblock. I suppose formally it would be like:

T := 'AFPData' | S | L;
L := list< S | L >
S := 'string' | 'int' | 'float' | 'bool' | 'null'