Add LLMS_Media_Protector class.

[thomasplevy]

@pondermatic I'm opening this on your behalf so I can record some notes and thoughts I'm having while I'm working through this

[thomasplevy]

I'll get Chris to get our design team to make an asset for this. For now use some CC0 image as a placeholder.

[pondermatic]

@thomasplevy, I've made a bunch of changes to this WIP based on the above code review.

[eri-trabiccolo]

@thomasplevy @pondermatic
haven't really looked at the PR logic, but I just want to highlight something about the type hinting:

1. I love it :D
2. We should be careful about using it for hook callbacks because if a 3rd party passes a wrong type along, the fatal will happen in OUR code. We can then blame others, but still we will be blamed first by our customers.

What do you think?

[pondermatic]

@eri-trabiccolo wrote:

We should be careful about using it (parameter type declaration) for hook callbacks because if a 3rd party passes a wrong type along, the fatal will happen in OUR code.

Scalar values (bool, int, float, string) passed to a parameter declared with a different scalar type will be coerced into the declared type.

If the script that calls a function in our script has strict mode enabled, or the passed type or declared type are not scalar (array, object, etc.), a TypeError is thrown that will mention our code and the caller's code, plus a stack trace.

PHP Fatal error:  Uncaught TypeError: LLMS_Media_Protector::authorize_media_image_src(): Argument #1 ($image) must be of type int, array given,
called in wp-includes/class-wp-hook.php on line 303 and defined in wp-content/plugins/lifterlms/includes/class-llms-media-protector.php:211
Stack trace:
#0 wp-includes/class-wp-hook.php(303): LLMS_Media_Protector->authorize_media_image_src(Array, 1136, 'thumbnail', false)
#1 wp-includes/plugin.php(189): WP_Hook->apply_filters(Array, Array)
#2 wp-includes/media.php(991): apply_filters('wp_get_attachme...', Array, 1136, 'thumbnail', false)
#3 wp-content/themes/astra/inc/blog/blog-config.php(360): wp_get_attachment_image_src(1136)
...

I put the parameter type declarations in there to explore their usefulness but am prepared to take them out if we think it'll cause problems.

Because of the lack of the union types, e.g. int|WP_Error or mixed, in PHP 7, I'm thinking we:

1. Wait until PHP 8 is our minimum requirement and then we start adding parameter and return type declarations.
2. We don't declare types for a value that uses multiple types with at least one type being non-scalar.

I like that parameter type declarations remove the need for a method to manually check the type or coerce it into another type.

If a 3rd party always calls one of our public methods with the wrong parameter types, they'll find out quickly during their development. I'm concerned that if a caller sometimes passes something like '1' instead of 1 because they are relying on PHP's type juggling, then the problem won't be noticed until it is released into a production environment and someone will blame us for being too strict.

@thomasplevy, what do you think about type declarations? Is this the right time to start using them?

[eri-trabiccolo]

@eri-trabiccolo wrote:

We should be careful about using it (parameter type declaration) for hook callbacks because if a 3rd party passes a wrong type along, the fatal will happen in OUR code.

Scalar values (bool, int, float, string) passed to a parameter declared with a different scalar type will be coerced into the declared type.

Well, that's partially true:
http://sandbox.onlinephpfunctions.com/code/ccbe0c1f3a60a6179cc37f8d32f35f44da1a795b

Although I think that you're right, in most of the cases we'll get a string representing a number where we'd want an int and PHP will just cast it.

If the script that calls a function in our script has strict mode enabled, or the passed type or declared type are not scalar (array, object, etc.), a TypeError is thrown that will mention our code and the caller's code, plus a stack trace.

PHP Fatal error:  Uncaught TypeError: LLMS_Media_Protector::authorize_media_image_src(): Argument #1 ($image) must be of type int, array given,
called in wp-includes/class-wp-hook.php on line 303 and defined in wp-content/plugins/lifterlms/includes/class-llms-media-protector.php:211
Stack trace:
#0 wp-includes/class-wp-hook.php(303): LLMS_Media_Protector->authorize_media_image_src(Array, 1136, 'thumbnail', false)
#1 wp-includes/plugin.php(189): WP_Hook->apply_filters(Array, Array)
#2 wp-includes/media.php(991): apply_filters('wp_get_attachme...', Array, 1136, 'thumbnail', false)
#3 wp-content/themes/astra/inc/blog/blog-config.php(360): wp_get_attachment_image_src(1136)
...

Yeah, and I also assume that this 3rd party script, since it's developed by someone who cares about types, will take care of respecting them :D

I put the parameter type declarations in there to explore their usefulness but am prepared to take them out if we think it'll cause problems.

I love them, I think there's a misunderstanding here :D I said - or I meant to say that - we should be careful ONLY for which regards to hooks callbacks. I don't care about all our public methods - who uses them should pass the expected type - I'm talking about wp core filters mostly.

Because of the lack of the union types, e.g. int|WP_Error or mixed, in PHP 7, I'm thinking we:

1. Wait until PHP 8 is our minimum requirement and then we start adding parameter and return type declarations.
2. We don't declare types for a value that uses multiple types with at least one type being non-scalar.

I'm for 2.

I like that parameter type declarations remove the need for a method to manually check the type or coerce it into another type.

I agree, but I still think this is just something we cannot took for granted when we talk about WP hook callbacks.

If a 3rd party always calls one of our public methods with the wrong parameter types, they'll find out quickly during their development. I'm concerned that if a caller sometimes passes something like '1' instead of 1 because they are relying on PHP's type juggling, then the problem won't be noticed until it is released into a production environment and someone will blame us for being too strict.

I agree, I don't care about 3rd parties calling our public methods with the wrong parameter type. See above.

@thomasplevy, what do you think about type declarations? Is this the right time to start using them?

[pondermatic]

@pondermatic wrote:

Scalar values (bool, int, float, string) passed to a parameter declared with a different scalar type will be coerced into the declared type.

@eri-trabiccolo wrote:

Well, that's partially true:
http://sandbox.onlinephpfunctions.com/code/ccbe0c1f3a60a6179cc37f8d32f35f44da1a795b

OMG that's right! Not all types are coerced. In fact, it seems strange that some are and some are not. I wrote a script to test all the combinations and am planning on making a blog post about it (off-the-clock).

You have good points @eri-trabiccolo.

1. It is OK that anyone calling our methods must obey the type declaration.
2. We should not have declared types on hook callbacks because we can't trust that other callbacks using the same hook will pass the correct type. We can't even be sure that a WordPress hook will always pass the correct type.

[pondermatic]

@gocodebox/engineering
Blog post about parameter type declarations and what happens when the wrong type is passed in.
https://www.pondermatic.com/2021/12/06/php-parameter-type-declarations/

[thomasplevy]

I'm in favor of type declarations but I don't like adding them with a bunch of exceptions (mainly there are no union types)... I don't like the idea of only declaring some types, it just feels sloppy... But I guess in reality it's less sloppy than not declaring any.

I know that if we start declaring them on callbacks for hooks we will run into issues. Let's be honest that most people using hooks don't actually know how to write PHP... We already have to explain code snippets we share (to developers) in far more detail that seems reasonable... if we also have to add explaining type declarations... well... you know...?

2. We don't declare types for a value that uses multiple types with at least one type being non-scalar.

And not declaring types on functions/methods we internally use as a hook callback.