onOffice-Web-Org / oo-wp-plugin

onOffice for WP-Websites
https://wp-plugin.onoffice.com
GNU General Public License v3.0
9 stars 9 forks source link

WPScreenWrapperDefault::getID() breaks other plugins #96

Closed walzph closed 2 years ago

walzph commented 3 years ago

Hi there, I noticed that several WordPress Plugins (e.g. WPForms, MonsterInsights) crash when I want to start their setup wizard.

The PHP logs pointed me to to a TypeError in onOffice\WPlugin\WP\WPScreenWrapperDefault::getID()

PHP Fatal error:  Uncaught TypeError: Return value of onOffice\WPlugin\WP\WPScreenWrapperDefault::getID() must be of the type string, null returned in /var/www/html/wp-content/plugins/onoffice-for-wp-websites/plugin/WP/WPScreenWrapperDefault.php:58
Stack trace:
#0 /var/www/html/wp-content/plugins/onoffice-for-wp-websites/plugin/WP/ListTableBulkActionsHandler.php(75): onOffice\WPlugin\WP\WPScreenWrapperDefault->getID()
#1 /var/www/html/wp-content/plugins/onoffice-for-wp-websites/plugin/Controller/AdminViewController.php(219): onOffice\WPlugin\WP\ListTableBulkActionsHandler->processBulkAction()
#2 /var/www/html/wp-includes/class-wp-hook.php(303): onOffice\WPlugin\Controller\AdminViewController->onOffice\WPlugin\Controller\{closure}()
#3 /var/www/html/wp-includes/class-wp-hook.php(327): WP_Hook->apply_filters()
#4 /var/www/html/wp-includes/plugin.php(470): WP_Hook->do_action()
#5 /var/www/html/wp-admin/includes/class-wp-screen.php(421): do_action()
#6 /var/www/html/wp-admin/includes/screen.php(243): WP_Screen->set_current_screen( in /var/www/html/wp-content/plugins/onoffice-for-wp-websites/plugin/WP/WPScreenWrapperDefault.php on line 58

I fixed it by rewriting the getID() function

public function getID(): string
{
   $id = $this->_pWPScreenFactory->getCurrentScreen()->id;
    if (is_null($id)) {
        return '';
    }
    return $id;
}

If this fixes a general bug, I can open a PR for it.

jayay commented 3 years ago

Hey and thank you for the bug report! I was able to reproduce the issue with the MonsterInsights plugin. It's good you found a fix, but I think it'd be good to dig and find out why the screen ID is null.

j-maas commented 3 years ago

This also happens with the Polylang plugin when running its Setup Wizard. I get the exact same error as walzph.

jmaas-onoffice commented 2 years ago

I can no longer reproduce this issue with plugin version 3.1. I installed Polylang, WPForms, and MonsterInsight and all setup wizards opened without a crash.

If you still have an issue, please open a new issue and tell us the plugin.