wp_cache_set, wp_cache_add, set_transient should handle and notify on non-ints expirations

[andreasnrb]

Currently plugin and theme developers pass strings and other types as expirations to the wp_cache_set, wp_cache_add, set_transient instead of the int that is required.
This causes issues when one uses object-cache.php drop in which connects to APC or other caching backends.

There should be stricter control of int values in these functions. Typecasting to ints and a wrong param type notice/warning. Preferably warning so plugin/theme developers are made aware of their wrong usage of the functions.

[nacin]

I think 25308.2.patch​ is good here. If we wanted we could add:

if ( false !== $expiration && ! is_int( $expiration ) )
    _doing_it_wrong( __FUNCTION__, '$expire should be an integer.' );

But I am not sure it is needed.

[andreasnrb]

Replying to nacin:

I think 25308.2.patch​ is good here. If we wanted we could add:

if ( false !== $expiration && ! is_int( $expiration ) )
    _doing_it_wrong( __FUNCTION__, '$expire should be an integer.' );

But I am not sure it is needed.

Could do so the notification is only generated if WP_DEBUG is true it would at least limit the amount of notifications people get.

[tollmanz]

@andreasnrb Can you explain what types of problems this causes with external object caches? Are you finding fatal errors, broken applications, notices, etc?

I think it might be on the authors of the external object caches to implement the warning, type casting, or input validation/sanitization. Changing this in the core class would only serve to warning devs, but would not actually fix the problem.

[nacin]

_doing_it_wrong() only fires a notice when WP_DEBUG. I just wonder at what point would the vast majority of WordPress functions need to follow this pattern. PHP is loosely typed, and so is most of WordPress — if it's scalar, it's usually accepted for boolean and integer arguments, then cast.

I doubt anyone is honestly "doing it wrong" here. At most, maybe they're passing a numeric string? I don't think we need to be annoyed with that. We can just cast it and move on.

"Currently plugin and theme developers pass strings and other types as expirations" - any evidence for this to show just how bad things are?

[c3mdigital]

Replying to nacin:

"Currently plugin and theme developers pass strings and other types as expirations" - any evidence for this to show just how bad things are?

Quickly going through the ​plugin repo it looks like many developers are not even setting expirations. I couldn't find one using a string.

[andreasnrb]

When using drop in with for example APC, APC will throw message "apc_store() expects parameter 3 to be long, string given". No drop-ins for APC handles wrong expiration type as far as I know.

And with regards to setting it can be when the data is user input, users fills out a text field that is not validated. Also typecasting to 0 is not always the best thing since that would not give the developer any hint that they are passing a wrong expiration in the first place.

@tollmanzs
"Changing this in the core class would only serve to warning devs, but would not actually fix the problem."
The whole point is to warn devs. There are other areas in WP that call doing it wrong to notificy developers. End users don't care, they dont want any messages at all. At least by adding this there is some covering of ones bases. The point is to give devs in case they do it wrong a heads up. It breaks intended functionality when it happens.

@c3digital
"Currently plugin and theme developers pass strings and other types as expirations"
Should have written "can" and in some cases it does happen. Cant do a search like that you need to check variables and where they originate from. Point is APC and perhaps other cache engines don't accept strings or other types. They give warning. I think WordPress should do the same to promote better developer methodology.

[c3mdigital]

Replying to andreasnrb:

@c3digital
"Currently plugin and theme developers pass strings and other types as expirations"
Should have written "can" and in some cases it does happen. Cant do a search like that you need to check variables and where they originate from. Point is APC and perhaps other cache engines don't accept strings or other types. They give warning. I think WordPress should do the same to promote better developer methodology.

I'm not saying it can't happen or doesn't but from the 30 or 40 plugins I looked at, the developers either didn't set an expiration, assigned an integer to a variable (yes I traced the origin ) like $expiration = 60 * 60 * 10, or put an integer in directly.

[nacin]

In 25451:

Always pass integer expirations to cache backends.

props SergeyBiryukov, andreasnrb.
fixes #25308.