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

NEW Validate DBFields #11408

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Sep 30, 2024

Issue #11403

  • Adds the new FieldValiator class and multple subclasses
  • Adds a mechanism for adding FieldValidators to DBFields and FormFields, and calling them when appropriate
  • Updates some setValue() methods on DBField subclasses to convert values e.g. string integers to integers. This is mainly done to soften some of the upgrade pain now that just about everything is being validated.
  • Adds new DBField types - DBEmail, DBIp, DBUrl
  • Adds a $value param to ValidationResult. This is mainly to make debugging easier.

Note that while this PR adds support to FormField for FieldValidators, I haven't actually added the FieldValidators to the FormField's in this PR, as too much would be changing at once and many things would break in CI. Instead I've split off a separate issue for that to be done after this has been merged Have removed FormField changes

There is some type conversion going on in DBField::setValue(), which happens before validation, though I'm trying to not be too permissive as this PR is essentially adding strict typing to DBFields. Some examples:

  • DBBoolean allows a wide range of values for backwards compatibility e.g. 't' will get converted to true
  • DBInt will convert numeric ints to int. One reason for allow this is because HTTP form field submissions are strings, so it's required when doing things like $form->saveInto($dataObject);
  • DBDecimal will convert int to float, because not allowing $obj->MyDecimal = 5; is pointlessly annoying as we're not loosing any precision. Note the opposite is not allowed i.e. there is no conversion when going $obj->MyInt = 5.1, as this would truncate the decimal place. This will trigger a validation exception instead.

A few things that we might want to consider changing (easiest done as follow up cards):

  • DBBoolean should possibly stop accepting values such as 't' will retain backward compatibility
  • DBDate currently accepts a range of dates and attempts to fix things. Changes were made in this PR, though one not unexpected and not great side effect is that a value that previously threw an error during DBDate::setValue() "03-03-04" now gets turned into "2003-03-04". The existing behaviour of turning "0" into "1970-01-01" remain. Perhaps we should be stricter and only allow 'Y-m-d' style inputs. Have reverted changes, now behaves like CMS 5
  • DBTime::parseTime() uses strtotime() so will parse things like "23.02.56" and "11:02 pm". We should consider forcing "H:i:s" format for all inputted strings
  • DBString subclasses e.g. DBVarchar will not convert int/float to string. The intention here is a user may mistakenly be assigning ints/floats to a Varchar field and then later attempt numeric operation e.g. adding, subtraction. Argument could be made that we should convert here. (opposite of above points, i.e. be "more helpful" rather than "more strict")

Also to answer the question "why are we even doing this when you can just get the database to do this?". It's because we cannot give any guarantees is MySQL is set in strict mode (will complain and not update value when invalid) vs not in strict mode (will truncate and insert). Also, FieldValidator is an abstract class that will be shared with FormField validation, which doesn't even touch the database. Also, things like EmailFieldValidator can't be validated by MySQL.

@emteknetnz emteknetnz mentioned this pull request Sep 30, 2024
10 tasks
@emteknetnz emteknetnz force-pushed the pulls/6/field-validators branch 25 times, most recently from 00a13da to 1f4a38a Compare October 3, 2024 06:18
@emteknetnz emteknetnz force-pushed the pulls/6/field-validators branch 2 times, most recently from 926371b to 9c169bd Compare October 4, 2024 02:07
@emteknetnz emteknetnz force-pushed the pulls/6/field-validators branch 2 times, most recently from 5242fd9 to 158a9d4 Compare October 30, 2024 01:59
@GuySartorelli
Copy link
Member

There's a bunch of stuff still not addressed.
I've decided to let most of them slide, but the following at the very least need to be responded to:

@emteknetnz
Copy link
Member Author

emteknetnz commented Oct 31, 2024

@GuySartorelli after rebasing in the template work I needed to make a small change to a test in CastingServiceTest.php as DBCurrency extends DBDecimal and getValue() now returns a float, previously the test was expecting a string

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, but found a few anomolies while testing

src/ORM/FieldType/DBPercentage.php Outdated Show resolved Hide resolved
src/ORM/FieldType/DBBoolean.php Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/6/field-validators branch 3 times, most recently from d18eeeb to 1a6c9ce Compare November 4, 2024 03:44
@emteknetnz emteknetnz force-pushed the pulls/6/field-validators branch 3 times, most recently from 873e01d to a5133fd Compare November 5, 2024 02:55
src/Dev/CsvBulkLoader.php Outdated Show resolved Hide resolved
src/Dev/CsvBulkLoader.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/6/field-validators branch 2 times, most recently from ed9c52d to a8ca5e9 Compare November 5, 2024 22:00
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the last thing before the code PRs can be merged.
Don't bother rerunning kitchen sink after making this change

src/Dev/CsvBulkLoader.php Outdated Show resolved Hide resolved
@GuySartorelli GuySartorelli merged commit 3f863b2 into silverstripe:6 Nov 6, 2024
5 of 10 checks passed
@GuySartorelli GuySartorelli deleted the pulls/6/field-validators branch November 6, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants