[4.0] Behavior.formvalidate

brianteeman commented 5 years ago

Can someone more intelligent than me confirm/deny that this doc block is correct

https://github.com/joomla/joomla-cms/blob/1f5cb315d3f242ebe5aad38a4b15a3ff202f06ca/libraries/cms/html/behavior.php#L51-L56

The reason for asking is that we have the behaviour on pages where the form doesn't have the class form-validate eg https://github.com/joomla/joomla-cms/blob/1f5cb315d3f242ebe5aad38a4b15a3ff202f06ca/administrator/components/com_messages/tmpl/config/default.php#L19

And we have the behaviour on forms where no field has the class validate - there are only 19 fields that have class="validate in the entire cms

SharkyKZ commented 5 years ago

Doc block is correct.

brianteeman commented 5 years ago

So

where the form doesn't have the class form-validate eg we should remove the behavior
we should check that if the behavior is used on a form only when there is a field with a class="validate

???

dgrammatiko commented 5 years ago

class=form-validate refers to https://github.com/joomla/joomla-cms/blob/e348081acdbb0a161fe8c1b713006e877295e17d/build/media_source/system/js/core.es6.js#L168 That's the automagic behaviour that magically validates a form without extra inline js...

Fedik commented 5 years ago

... a form only when there is a field with a class="validate

The field class seems not required, but if field has class="validate-xx" then validate.js will try to execute a "xx" validation "Handler" Maybe it was required in the past, but not now https://github.com/joomla/joomla-cms/blob/1f5cb315d3f242ebe5aad38a4b15a3ff202f06ca/build/media_source/system/js/fields/validate.es6.js#L206-L209

form-validate

This also seems not required, as @dgrammatiko already wrote. The form may be validated by Joomla.submitbutton by use of another selector. But I found that form-validate used for some attachToForm() thing, that I not remember for what is it :smiley: (maybe also some old legacy behavior that can be trashed, hm hm)

https://github.com/joomla/joomla-cms/blob/1f5cb315d3f242ebe5aad38a4b15a3ff202f06ca/build/media_source/system/js/fields/validate.es6.js#L38-L47

Fedik commented 5 years ago

ah well, attachToForm() it something for "live validation", Seems form-validate not really required, but I would leave it as is

dgrammatiko commented 5 years ago

@Fedik I'm not sure anymore but the class=form-validate in a form in conjunction with HTMLHelper::_('behavior.formvalidate') is a magic thing that bypasses all the inline scripts in the J3 like https://github.com/joomla/joomla-cms/blob/fdeeced442873c9184d639021d45f57e90db27f4/administrator/components/com_content/views/article/tmpl/edit.php#L36-L52

So form-validate class is for enabling some magic behaviour in core.js, the usage of the same class in the validate.js I guess acts as a fallback (?) but really I don't remember. Anyways the validate.js supports the HTML5 pattern attribute per field (with an optional custom validation message) and J4 should use that instead of the old handlers, which btw should be deprecated in J4 and removed at some point.

My point of view was that all js should be driven by HTML classes or other attributes, thus ruling out the inline scripts but most importantly making the script usage much easier, as HTML is something that most Joomla users are better than coding any Javascript. This also explains my point of view for custom elements as these are the definition of what I just wrote above 😉

brianteeman commented 5 years ago

roflmao - even the js guys dont know what the correct way should be

dgrammatiko commented 5 years ago

@brianteeman I'll stick to what I wrote. For further clarification go back to https://github.com/joomla/joomla-cms/pull/14710

brianteeman commented 5 years ago

@mbabker @wilsonge the js guys dont seem able to give a definitive answer on this - can you.

brianteeman commented 5 years ago

@HLeithner any ideas? I am convinced that we're not doing this correctly in the cms but its so hard to determine

Fedik commented 5 years ago

I think there no "clear answer", it need to investigate and possible refactor.

The reason, originally validate.js was MooTools script, and all you see in DocBlock is for that script. Then it was rewritten to jQuery (by someone while GSOC, if I right), and part of logic was changed/lost. Then it was rewritten to be "vanila" js, and another part of logic was changed/lost.

