jrfnl/20210917-notes-on-input-validation.md

## 20210917-notes-on-input-validation.md

      
    Raw
  

              20210917-notes-on-input-validation.md
            
          
    What is input validation ?

Checking for a variable/ array index:

Does it exist ?
Is it of the right type ?
Is the value usable for our purposes ?

Problem definition:


Lack of input validation throughout most of Core.
... which is especially problematic in the context of filter hooks / callbacks.
... especially as the error will be thrown for a different plugin/Core than the one causing it.
Also see: https://core.trac.wordpress.org/ticket/51525

Larger context:


PHP is moving from a loose type language towards stricter typing, so more problems along the lines of what we've seen in PHP 8.0 and 8.1 are expected for the future.
Anything deprecated in PHP 8.x will become unsupported (catchable fatal error) in PHP 9.0.
Error levels in PHP - a "doing it wrong" would be raising the error level compared to letting a deprecation be
Ref: https://www.php.net/manual/en/errorfunc.constants.php
Scalar type declarations is PHP 7.0, nullable 7.1 (union types 8.0, intersection 8.1)
Adding type declarations to public/protected class methods in non-final classes is a signature change for any class which extends.
Global namespace functions or private class methods can get type declarations.
Type juggling will happen, unless declare(strict_typing=1) is set.

Limitations:


WP does not use semantic versioning
... and doesn't allow BC breaks.

Challenges


Create solutions which can be freely applied to new code.
Create stop-gap solutions for old code.

Questions:

Is fixing these deprecations a BC-break ? or is it a bug fix ?


Add type declarations:

Pro:

Makes code more self-documentation
Removes need for in-function type validation
Would gracefully type juggle for scalar types (as long as strict_types is not used)

Con:

Most types not currently available in min PHP version.
Adding types in public/protected methods in non-final classes is a signature changes.
Would throw fatal error for non-compliant variables when a non-scalar type is used.
But: that is a CATCHABLE error.

Prerequisites:

PHP 7.1 minimum (for nullable)
filter callback fatal type errors would still need to be handled in Core, again, see the type safe filter ticket

Type cast everything

Pro:

Simple solution

Con:

HIDES the errors from the devs, while they should be fixed instead.
Will actually not prevent type errors for non-scalar.
Will hide opportunities for performance optimization.
Might actually change behaviour.
Might actually cause unexpected behaviour. Think (array) false does not yield an empty array, but array( 0 => false )

Use castable objects

As a form of input validation, pass the input to a new object of a particular castable type, like CastableString, CastableCallable, CastableACFField.
Pro:

Consistent handling across PHP versions
Consistent graceful handling of "uncastables"

Con:

Adds a layer complexity, so it would also need sufficient developer education to prevent it from being unused.
Other than that, similar cons to "type cast everything".

Use ValueObjects where applicable

Pro:

Consistent objects
Can contain good safeguards for things which would otherwise be an array
Will make the path to Type declaration easier (object type declarations are supported in PHP 5.x after all).

Con:

BC break with no good path for migrations

[Not viable] Emulate type declarations via ValueObjects in PHP 5.6

Pro:

Could mean the start of adding type declarations earlier.

Con:

Those classes could only be declared in PHP 5.6, as of PHP 7.0, the names are reserved keywords.
A "string string" or a "Stringable ValueObject" are not interchangeable.

Add in-function input validation

Pro:

Will allow for throwing _doing_it_wrong's - allows for gracefully handling different errors and all error levels.
Allows for consistent (though higher level) errors for devs by using _doing_it_wrong()
Puts the necessary safeguards in place

Con:

Performance overhead
As it adds logic to the function, requires lots of tests to prevent regressions.
May need extra documentation to explain why this is needed, makes the code less self-explanatory.

Notes:

Also needed for verify the return type after things like apply_filters(), not just and only for the initial inputs.

Callbacks (with apply_filters) - have type_safe variants which check returned values as well for type

Pro:

Makes return values reliable.
Makes life easier on extenders - no extra type validation needed + can add type declarations.
Pinpoints the place where things were done wrong exactly instead of blaming the next callback.

Con:

This is only a very partial solution - only for apply_filters() and only for (initially) single type filters.

Wrap everything within a function within a try-catch

Pro:

It would probably™ work.

Con:

Anti-pattern.
When doing this everywhere it will impact performance.
Over the top solution which isn't needed everywhere.
Deprecations, notices and warning are not catchable.
Only way to catch those would be by registering a global error handler which would throw catchable exceptions.
Hides errors again, unless we throw them anyway, but then it would be a BC-break and raising of error level, which is kind of the opposite of what we want.
Shitload of work to add this everywhere.

Summary assessment

In our estimated opinion, we would like to advise that a combination of:

In-function input validation, in line with PHP type validation, as a stepping stone to...
Adding actual type declarations where possible and when possible.

In combination with type safe filter functions and a move towards filters used in Core becoming single type, in so far possible, would be the most likely road to successfully handling this.
Implementation suggestion


Deprecate old functions and let those handle their own input validation
Introduce new wp_ prefixed functions with type declarations where we let PHP handle it.

Example code:
https://speakerdeck.com/jrf/for-the-love-of-code-modernizing-wordpress-plugins-and-themes-full-deck?slide=58
Pro:

Mitigates BC-breaks as much as possible
Ensures that more functions will be prefixed (less risk of conflict with PHP).
Could even consider "poor mans namespacing", i.e static functions in a class to get out of the global namespace.

Cons:

Introduces a lot of newly deprecated functions, so package size will grow.
Will require plugins/themes to update their codebases to use the new functions.

Side-line discussions:


Maintaining a LTS and an active more modern version of WP
Improving the _doing_it_wrong() to provide more info when the error has been lowered.
How long should we carry around deprecated functions and classes and files ?
Coupled with the discussion on how far should security fixes be backported ? And possible strict-coupling those.
I.e. at this time, everything deprecated before WP 3.7 "should" be safe to remove.
Site-health: how can we empower the team to reactivate and possible backport some of the messaging further back to reach, especially the real stragglers ? both for WP as well as PHP.