PopupMaker/Popup-Maker

Regression: line 125 of classes/AssetCache.php has broken caching in recent versions

robsheldon opened this issue · 2 comments

Hi,

Commit dda3314 introduced a bug on line 125 of classes/AssetCache.php:

if ( true !== get_option( 'pum_files_writeable', true ) ) {

If a strict comparison is required here, then the line needs to be changed to:

if ( '1' !== get_option( 'pum_files_writeable', true ) ) {

See also https://developer.wordpress.org/reference/functions/get_option/, which states that boolean true values get stored as a string(1) "1" value in wp_options, and "1" is what get_option() returns after that.

I've manually confirmed this behavior.

This bug is causing recent versions of Popup Maker to incorrectly mark its cache as not writeable.

Thanks!

Looks like line 663 of the same file may have the same bug too:

return true === get_option( 'pum_files_writeable', true ) || true === get_option( '_pum_writeable_notice_dismissed', true );

@robsheldon Thanks for the report, marking this one closed as it has already been patched in the develop branch.

Your correct, and the final solution was to coerce the get_option into a boolean for the strict comparison.

https://github.com/PopupMaker/Popup-Maker/blob/develop/classes/AssetCache.php#L715-L717

We are pushing PHPCS cleanup, and this one got by us. We have a review going over all the other changes to strict comparisons looking for anything comparing boolean to possibly non-boolean values, where similar fixes will be employed.