brianteeman commented 5 years ago

My concern is that we are nt validating data when we think we are and/or there is data not being validated that should be

wilsonge commented 5 years ago

This looks terribly wrong everywhere.

I don't see why the classes in core.js and the validation script are the same (i'm not sure they should be having looked at it but I'd have to test combinations locally to be sure. I think the former is required for validation on submission on form but the latter is doing it as you edit the field)
https://github.com/joomla/joomla-cms/pull/14710 looks just wrong on a new passover code review because we're not setting form-validate there OR passing in the data-cancel-task attribute where we removed our custom code from components
I think we need the activity happening in the core.js file (that instance of validation) with data attributes added as appropriate. However I'm unsure whether we need the stuff happening in validate.js or not right now as clearly at some point they overlap calling isValid function

brianteeman commented 5 years ago

So I wasn't being crazy in spotting this clusterf*** of validation.

It's such a mess that I am confident that we have data that should be validated that isnt

brianteeman commented 5 years ago

and this is why i can get so picky about correcting code comments

dgrammatiko commented 5 years ago

@wilsonge, to clear up a couple things here:

you are right
You are wrong form-validate was correctly set on the next PRs. Also data-cancel-task is used only if the task is not a direct article.cancel (I think com_config was the only one with double dot notation)
The validation.js was kept with the previous code (with some enhancements, eg patterns and custom validation messages) because you asked so. Both I and @Fedik were asking to move to HTML5 native browser validation but we've been said to keep B/C

dgrammatiko commented 5 years ago

To everyone participates here:

