Dialog validators does not work if there is no 'getValue()' method provided in the current context

[f1ames]

Type of report

Bug

Provide detailed reproduction steps (if any)

The dialog plugin provides it's own set of validators, for example cssLength:

ckeditor4/plugins/dialog/plugin.js

Lines 3341 to 3345 in 5380706

	'cssLength': function( msg ) {
	return this.functions( function( val ) {
	return cssLengthRegex.test( CKEDITOR.tools.trim( val ) );
	}, msg );
	},

And the API docs states they can be simply used as follows:

CKEDITOR.dialog.validate.cssLength( 'error!' )( '10pt' );

However, such calls throws an error:

Uncaught TypeError: b.replace is not a function
    at Object.trim (ckeditor.js:30)
    at Array.<anonymous> (ckeditor.js:663)
    at ckeditor.js:663
    at <anonymous>:1:47

Expected result

It works as described in API docs.

Actual result

Error is thrown.

Other details

Well, that's the interesting part... Many of the validators use special functions() method which combines validators:

ckeditor4/plugins/dialog/plugin.js

Lines 3219 to 3258 in 5380706

	functions: function() {
	var args = arguments;
	return function() {
	// It's important for validate functions to be able to accept the value
	// as argument in addition to this.getValue(), so that it is possible to
	// combine validate functions together to make more sophisticated
	// validators.
	var value = this && this.getValue ? this.getValue() : args[ 0 ];
	var msg,
	relation = CKEDITOR.VALIDATE_AND,
	functions = [],
	i;
	for ( i = 0; i < args.length; i++ ) {
	if ( typeof args[ i ] == 'function' )
	functions.push( args[ i ] );
	else
	break;
	}
	if ( i < args.length && typeof args[ i ] == 'string' ) {
	msg = args[ i ];
	i++;
	}
	if ( i < args.length && typeof args[ i ] == 'number' )
	relation = args[ i ];
	var passed = ( relation == CKEDITOR.VALIDATE_AND ? true : false );
	for ( i = 0; i < functions.length; i++ ) {
	if ( relation == CKEDITOR.VALIDATE_AND )
	passed = passed && functions[ i ]( value );
	else
	passed = passed || functions[ i ]( value );
	}
	return !passed ? msg : true;
	};
	},

When the validator is created (like in cssLength above), functions() returns another function (let's call it validatorFn()) which has access to initial args. This means calling CKEDITOR.dialog.validate.cssLength( 'error!' ) internally calls:

.functions( `cssLengthInternalFn`, `error!` )

ckeditor4/plugins/dialog/plugin.js

Lines 3342 to 3344 in 5380706

	return this.functions( function( val ) {
	return cssLengthRegex.test( CKEDITOR.tools.trim( val ) );
	}, msg );

Which returns validatorFn() mentioned above. This functions accesses the value to be validated in two ways. Using getValue() method from the current context or accessing args[ 0 ] (the ones from functions() call):

ckeditor4/plugins/dialog/plugin.js

Line 3226 in 5380706

var value = this && this.getValue ? this.getValue() : args[ 0 ];

If getValue() method is present in the current context everything works fine. But if not, args[ 0 ] are used as a value to be validated. However, in the context of .functions( cssLengthInternalFn, 'error!' ) call, args[ 0 ] are cssLengthInternalFn (so the validator function). And so args[ 0 ] are both used as validator function and value to be validated (not a string so Uncaught TypeError: b.replace is not a function is thrown).

I wonder if the API docs are just confusing and it was never meant to be used without extending dialog and adding getValue() there, but the comment there states it should work:

ckeditor4/plugins/dialog/plugin.js

Lines 3222 to 3226 in 5380706

	// It's important for validate functions to be able to accept the value
	// as argument in addition to this.getValue(), so that it is possible to
	// combine validate functions together to make more sophisticated
	// validators.
	var value = this && this.getValue ? this.getValue() : args[ 0 ];

• Browser: All
• OS: All
• CKEditor version: At least 4.10.0 but looking at git history it might be 4.0.0 as well 🤔
• Installed CKEditor plugins: dialog

[bunglegrind]

Related? #4449

[f1ames]

Related? #4449

@bunglegrind I was checking it, but it seems those are different issues. As you mentioned in #4449 (comment)

Or explicity check for boolean (=== true) in the following lines...

Should be enough to fix #4449. And here the fix would be replacing:

var value = this && this.getValue ? this.getValue() : args[ 0 ];

with

var value = this && this.getValue ? this.getValue() : arguments[ 0 ];

so that arguments from validatorFn() are used instead of the one bond to functions() context call.

Still, it would be reasonable to fix both in one PR seems they concern the same function.

[sculpt0r]

Looks like notEmpty validator contains also part of the fix logic - so we definitely could solve this for all validators.

[CKEditorBot]

Closed in #5254