500 error on every page with read-only file system

[omarjackman]

Describe the bug

The plugin causes a 500 error on every page when the uploads directory is not writable

PHP version: 8.2

Expected behavior

The page should load

Current behavior

A 500 error is displayed to the user and the error log shows an out of memory error

Steps to reproduce

1. Make the uploads directory read-only. It is important that wp_upload_dir return an error.
2. load any page with this plugin enabled
3. notice that the page hangs

Additional context

After debugging I found that the error is caused by an infinite loop that starts at https://github.com/PopupMaker/Popup-Maker/blob/v1.18.2/classes/Utils/Logging.php#L82 Heres what happens:

1. The logger class initializes and tries to get the upload dir within its constructor via PUM_Helpers::get_upload_dir.
2. The PUM_Helpers::get_upload_dir detects that there was an error when calling wp_upload_dir so it then tries to log the error here.
3. Since the constructor never finished executing the first instance of the logger, a new instance is created and we start over at 1 again.

[danieliser]

@omarjackman - First, why would the uploads directory ever be read only, that doesnt seem correct for the WP core specs Seems to indicate a deeper problem than just our plugin.

Will see if there is a simple fix to multiple instances being spawned, seems simple enough stop gap.

[danieliser]

Does the version of PHP being 8.2 actually matter, or does it occur on others as well?

[omarjackman]

@omarjackman - First, why would the uploads directory ever be read only, that doesnt seem correct for the WP core specs Seems to indicate a deeper problem than just our plugin.

Will see if there is a simple fix to multiple instances being spawned, seems simple enough stop gap.

Thanks for looking into the issue. The "why" shouldn't matter, my use-case has the file system as read-only and enabling this plugin caused every request to the site to go into an infinite loop. If it helps, forget about my use-case and just imagine any other scenario where wp_upload_dir returns with an error.

[omarjackman]

Does the version of PHP being 8.2 actually matter, or does it occur on others as well?

It probably doesn't matter but the issue template asked for the version of PHP I was using

[danieliser]

Saw you mention it originally in issue title, just wanted to confirm.

[dougaxe1]

This could partially be fixed by a solution to #1043 which bypasses all filesystem checks if logging is disabled (define( 'PUM_DISABLE_LOGGING', true )). One would presumably set that constant if the uploads directory is known to be read-only. It doesn't account for other temporary error conditions, however.

[danieliser]

Constant should now be respected, and we check that the method exists before using it in these sections of code.