calendar_week_mod function is not type safe

[dingo_d]

While checking for the ​list of autoEscapedFunctions in WPCS I noticed that the function calendar_week_mod is not tested and not type safe.

If you pass a non-numeric string you'll get a fatal error on PHP 8+ and a warning on PHP <8 (and a 0 returned): ​https://3v4l.org/SLlHH

The proposed action is to:

1. Write tests that will cover things like ​numeric strings, ​integers, ​floats, ​strings, and all the other types
2. Refactor the function in a non-BC way (add type checks and gracefully exit)

The core impact change of this refactor is small, since it's only used in one place. The plugins directory search shows ​193 matches and only ​10 matches for themes.

[malthert]

I think this is not an issue. The docs state the arg must be int. If you pass a non-int argument, it's expected to a warning/fatal, just like with many regular PHP functions (e.g. pass a string to array_pop and you'll get the same thing)

Additionally, it returns a float in 100% of cases anyway.

Given the instrumentation modern PHP gives (strict types,...) I think it's not worth to cover every aspect and add tons of redundant conditions, that are flagged by every basic CI under the sun anyway.

[hellofromTonya]

The function calendar_week_mod() is documented as requiring a int. Passing anything else is doing it wrong. I agree with @malthert.

The topic of input (parameter) validation is more of an architectural discussion. I think (maybe) there's a Trac ticket for it (if no, will create it). Rather than adding the validation in this one function, I think a project wide discussion and consensus needs to happen first. Then the implementation can be applied to this function and all others in Core.

I do agree with you @dingo_d that defensive coding is better and that tests are needed. Would you be interested in creating the unit tests?

Rather than solving type safe in one ticket for one specific function, I think this topic needs a project-wide architectural proposal, discussion, and consensus. Then once there's consensus of a way forward, the changes can be applied to all Core functions. I think (maybe) there's a Trac ticket open for this architectural discussion, though I'm currently not finding it. @jrf likely knows off the top of her head. If the ticket doesn't currently exist, it is something that has been discussed and thus a ticket and/or proposal will be coming.

I'll close this ticket as I agree it's not an "issue" to solve at an individual function level, but am marking it as maybelayer.

Thank you for the suggestion @dingo_d!

[jrf]

I think (maybe) there's a Trac ticket open for this architectural discussion, though I'm currently not finding it.

I can't remember a project/task ticket being open for this. I think a Make post would probably be the way to go anyway as any ticket would be inactionable before a strategy has been agreed upon.

Would it be an idea to keep this ticket open for adding tests to the calendar_week_mod() function ? (without - at this time - addressing the defensive coding issue)