Introduce registerUndefinedTestCallback

[VincentLanglet]

This is similar to registerUndefinedFilterCallback and registerUndefinedFunctionCallback

[fabpot]

Can you add a note in CHANGELOG?

[stof]

How does this behave regarding the fact that test names can be either 1 identifier or 2 identifiers ? Will this call the undefined test callback for the first identifier before trying to find a known test named in 2 words ?

And doesn't this require a special handling for guard nodes for the case where the callback is throwing its own SyntaxError to provide a custom error message ?

[VincentLanglet]

How does this behave regarding the fact that test names can be either 1 identifier or 2 identifiers ? Will this call the undefined test callback for the first identifier before trying to find a known test named in 2 words ?

Yes, the same way getTest already works.

Currently, if I define a divisible test, the divisible by test won't work anymore.
I think it could be better to only use single-word tests or to reserve some words, but that another debate.

Here, when implementing registerUndefinedTestCallback, I'll have to return false for divisible and same ; and a mock for others.

And doesn't this require a special handling for guard nodes for the case where the callback is throwing its own SyntaxError to provide a custom error message ?

I copied the implementation of registerUndefinedTokenParserCallback, registerUndefinedFunctionCallback and registerUndefinedFilterCallback ; why would I need a special handling here ?

[stof]

Yes, the same way getTest already works.

The difference is that currently, there is nothing special being run when the first identifier does not correspond to an undefined test. The fact that the parser checks 2 different test names is not an observable behavior currently.
Calling the undefined handler callback for starts when parsing {% if foo starts with('bar') %} looks wrong to me as starts with is a defined test.

I copied the implementation of registerUndefinedTokenParserCallback, registerUndefinedFunctionCallback and registerUndefinedFilterCallback ; why would I need a special handling here ?

Because those undefined callbacks also required special handling to account for the case where the callback is throwing. See #4575 where this was implemented to fix #4505

[stof]

Here, when implementing registerUndefinedTestCallback, I'll have to return false for divisible and same ; and a mock for others.

This makes it possible for an undefined test callback to break core tests. This looks very fragile to me, and is not something I would like to see being shipped in Twig (it will be a maintenance nightmare, especially to preserve BC on it).

[VincentLanglet]

The difference is that currently, there is nothing special being run when the first identifier does not correspond to an undefined test. The fact that the parser checks 2 different test names is not an observable behavior currently. Calling the undefined handler callback for starts when parsing {% if foo starts with('bar') %} looks wrong to me as starts with is a defined test.

After a new look @stof, this issue doesn't exist since the Twig Parser is doing the right check (2 word or 1 word) based on the number of name token.

Twig/src/Parser.php

Lines 493 to 502 in 98b664a

	if ($this->stream->test(Token::NAME_TYPE)) {
	// try 2-words tests
	$name = $name.' '.$this->getCurrentToken()->getValue();
	if ($test = $this->env->getTest($name)) {
	$this->stream->next();
	}
	} else {
	$test = $this->env->getTest($name);
	}

So you can mock the tests without expecting issue with one/two words tests. I tried on my project.

BTW, starts with is an operaor.

I think tests are used in the format {% if foo is myTest('bar') %}

I copied the implementation of registerUndefinedTokenParserCallback, registerUndefinedFunctionCallback and registerUndefinedFilterCallback ; why would I need a special handling here ?

Because those undefined callbacks also required special handling to account for the case where the callback is throwing. See #4575 where this was implemented to fix #4505

Should be updated

[stof]

Indeed, I used the wrong example when using starts with. I should have used same as or divisible by

So you can mock the tests without expecting issue with one/two words tests. I tried on my project.

would be great to add tests covering this to prevent regressions (add a undefined test handler doing something for divisible for instance)

[stof]

And I suggest also adding a test guarding a 2-word test name to ensure it works.

[VincentLanglet]

And I suggest also adding a test guarding a 2-word test name to ensure it works.

GuardTokenParser didn't have support for 2-word test in the first place. I had to add it.

All tests are added.

[VincentLanglet]

If preferred @fabpot @stof I extracted the support for two words test in guard tag in a PR #4689 which can be reviewed/merged first.

[VincentLanglet]

Rebased @fabpot @stof

Ci failure seems unrelated.

[fabpot]

Suggested change

	* Add `Environment::registerUndefinedTestCallback()` method.
	* Add `Environment::registerUndefinedTestCallback()`

[fabpot]

I think you also need to update the docs for that new feature.

[VincentLanglet]

I think you also need to update the docs for that new feature.

Indeed, I updated the doc

[fabpot]

Thank you @VincentLanglet.