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

Use objects for validation errors #3875

Closed
schlessera opened this issue Dec 3, 2019 · 1 comment
Closed

Use objects for validation errors #3875

schlessera opened this issue Dec 3, 2019 · 1 comment
Labels
Groomed P2 Low priority Validation WS:Core Work stream for Plugin core

Comments

@schlessera
Copy link
Collaborator

Feature description

(Copied from #3780 (comment))

We should use objects for the different validation errors (like you would use separate objects for exceptions as well).

Benefits:

  1. Instead of passing arrays around with multiple types of information, we can pass a single object of type AMP_Validation_Error around.
  2. The objects can have proper constructors that know which type of data to collect for each error, allowing us to add type checking and other safeguards:
    return new DisallowedFirstChildError( $child_elements[0]->nodeName, $child_tags['first_child_tag_name_oneof'] );`
  3. If we have variations for a given error, we can have named constructors instead of the built-in one. (Don't think that is needed, though)
  4. All the display message generation can be encapsulated:
    foreach ( $validation_errors as $error ) {
    	$title = $error->get_title();
    	// [...]
    }
  5. Faster, as it replaces string comparisons with type checks:
    // if ( $validation_error['code'] === AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_ATTR ) {}
    if ( $validation_error instanceof DisallowedAttributeError ) {}
  6. Uses less memory, because most arrays with strings will be turned into internal properties instead.
  7. Removes big switch() statements like the one below. Whatever needs to be checked across all validation errors will be encapsulated as a method on them.
  8. I personally think it is much easier to read (see 5. as an example).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@kmyram kmyram added P1 Medium priority P2 Low priority and removed P1 Medium priority labels Mar 3, 2020
@westonruter
Copy link
Member

westonruter commented Mar 3, 2020

Will be important as part of #3730.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Groomed P2 Low priority Validation WS:Core Work stream for Plugin core
Projects
Archived in project
Development

No branches or pull requests

3 participants