Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dialog.validate.functions incorrectly joins validator results #4449

Closed
bunglegrind opened this issue Dec 24, 2020 · 5 comments · Fixed by #5248
Closed

dialog.validate.functions incorrectly joins validator results #4449

bunglegrind opened this issue Dec 24, 2020 · 5 comments · Fixed by #5248
Assignees
Labels
good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. plugin:dialog The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug.
Milestone

Comments

@bunglegrind
Copy link
Contributor

bunglegrind commented Dec 24, 2020

Type of report

Bug

Provide detailed reproduction steps (if any)

just implement the sample code in the documentation

https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_dialog_validate.html#method-functions

Expected result

A validator composed by notEmpty and number

Actual result

A validator which always return true

Other details

Tested on 4.15.1 on Firefox latest version.

The sample code should be:

CKEDITOR.dialog.validate.functions(
    CKEDITOR.dialog.validate.notEmpty( ), //Note: no error message!
    CKEDITOR.dialog.validate.number( ) ,//Note: no error message!
    'error!'
);

The real problem is that in case of failed validation, the validation functions (validate.regex for instance) return the error message which is actually a truthy value in javascript (unless it's the empty string). They should return false: the current behavior is quite misleading.

At least, please update the sample code in the documentation.

btw, there are no tests related to dialog.validate in tests/plugin/dialog

@bunglegrind bunglegrind added the type:bug A bug. label Dec 24, 2020
@bunglegrind
Copy link
Contributor Author

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

passed = passed && functions[ i ]( value );

passed = passed || functions[ i ]( value );

@f1ames
Copy link
Contributor

f1ames commented Dec 29, 2020

Interesting find @bunglegrind, thanks for reporting. Indeed it looks like an issue in the code (and misleading documentation too). We will take a look on this one (and if there are any unintended consequences in the existing plugins).

@f1ames f1ames added good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. plugin:dialog The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. labels Dec 29, 2020
@bunglegrind
Copy link
Contributor Author

Well, you could start by updating the example in the docs. I spent one hour in order to discover what was going wrong...

@lukewhitmore
Copy link

I can confirm the same.

@bunglegrind - thanks for the suggestion of a workaround.

@sculpt0r sculpt0r changed the title Misleading example of dialog.validate.functions or bug? dialog.validate.functions incorrectly joins validator results Jun 22, 2022
@sculpt0r sculpt0r added size:XS and removed size:? labels Jun 22, 2022
@sculpt0r sculpt0r self-assigned this Jun 22, 2022
sculpt0r added a commit that referenced this issue Jul 4, 2022
@CKEditorBot
Copy link
Collaborator

Closed in #5248

@CKEditorBot CKEditorBot added this to the 4.19.1 milestone Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. plugin:dialog The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants