Error log tool is broken in WordPress VIP Setup

[shreyasikhar]

Describe the bug

When the Popup Maker plugin is enabled on the WordPress VIP platform, it breaks with a fatal error

The reason for the above error is condition 'direct' === $wp_filesystem->method on line https://github.com/PopupMaker/Popup-Maker/blob/master/classes/Utils/Logging.php#L119 failing, as with WordPress VIP the value of '$wp_filesystem->method is 'vip'.

So, I think we can add extra checks to avoid calling the filesystem methods with a bool.

Site information

Popup Maker version: 1.18.1

WordPress version: 6.1.1

PHP version: 8.1.9

Expected behavior

The error log tool should show logs without throwing a fatal error.

Current behavior

The error log tool is throwing some fatal error due to incompatibility with WordPress VIP platform.

Steps to reproduce

1. Setup a WordPress VIP instance (Reference docs)
2. Install and Activate the Popup Maker plugin
3. Go to Popup Maker -> Tools -> Error log from the WP-Admin screen.

[danieliser]

@shreyasikhar Appreciate the report and PR. Can you confirm this change does in fact resolve the error?

[shreyasikhar]

Thank you @danieliser

Yes, it resolves the fatal error attached in the above screenshot and the changes from linked PR won't break other sites as this is just an extra check for the object before calling its methods.

[ssapire]

Hi @danieliser I am not the technical person on our team, but we enabled the Popup-Maker plugin on our site. I clicked some sort of update or cache link related to your plugin and our website went down. Our developer has indicated that this is the issue related to why that happened and they won't reenable this until the bug is fixed. I am following here to make sure we know when it can be addressed. Thanks!

[danieliser]

@shreyasikhar - As I mentioned before the solution you provided isn't actually fixing things, likely breaking popups entirely.

To be clear your code simply bypassed the file writes, but let the plugin still finish the routines as if those files were written. Thus not loading the alternative files that would normally be used if those asset cache files couldn't be written or found.

You can simply disable asset caching in the plugin's settings page already and accomplish that in a more reliable way. That said there are penalties for doing so, our plugin will go from loading a single css & js file to multiple and inlining CSS in the head as well.

That said I've got VIP devs aware of this ticket and so far I haven't heard yet that we should be making changes on our end to make this work, but still waiting for confirmation up the chain and will work with them to make sure we do our part.

Moving on to the actual issue.

From both the error message itself and from VIP docs, it appears they intentionally took out normal WP functionality that should be expected to be there without replacing it. We can't necessarily go by what VIP does as we follow the WordPress Core docs/guidelines and this is a break in that.

Further their own docs insinuate it is likely on the site owner/admin to add that snippet of code I linked to before to your site to make the needed values be available for plugins if you need them.

If they say we should add it on our end we will, but I'm deferring to VIP currently as changes to the asset cache or manipulating the underlying WP_Filesystem vars is not something we are looking to do honestly.

[danieliser]

@ssapire - See my comments above but you have 2 options today:

1. Add the code VIP said to add in their docs: https://docs.wpvip.com/technical-references/vip-go-files-system/media-uploads/#h-wp-filesystem-api
2. Disable asset caching in our plugin entirely via the Popup Maker -> Settings page.

[dougaxe1]

Hello, @danieliser. I just tried the second option above and on VIP + Popup Maker 1.18.2, and unfortunately the Popup Maker -> Tools page (/wp-admin/edit.php?post_type=popup&page=pum-tools) still throws the fatal error described in this ticket.

Our client reported to us another functional area broken by this issue in the preview functionality. Attempting to preview a popup throws a full-screen fatal error:

We have even attempted to disable logging functionality with define( 'PUM_DISABLE_LOGGING', true ), however this too does not prevent the filesystem check (but it should).

I can test the VIP WP Filesystem code, but with logging disabled, it should short circuit all filesystem checks altogether.

[danieliser]

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