#4142003: Critical error - ScriptLoader

pospisk commented 7 months ago

As a user of the onOffice for WP-Websites plugin, I encounter critical errors caused by the Script Loader. One such error occurs wwhen the str_replace() function is called with an invalid argument, leading to a fatal error message and impactic the plugins functionality.

Plugin Version 4.18.1
PHP Version: 8.1.23
WP Version: 6.4.3

File:

https://github.com/onOffice-Web-Org/oo-wp-plugin/blob/e871c0ba2f42d4342bd1ddc571c0f3e773cff700/plugin/ScriptLoader/ScriptLoaderGenericConfigurationDefault.php#L134

Error:

Fatal error: Uncaught TypeError: str_replace(): Argument #3 ($subject) must be of type array|string, null given in ../wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderGenericConfigurationDefault.php:134

To ensure the stability and reliability of the plugin and address the critical error we propose the following changes:

Current:

$metaValue = str_replace('\u0022', '"', $metaValueArray[0]);

Fix example:

$metaValue = '';
if (isset($metaValueArray[0])) {
    $metaValue = str_replace('\u0022', '"', $metaValueArray[0]);
}

Tasks

[x] Analyze the faulty code in the Script Loader to identify the root cause of the issue
[x] Fix the error by implementing the suggested fix
[x] Conduct comprehensive testing to ensure that the changes resolve the critical error and do not introduce new issues

By resolving this critical error, users will be able to use the onOffice for WP-Websites plugin without interruptions and benefiting from improved stability.

fredericalpers commented 7 months ago

@dai-eastgate @yeneastgate if possible please go ahead and try to fix this before any other issues in this cycle. thank you :)

dai-eastgate commented 7 months ago

@fredericalpers I got it

dai-eastgate commented 7 months ago

Analyze the faulty code in the Script Loader to identify the root cause of the issue

@fredericalpers @pospisk Here is my analysis of the root cause of the issue. If $metaValueArray = null.

This is because str_replace() requires the type of parameter passed to be a string, but currently, it is receiving a value of $metaValueArray[0] is null.
Currently, we retrieve all metakeys from the wp_postmeta table to get the shortcode embedded in the advanced custom field.
There will be no user behavior here leading to $metaValueArray[0] = null in the case of using a custom field because when we do not enter the value of the custom field, the return value is an empty string.
Therefore, it may be due to another user action that saved the meta_value as null.

=> This reason is outside the scope of the function we are using, so it is not a problem, and the current code fix is only to eliminate this case.

pospisk commented 7 months ago

@dai-eastgate We are missing the main plot of how the customer has stumbled upon this error. However, when working with plugins, especially in a WordPress context, the plugin's responsibility includes ensuring robustness and compatibility across various environments and usage scenarios. This means even if an issue arises from what could be considered "misuse" or an "edge case," it is within the plugin's scope to handle such cases gracefully. The current customer uses wordpress.com for hosting and when replicated in a local environment the error still persists. Since the error itself is only due to a preventive guard clause being missing it should fall within the plugins scope.

Reasoning: If a customer changes a setting and causes a critical error it is therefor not possible to unset the changes due to the critical error. It requires support maintenance to be resolved.

The full stack trace:

Fatal error: Uncaught TypeError: str_replace(): Argument #3 ($subject) must be of type array|string, null given in /Users/pospisk/Local Sites/molt/app/public/wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderGenericConfigurationDefault.php:134 Stack trace: #0 /Users/pospisk/Local Sites/molt/app/public/wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderGenericConfigurationDefault.php(134): str_replace('\\u0022', '"', NULL) #1 [internal function]: onOffice\WPlugin\ScriptLoader\ScriptLoaderGenericConfigurationDefault->onOffice\WPlugin\ScriptLoader\{closure}(Array, '_starter_page_t...') #2 /Users/pospisk/Local Sites/molt/app/public/wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderGenericConfigurationDefault.php(142): array_walk(Array, Object(Closure)) #3 /Users/pospisk/Local Sites/molt/app/public/wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderGenericConfigurationDefault.php(327): onOffice\WPlugin\ScriptLoader\ScriptLoaderGenericConfigurationDefault->getShortcodeByPostMeta() #4 /Users/pospisk/Local Sites/molt/app/public/wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderGenericConfigurationDefault.php(72): onOffice\WPlugin\ScriptLoader\ScriptLoaderGenericConfigurationDefault->renderStyleForEstateDetailPage('/Users/pospisk/...', 'style') #5 /Users/pospisk/Local Sites/molt/app/public/wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderGeneric.php(100): onOffice\WPlugin\ScriptLoader\ScriptLoaderGenericConfigurationDefault->getScriptLoaderGenericConfiguration() #6 /Users/pospisk/Local Sites/molt/app/public/wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderGeneric.php(79): onOffice\WPlugin\ScriptLoader\ScriptLoaderGeneric->getModelByType('script') #7 /Users/pospisk/Local Sites/molt/app/public/wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderRegistrator.php(79): onOffice\WPlugin\ScriptLoader\ScriptLoaderGeneric->register() #8 /Users/pospisk/Local Sites/molt/app/public/wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderRegistrator.php(61): onOffice\WPlugin\ScriptLoader\ScriptLoaderRegistrator->register() #9 /Users/pospisk/Local Sites/molt/app/public/wp-includes/class-wp-hook.php(324): onOffice\WPlugin\ScriptLoader\ScriptLoaderRegistrator->onOffice\WPlugin\ScriptLoader\{closure}('') #10 /Users/pospisk/Local Sites/molt/app/public/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters(NULL, Array) #11 /Users/pospisk/Local Sites/molt/app/public/wp-includes/plugin.php(517): WP_Hook->do_action(Array) #12 /Users/pospisk/Local Sites/molt/app/public/wp-includes/script-loader.php(2262): do_action('wp_enqueue_scri...') #13 /Users/pospisk/Local Sites/molt/app/public/wp-includes/class-wp-hook.php(324): wp_enqueue_scripts('') #14 /Users/pospisk/Local Sites/molt/app/public/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters(NULL, Array) #15 /Users/pospisk/Local Sites/molt/app/public/wp-includes/plugin.php(517): WP_Hook->do_action(Array) #16 /Users/pospisk/Local Sites/molt/app/public/wp-includes/general-template.php(3052): do_action('wp_head') #17 /Users/pospisk/Local Sites/molt/app/public/wp-includes/template-canvas.php(17): wp_head() #18 /Users/pospisk/Local Sites/molt/app/public/wp-includes/template-loader.php(106): include('/Users/pospisk/...') #19 /Users/pospisk/Local Sites/molt/app/public/wp-blog-header.php(19): require_once('/Users/pospisk/...') #20 /Users/pospisk/Local Sites/molt/app/public/index.php(17): require('/Users/pospisk/...') #21 {main} thrown in /Users/pospisk/Local Sites/molt/app/public/wp-content/plugins/onoffice-for-wp-websites/plugin/ScriptLoader/ScriptLoaderGenericConfigurationDefault.php on line 134

https://github.com/onOffice-Web-Org/oo-wp-plugin/pull/767/files/eecc70b62cf9b9828422451745be66a386b00ea1..435548e2c4855bdbfc0cd613d404ae13f432e641

Fix

    if (!is_array($metaValueArray) || empty($metaValueArray) || !isset($metaValueArray[0])) {
            return;
        }
        $metaValue = str_replace('\u0022', '"', $metaValueArray[0]);

Fix

$metaValue = '';
        if (isset($metaValueArray[0])) {
            $metaValue = str_replace('\u0022', '"', $metaValueArray[0]);
        }

Both fixes resolve the critical error, one of them has to make it into the next release.

dai-eastgate commented 7 months ago

@pospisk Yes,I understand the importance of the plugin's responsibility includes ensuring robustness and compatibility across various environments and usage scenarios. I will attention to check this condition when using variables. Thank you for sharing your knowledge!

onOffice-Web-Org / oo-wp-plugin