Convert form validation of users to entity validation

Hello.
DrupalSib team (Russia, Novosibirsk) want to implements this on today code sprint.

We could not solve this problem completely. We have progress, but many questions remain. We made an temporary patch where using @todo for marking of our questions. We need help to finish this task.

tagging

And the User NG conversion from #1818570: Convert users to the new Entity Field API that you started doing in here. You'll need these two things to start with this.

Thank you, but I will have time for it only in next weekend (June 8, 9)

Getting started here with user name and email validation.

Imported the email_field fixes from #2002158: Convert form validation of comments to entity validation, implemented the unique constraint for user name and e-mail. Centralized the user name validation into constraints and factored that out of user_validate_name().

Added tests for the remaining user base fields. I also had to fix EntityReferenceItem to not assume entity IDs are integers. I guess this will fail in a couple of places I haven't noticed yet.

Ok, converting user roles to an enttiy reference field is much harder than I thought, so I split that out to a separate issue: #2044859: Convert user roles to entity_reference_field.

so I split that out to a separate issue:

Good idea!

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UniqueValidator.phpundefined
@@ -0,0 +1,38 @@
+    $field_name = $field->getName();

This can be removed.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UniqueValidator.phpundefined
@@ -0,0 +1,38 @@
+    $uid = $user->get('uid')->value;

This can also be removed $user->id() can be used in query.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserNameConstraintValidator.phpundefined
@@ -0,0 +1,54 @@
+    if (preg_match('/[^\x{80}-\x{F7} a-z0-9@_.\'-]/i', $name)) {
+      $this->context->addViolation($constraint->illegalMessage);
+    }
+    elseif (preg_match('/[\x{80}-\x{A0}' .         // Non-printable ISO-8859-1 + NBSP
+                    '\x{AD}' .                // Soft-hyphen
+                    '\x{2000}-\x{200F}' .     // Various space characters
+                    '\x{2028}-\x{202F}' .     // Bidirectional text overrides
+                    '\x{205F}-\x{206F}' .     // Various text hinting characters
+                    '\x{FEFF}' .              // Byte order mark
+                    '\x{FF01}-\x{FF60}' .     // Full-width latin
+                    '\x{FFF9}-\x{FFFD}' .     // Replacement characters
+                    '\x{0}-\x{1F}]/u',        // NULL byte and control characters
+                    $name)) {
+      $this->context->addViolation($constraint->illegalMessage);

It is adding same violation so why not use || instead of if and elseif

Thanks, fixed those issues.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UniqueValidator.phpundefined
@@ -0,0 +1,37 @@
+ * Validates the unique user proeprty constraint, such as name and e-mail.

typo

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UniqueValidator.phpundefined
@@ -0,0 +1,37 @@
+class UniqueValidator extends ConstraintValidator {

Do we want to put 'user' into the classname. Would help in an IDE I think?

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UniqueValidator.phpundefined
@@ -0,0 +1,37 @@
+      ->condition('uid', (int) $uid, '<>')

Is the cast here needed? Can we not trust the id() method call above?

Fixed all points from #17.

Thanks. RTBC.

It'd be nice to profile the user_validate_name() function to see the cost of typed_data and the constraint system.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -190,7 +190,8 @@ public function invokeFieldItemPrepareCache(EntityInterface $entity) {
-            || (is_subclass_of($type_definition['class'], '\Drupal\field\Plugin\field\field_type\LegacyConfigFieldItem') && function_exists($type_definition['module'] . '_field_load'))) {
+            || (is_subclass_of($type_definition['class'], '\Drupal\field\Plugin\field\field_type\LegacyConfigFieldItem')
+              && isset($type_definition['module']) && function_exists($type_definition['module'] . '_field_load'))) {

This will have changed due to #2041423: Rely on 'provider' instead of 'module' for Field plugin types

Rerolled.

Not sure why this need profiling - the validation is not in the critical performance path. I doubt that profiling this will provide any useful results - of course it will be 1000% slower in relative numbers, because it now does more than just some regular expressions. In absolute numbers I predict that it would go from 0.005ms to 0.1ms.

I'm going to write a drush script for profiling.

So here are the benchmarks, execute the attached script with "drush php-script profiling.inc".

Before the patch:
1000 calls: 24.68ms
1 call average: 0.02468ms

After the patch:
1000 calls: 265.23ms
1 call average: 0.26523ms

So as expected the old code is 10 times faster, but one call is still well under one millisecond, so nothing to worry about.

It is true that this isn't performance critical. It is also pathetic IMO that we are slowing down Drupal by 10x. Other examples of this slowdown *are* on the critical path.

We are not slowing down Drupal by 10x, we are slowing down user name validation by 10x. This is an important distinction. The overall Drupal page request takes hundreds of milliseconds, and this change takes 0.1% of that time to validate a user name. And of course not on every page request, but only when users register on the site or change their user name.

We are trading a flexible and powerful validation system here against a tiny performance penalty. We should focus our performance considerations on the problems in the critical every-request-path, and I doubt that the entity validation API ever shows up in it. Please link those example issues here, would be interested to look at them!

Patch still applies, anyone available for review or RTBC?

OK, lets let the committers evaluate that profiling. The code looks proper.

Assigning to catch as the committer with most performance experience.

@klausi can you confirm that this really is only on registration or when usernames are changed. Generally we don't differentiate between saving identical vs. updated values on entity updates so I'd expect it to run every time the entity is saved regardless of whether it's actually updated or not.

If we are doing these checks on every entity save, then we could likely make an overall improvement in performance despite this patch if we changed our validation logic to only happen on updated values - i.e. if a user has ten fields and only one of them is a changed value, then that's 1/10th the validation code running.

The ->validate() method is never called from an entity save, that are two separate things. Entity save must not validate data (well, the DB schema does a bit and throws PDOExceptions, but that does not count here).

So this does not affect entity saves at all, the validation API is only called explicitly in forms or in REST module.

Right but if a user account form is submitted, that might be to update contact preferences or something, and not username or e-mail address. Validation will run every time though.

That's true, but this is how we do validation: we validate all submitted form data, regardless of the state in the database. I'm not convinced we should change that for a minor performance gain.

It could be tricky if you have validation dependencies between fields, example: a workflow field can only be set to "needs review" if the body field contains some text. You fill out the body field, set the worklfow field to "needs review". Then you edit the entity again and empty the body field, but leave the workflow field at "needs review". Now the validation should trigger: "needs review" is now an invalid workflow state, although the field value was not changed. You would only run the validation on the body field (which has changed). You could argue that in such a case the validation must be done on the body field, but that does not seem right to me since the logic is in the domain of the workflow field.

You also have a problem if you have migrated some legacy data into Drupal which is not completely valid. If a required field is missing for example, then a user can edit the entity and submit an empty required field without getting validation errors, which is bad IMO. (the form API might catch this for a required field widget for us, but you can imagine other examples of invalid data)

Does not apply anymore, I'm going to reroll.

Rerolled, the change in EmailItem.php should not be necessary anymore.

Needs a reroll now that #2024963: Move baseFieldDefinitions from storage to entity classes is in

git ac https://drupal.org/files/user-validation-2002162-34.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 20246  100 20246    0     0  14803      0  0:00:01  0:00:01 --:--:-- 17559
error: patch failed: core/modules/user/lib/Drupal/user/UserStorageController.php:195
error: core/modules/user/lib/Drupal/user/UserStorageController.php: patch does not apply

Rerolled, not other changes.

1ms is fine here. klausi's right that this isn't the cause of our performance problems.

The commit contains changes in LocalActionManager.php that do not seem related ?

Doh. Reverted and re-committed.