Font collection registration: handling invalid slugs

[creativecoder]

Font collections are registered with a slug that is used as a unique identifier for the collection.

For example:

wp_register_font_collection( 'my-collection, ... )

I now reference that collection by its slug in PHP or using the REST API:

• WP_Font_Library::get_font_collection( 'my-collection')
• wp/v2/font-collections/my-colleciton

Collection slugs are sanitized with sanitize_title to create slugs that are url safe values appropriate to use as identifiers.

The question came up in this wordpress-develop PR, what is the appropriate level of error handling when a slug is invalid (the sanitized slug value does not match it's given value)?

Some possibilities include

• Triggering a _doing_it_wrong notice (this is what we're doing at the moment)
• Throwing an exception, which would error the entire request
• Storing a WP_Error instance somewhere and returning it at an appropriate time (perhaps when retrieving the slug?)

[creativecoder]

To capture some of the initial discussion in WordPress/wordpress-develop#6027 (comment)

From @hellofromtonya

A few questions before assessing if throwing an exception vs doing it wrong.

How critical is this check?

if ( $this->slug !== $slug )

Is it critical that the sanitized slug exactly match the given slug?

What happens if the 2 don't match? Does the collection not work?

If it's critical to functionality, then throwing an exception might be the way to go. If it's not critical and everything will continue to work properly, then it's informational.

If it's information, I then wonder: Is a doing it wrong necessary?

Answer from @creativecoder

What happens if the 2 don't match? Does the collection not work?

The collection will be registered and work, but a few potentially unexpected things happen. For example, if I use the slug invâlíd / slüg, e.g. wp_register_font_collection( 'invâlíd / slüg', ... )

• The $collection->slug value is now the sanitized value invalid-slug rather than invâlíd / slüg.
• WP_Font_Library::get_font_collection( 'invâlíd / slüg' ) returns an error, because a collection with that slug is not registered.
• wp/v2/font-collections/invâlíd / slüg (or it's uri encoded version) will return an error, because a collection with that slug is not found.

However, WP_Font_Library::get_font_collections and wp/v2/font-collections still returns the collection, but with a slug that is sanitized (invalid-slug). Additionally

• WP_Font_Library::get_font_collection( 'invalid-slug' ) returns the collection without error.
• wp/v2/font-collections/invalid-slug returns the collection without error.

[creativecoder]

Additionally from @hellofromtonya

Since it still works, thinking throwing an exception is too severe. The _doing_it_wrong() informs just in case unexpected things happen. Currently thinking it's fine as is.

[mcsf]

I agree that throwing is heavy-handed, and the more idiomatic WordPress thing would be to return an error, but this isn't possible inside a class constructor. But I still think the system should reject any collection with an invalid slug. We're building something brand new, there are no legacy concerns here, and so we should be as strict as possible.

The obvious possible approaches to me are:

• On instantiation, somehow mark the WP_Font_Collection instance if it's not a valid one. In the original PR I suggested assign a WP_Error instance to $this->data, for the sole reason that there is already a precedent in that class for data's polymorphism (in get_data, we see if ( is_wp_error( $this->data ) ) return $this->data). At the time I didn't realise that this was silly, because that variable is private. A more explicit way would be to introduce a public variable or method like is_valid that the caller of the constructor can check.
• Alternatively, work around the fact that class constructors cannot return other values by keeping the constructor private and mediating instantiation as follows:

class Foo {
	private function __construct($value) {
		if (! $value) {
			throw new ValueError();
		}
	}
	public static function instantiate($value) {
		try {
			return new self($value);
		} catch (ValueError $e) {
			return $e;
		}
	}
}
assert(Foo::instantiate(1) instanceof Foo);
assert(Foo::instantiate(0) instanceof ValueError);

Is it weird, ugly and unlike how conventions? Yes. So maybe something along the first approach is more suitable.

What matters is that, in the end, the only caller we have so far — WP_Font_Library::register_font_collection — should do the following:

	public function register_font_collection( $slug, $data_or_file ) {
		$new_collection = new WP_Font_Collection( $slug, $data_or_file );

+		if ( ! $new_collection->is_valid() ) {
+			$error_message = sprintf( '...' );
+			return new WP_Error( '...' );
+		}

		if ( $this->is_collection_registered( $new_collection->slug ) ) {
			$error_message = sprintf(
				/* translators: %s: Font collection slug. */
				__( 'Font collection with slug: "%s" is already registered.' ),
				$new_collection->slug
			);
			_doing_it_wrong(
				__METHOD__,
				$error_message,
				'6.5.0'
			);
			return new WP_Error( 'font_collection_registration_error', $error_message );
		}
		$this->collections[ $new_collection->slug ] = $new_collection;
		return $new_collection;
	}

[creativecoder]

$new_collection->is_valid()

One of the complications here is that it's desirable to lazy load font_families when they're provided in a json file, to delay the overhead of reading a (potentially large) file or making an http request until the data is needed.

So $new_collection->is_valid() during registration seems misleading, as it would only be checking the slug at this point.

We could add a slug specific method, e.g. $new_collection->is_slug_valid() that can be used during registration, but that seems clunky.

[mcsf]

Fair enough, I don't want to block this. I trust your judgement :)

[creativecoder]

I'm going to close this issue, since WP 6.5 has shipped and the _doing_it_wrong notice seems adequate, for now.