MinnPost / object-sync-for-salesforce

WordPress plugin that maps and syncs data between Salesforce objects and WordPress objects.
https://wordpress.org/plugins/object-sync-for-salesforce/
GNU General Public License v2.0
93 stars 48 forks source link

Prevent error on blank WordPress install caused by a string instead of an integer. Thanks to GitHub user @federicojacobi for the pull request. #533

Closed federicojacobi closed 1 month ago

federicojacobi commented 1 month ago

What does this Pull Request do?

In this particular case, $schedule_number must be a number because in line 393 it is being multiplied. I'm not sure if it is a PHP8 thing, but you cannot multiple an int ($seconds) by a string (empty or otherwise). $schedule_number must be a positive int, just casting isn't enough, absint should cover it.

How do I test this Pull Request?

I only noticed this on a blank WP install, if you turn logging on it'll throw a fatal as no schedules have ever been set.

jonathanstegall commented 1 month ago

@federicojacobi thanks for the pull request. I'm happy to merge it when I get a bit of time to look over it (nobody is currently working on the plugin, but I try to keep an eye on it when I can).

If I'm reading it correctly, it should be possible to use $schedule_number = filter_var( get_option( $this->option_prefix . 'logs_how_often_number', '' ), FILTER_VALIDATE_INT ); instead of absint, right? I'm asking just because that is much more commonly what this codebase does, so it makes sense to me to try to keep it consistent if possible.

federicojacobi commented 1 month ago

@jonathanstegall yes, it would almost be the same thing. filter_var in your example would allow negative numbers where absint wouldn't, but it would be difficult to get that option to be negative to begin with. So yeah, it is basically the same thing.

Personally, I find absint more readable, but if you are striving for consistency ... 👍🏽