The Joomla.submitButton() needs to know only if a form is valid or not
The JFormValidator() essentially does -poorly IMHO- what browsers offer natively: realtime (not exactly, it is on blur event) validation of each form field. If heurustics for this class are wrong (honestly I dont think they are, it's just confussing that the classes clash) then that should be fixed..

brianteeman commented 5 years ago

To be clear. I raised this NOT to comment on the code involved. My only concern is that no one knows what is the correct way to do form validation AND as a result there is data not being validated that should be OR data we think is being validated but isn't.

dgrammatiko commented 5 years ago

@brianteeman in the core all form validation is done by calling the HTMLHelper you commented and adding a class form-validate on the form that needs to be submitted. That is not the only way though, you can do it by using your own Joomla.submitButton or any other js logic that will trigger the form submittion but then you will have to take care of the form validation. The form-validate class used both in core.js and valitation.js essentially is a trigger for different things on each script: in core.js will bind the submitButton to the formValidator.isValid() and in validation.js will act as a heuristic so that fields gets red color and some message if the inserted value in the input is not the expected one.

I hope this makes some sense...

brianteeman commented 5 years ago

My english skills must be really bad. Maybe this explains my concern

There are 53 files with behavior.formvalidator

To enable form validation the form tag must have class="form-validate"

There are 54 files with class=form-validate

Each field that needs to be validated needs to have class="validate".

There are 0 fields with class=validate

Additional handlers can be added to the handler for username, password, numeric and email. To use these add class="validate-email" and so on.

There are 3 fields with class=validate-numeric There are 7 fields with class=validate-password There is 1 field with class=validate-password-strength There is 1 field with class=validate-passwordExtra

dgrammatiko commented 5 years ago

@brianteeman About the 53/54 I guess somewhere a behavior.formvalidator is missing

About class=validate: that's what @Fedik wrote about the validation.js and it's totally accurate. The code transformed from Mootools to jQuery to vanilla and on the way, this requirement was dropped. The validation script will validate all inputs unless disabled or hidden (IIRC)

About the classes that denote the handler: this should work as well (that was the B/C George wanted to keep) but it's really much better to use the pattern attribute...

brianteeman commented 5 years ago

I give up

dgrammatiko commented 5 years ago

@brianteeman how about:

    /**
     * Add unobtrusive JavaScript support for form validation.
     *
     * To enable form validation the form tag must have class="form-validate".
         * The submit button, if core.js is loaded, can be hadled automatically with an onclick="Joomla.submitButton('someTask', this.form.id, this.form)" where
         *  - someTask is the task that needs to be fired in the php side
         *  - this,form.id is the id of the form, defaults to adminForm
         *  - this.form is the form element
         *  - a cancel task can be passed through data-cancel-task="conponent.cancel.form", defaults to [current view name].cancel 
         * Each field, that's not disabled or hidden, will be validated against:
         *  - a pattern attribute if exists
         *  - using a defined js handler function. For, eg: a handler email the input needs to have a
         *    a class in the form of class="validate-email"
     *
     * @return  void
     *
     * @since   3.4
     */

brianteeman commented 5 years ago

We also have cases of the behavior and/or the form-validate class being placed on forms where there is nothing to validate

dgrammatiko commented 5 years ago

The validation script will validate all inputs unless disabled or hidden (IIRC)

I am wrong here, every field that needs to be validated needs a class="validate-handler" or a pattern="some regex" according to the code otherwise only the required part is validated by default for the fields that have a required attribute. In other words, most of the backend forms don't really validate anything more than that the required inputs are not empty. Also the same goes for the J3 version, so it's not something that we introduced in the J4 repo.

I will point your own RFC here: https://github.com/joomla/joomla-cms/issues/18995 as a reminder that the tasks for the validation were never fulfilled (not blaming anyone here) but AFAIK the scripts should be production-ready already (I mean there might be some hidden bugs but since there are 0 tests these are pretty hard to be surfaced by trial and error).

We also have cases of the behavior and/or the form-validate class being placed on forms where there is nothing to validate

According to what I wrote above there is quite some work that needs to be done here, it's not only a misplaced class. Then again most forms in the backend just test the title and the category fields so I guess what's in the repo is also fine

brianteeman commented 5 years ago

finally!!

brianteeman commented 5 years ago

another great example of the problem https://github.com/joomla/joomla-cms/pull/25733#issuecomment-515934249

wilsonge commented 4 years ago

Cleanup of class vs usage https://github.com/joomla/joomla-cms/pull/28352

wilsonge commented 4 years ago

Are we able to validate if we now have the same behaviour in 3.x and 4.x? My feeling is we do (even though I fully acknowledge that behaviour is probably not expected - but its no longer a 4.x specific issue)

dgrammatiko commented 4 years ago

@wilsonge maybe you should consider: https://formally-valid.info https://codepen.io/pen?&editable=true&editors=100

brianteeman commented 4 years ago

yay for html5 validation first

wilsonge commented 4 years ago

I'm happy to consider HTML5 validation. But I do want this to provide behaviour as a b/c thing for 3.x. If what we have after my PR is the same as what we have there then I can remove this as a beta blocker.

brianteeman commented 4 years ago

First test was on email validation In J3 you can correctly have an email of name@host In J4 this will fail

wilsonge commented 4 years ago

As we discussed this was from the PHP not the JS - so still looking good so far to downgrade this from beta blocker

brianteeman commented 4 years ago

so far yes. busy today but will carry on tomorrow

zero-24 commented 4 years ago

@wilsonge can you please make clear what you want to be tested / compared for this issue to be marked as solved? So we track all of them as to-do list and work on them step by step.

Maybe @brianteeman can than share what has been tested already. So we don't dublicate the efforts. When there is a list we might can get some testers at the next Bugs & Fun @home event too.

ceford commented 4 years ago

Something to ponder: if a numeric field has invalid data entered the browser treats the field as empty and validation passes. For example, type in 100x in Firefox or 100+-10.5 in Google Chrome and line 243 in validate.js says element.value is empty and validate.numeric is not called, so passes validation. Otherwise I can confirm in the original question - the form does need class="form-validate" and fields with specific handlers do need class="validate-handler". The doc block wording "can be added" is maybe misleading because the mentioned handlers are already added. There is no passwordExtra handler so no client side validation that passwords 1 and 2 match.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25490.}

richard67 commented 4 years ago

Anything new with this issue?

wilsonge commented 4 years ago

I believe from a parity perspective with Joomla 3 this is fixed. if it's not it can be re-opened as a fresh release blocker issue

joomla / joomla-cms

[4.0] Behavior.formvalidate #25490