harmen/hypha
1
0
Fork 0
Code Issues 142 Pull requests 4 Projects Releases Packages Wiki Activity

Refactor form validation #360

Merged
matthijskooijman merged 10 commits from refactor-form-validation into master 2021-02-03 13:22:47 +00:00
Conversation 17 Commits 10 Files changed 8 +160 -129
matthijskooijman commented 2020-12-07 16:34:19 +00:00 (Migrated from github.com)
Copy link

The main point of this PR is the last commit, which guards against insertion of <script> tags in HTML fields. The other commits refactor the form input validation so there is now a single validate() method that must always be called, where this check can be done automatically on all wymeditor fields (previously, validation always required calling validateXXXField() methods explicitly). These refactors were something I had been considering for a while, since they make the form generation again a little bit more declarative and automatic.

The main point of this PR is the last commit, which guards against insertion of `<script>` tags in HTML fields. The other commits refactor the form input validation so there is now a single `validate()` method that must always be called, where this check can be done automatically on all wymeditor fields (previously, validation always required calling `validateXXXField()` methods explicitly). These refactors were something I had been considering for a while, since they make the form generation again a little bit more declarative and automatic.
RoukePouw (Migrated from github.com) reviewed 2020-12-11 14:10:35 +00:00
system/core/document.php
@ -591,0 +661,4 @@
function validateHtmlField($name) {
$value = $this->dataFor($name);
if (!$value)
return true;
RoukePouw (Migrated from github.com) commented 2020-12-11 14:10:35 +00:00
Copy link

If an email field is empty I think you would like the message 'required field missing', not 'incorrect email address' which seems to happen now that the first error gets overwritten by the second? Perhaps shift the order? Or make the errors per name into an array instead?

If an email field is empty I think you would like the message 'required field missing', not 'incorrect email address' which seems to happen now that the first error gets overwritten by the second? Perhaps shift the order? Or make the errors per name into an array instead?
RoukePouw (Migrated from github.com) reviewed 2020-12-11 14:17:14 +00:00
system/languages/en.php
RoukePouw (Migrated from github.com) commented 2020-12-11 14:17:14 +00:00
Copy link

I find this message slightly confusing..
If I as a user would copy paste some html I wanted to use and I get a notification that my computer might be infected by a virus. I would get worried, perhaps for the wrong reasons.
Something along these lines perhaps? "HTML <script> tags are not allowed. They are considered unsafe as they can be used for hacks or virusses."

I find this message slightly confusing.. If I as a user would copy paste some html I wanted to use and I get a notification that my computer might be infected by a virus. I would get worried, perhaps for the wrong reasons. Something along these lines perhaps? `"HTML <script> tags are not allowed. They are considered unsafe as they can be used for hacks or virusses."`
RoukePouw (Migrated from github.com) reviewed 2020-12-11 14:19:45 +00:00
system/core/document.php
RoukePouw (Migrated from github.com) commented 2020-12-11 14:19:45 +00:00
Copy link

Does this need to be cleaned up afterwards?

Does this need to be cleaned up afterwards?
RoukePouw (Migrated from github.com) reviewed 2020-12-11 14:21:50 +00:00
system/languages/en.php
RoukePouw (Migrated from github.com) commented 2020-12-11 14:21:50 +00:00
Copy link

Ah, just read this comment in the code above, this explains the reasoning behind the stringent message. But I think the message could still be made more clear.

 (and these have been seen to be
 			// automatically inserted into requests by
 			// viruses...).
Ah, just read this comment in the code above, this explains the reasoning behind the stringent message. But I think the message could still be made more clear. ``` (and these have been seen to be // automatically inserted into requests by // viruses...). ```
RoukePouw (Migrated from github.com) reviewed 2020-12-11 14:23:31 +00:00
system/core/document.php
@ -591,0 +661,4 @@
function validateHtmlField($name) {
$value = $this->dataFor($name);
if (!$value)
return true;
RoukePouw (Migrated from github.com) commented 2020-12-11 14:23:31 +00:00
Copy link

Ah, no because the an empty email field is considered valid, the error won't be overwritten.

Ah, no because the an empty email field is considered valid, the error won't be overwritten.
matthijskooijman (Migrated from github.com) reviewed 2020-12-11 14:25:28 +00:00
system/core/document.php
@ -591,0 +661,4 @@
function validateHtmlField($name) {
$value = $this->dataFor($name);
if (!$value)
return true;
matthijskooijman (Migrated from github.com) commented 2020-12-11 14:25:28 +00:00
Copy link

Exactly.

Exactly.
RoukePouw (Migrated from github.com) reviewed 2020-12-11 14:31:10 +00:00
system/core/document.php
@ -584,2 +639,4 @@
* directly.
*/
function validateEmailField($name) {
$value = $this->dataFor($name);
RoukePouw (Migrated from github.com) commented 2020-12-11 14:31:10 +00:00
Copy link

Executing the form validate() here, if not yet done, would reduce some code duplication. It would loose it's const-ness but it would perform more as the name suggests.

Executing the form validate() here, if not yet done, would reduce some code duplication. It would loose it's const-ness but it would perform more as the name suggests.
matthijskooijman (Migrated from github.com) reviewed 2020-12-11 14:34:58 +00:00
system/languages/en.php
matthijskooijman (Migrated from github.com) commented 2020-12-11 14:34:58 +00:00
Copy link

Yeah, I didn't spend too much effort on the message, since it should not normally be shown. Do you have a specific suggestion for a better message?

Yeah, I didn't spend too much effort on the message, since it should not normally be shown. Do you have a specific suggestion for a better message?
matthijskooijman (Migrated from github.com) reviewed 2020-12-11 14:37:08 +00:00
system/core/document.php
matthijskooijman (Migrated from github.com) commented 2020-12-11 14:37:08 +00:00
Copy link

I don't think so, since it is created but not added into the document tree (so I'm assuming PHP will garbage collect the DomNode itself).

I don't think so, since it is created but not added into the document tree (so I'm assuming PHP will garbage collect the DomNode itself).
matthijskooijman (Migrated from github.com) reviewed 2020-12-11 14:41:40 +00:00
system/core/document.php
@ -584,2 +639,4 @@
* directly.
*/
function validateEmailField($name) {
$value = $this->dataFor($name);
matthijskooijman (Migrated from github.com) commented 2020-12-11 14:41:40 +00:00
Copy link

Interesting idea, though I'm not entirely sure. Always running validate would mean a form can be validated more than once (which is a waste of efffort), but running it only if not ran before means that re-validating after a change in the form is not possible (we don't really use or support this now, though).

But maybe running it when not ran before is a good idea. If we ever need to re-validate, you could just call validate() explicitly then. I'll see how to best implement this, since it might nicest to fold this change back into the git history (but maybe that's too much work and I'll just add a new commit).

Interesting idea, though I'm not entirely sure. Always running validate would mean a form can be validated more than once (which is a waste of efffort), but running it only if not ran before means that re-validating after a change in the form is not possible (we don't really use or support this now, though). But maybe running it when not ran before is a good idea. If we ever need to re-validate, you could just call `validate()` explicitly then. I'll see how to best implement this, since it might nicest to fold this change back into the git history (but maybe that's too much work and I'll just add a new commit).
RoukePouw (Migrated from github.com) reviewed 2020-12-14 08:56:08 +00:00
system/languages/en.php
RoukePouw (Migrated from github.com) commented 2020-12-14 08:56:07 +00:00
Copy link

Something like this?
"HTML

Something like this? "HTML <script> tags are not allowed. They are considered unsafe as they can be used for hacks or viruses. If you not add them yourself your computer could even be infected."
laurensmartina (Migrated from github.com) reviewed 2020-12-23 10:51:33 +00:00
system/core/document.php
@ -584,2 +639,4 @@
* directly.
*/
function validateEmailField($name) {
$value = $this->dataFor($name);
laurensmartina (Migrated from github.com) commented 2020-12-23 10:51:33 +00:00
Copy link

If it would self validate, then, when a value gets updated, the is_validated flag could be set to false.

If it would self validate, then, when a value gets updated, the is_validated flag could be set to false.
laurensmartina (Migrated from github.com) approved these changes 2020-12-23 10:56:30 +00:00
laurensmartina (Migrated from github.com) left a comment
Copy link

Very cool improvement!

Very cool improvement!
matthijskooijman (Migrated from github.com) reviewed 2021-02-03 12:51:53 +00:00
system/languages/en.php
matthijskooijman (Migrated from github.com) commented 2021-02-03 12:51:53 +00:00
Copy link

Thanks, I've used your suggestion.

Thanks, I've used your suggestion.
matthijskooijman (Migrated from github.com) reviewed 2021-02-03 13:21:59 +00:00
system/core/document.php
@ -584,2 +639,4 @@
* directly.
*/
function validateEmailField($name) {
$value = $this->dataFor($name);
matthijskooijman (Migrated from github.com) commented 2021-02-03 13:21:59 +00:00
Copy link

Ok, I implemented automatic validation like suggested. Re-validating / clearing is_validated on changes is not yet implemented, since there is no meaningful way to change values right now I believe, so this is left for later.

Ok, I implemented automatic validation like suggested. Re-validating / clearing `is_validated` on changes is not yet implemented, since there is no meaningful way to change values right now I believe, so this is left for later.
matthijskooijman commented 2021-02-03 13:22:28 +00:00 (Migrated from github.com)
Copy link

Rebased on top of master and made some small changes based on review, going to review next.

Rebased on top of master and made some small changes based on review, going to review next.
matthijskooijman commented 2021-02-09 12:03:25 +00:00 (Migrated from github.com)
Copy link

Hm, turns out this has an unintended side effect: Because everything lives in one big form, validation errors in e.g. the comment form let the browser prevent submitting the login form.

For now, I've fixed this by reverting this PR on www.destadsbron.nl, but the proper fix for this is probably to separate each form into its own <form> tag, which is a refactor that we were planning to do anyway (which is #359).

Hm, turns out this has an unintended side effect: Because everything lives in one big form, validation errors in e.g. the comment form let the browser prevent submitting the login form. For now, I've fixed this by reverting this PR on www.destadsbron.nl, but the proper fix for this is probably to separate each form into its own `<form>` tag, which is a refactor that we were planning to do anyway (which is #359).
Sign in to join this conversation.
Reviewers
No reviewers
laurensmartina
Labels
Clear labels   
Component: User interface

   
Component: Wymeditor

   
Help wanted

   
Level: Difficult

Structural changes, need discussion

  
Level: Easy

Under 4 hours of work

  
Level: Moderate

Under 16 hours of work

  
Pagetype: Festival

   
Pagetype: Mailinglist

   
Pagetype: Peer reviewed article

   
Pagetype: Text

   
Privacy GDPR AVG

   
status: has conflicts

   
Status: Needs changes

   
Status: Needs discussion

   
Status: Needs review

   
Status: Ready to merge

   
Status: Waiting for response

   
Type: Bug

   
Type: Enhancement

   
Type: Question

   
Usecase: De Stadsbron

   
Usecase: Koppelting

   
Usecase: MeetjeStad

   
Value: Coders

Enhancement of codebase, API, documentation

  
Value: Security

   
Value: Users

Improve user experience, add new features

  
Value: Visitors

Improve navigation, styling

No labels
Component: User interface
Component: Wymeditor
Help wanted
Level: Difficult
Level: Easy
Level: Moderate
Pagetype: Festival
Pagetype: Mailinglist
Pagetype: Peer reviewed article
Pagetype: Text
Privacy GDPR AVG
status: has conflicts
Status: Needs changes
Status: Needs discussion
Status: Needs review
Status: Ready to merge
Status: Waiting for response
Type: Bug
Type: Enhancement
Type: Question
Usecase: De Stadsbron
Usecase: Koppelting
Usecase: MeetjeStad
Value: Coders
Value: Security
Value: Users
Value: Visitors
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
harmen/hypha!360
No description provided.