Check filterable values to make sure they have necessary structure/attributes before using them

[charmoney]

Describe the bug
Noted this week on a site running 1.8.6 after user reported inconsistent sync behavior sporadically failing to sync with no SF-side error message provided.

To Reproduce
Steps to reproduce the behavior are uncertain unfortunately. Bidirectional field map for User records. SF to WP field map for a custom post type with a large number of meta fields.

Expected behavior
Plugin should not throw PHP Fatal errors (and ideally not undefined indexes).

Screenshots
Three distinct PHP error log sections from yesterday that encounter the fatal error, as well as a variety of undefined index and non-object notices.

[16-Jul-2019 00:35:43 UTC] PHP Notice:  Undefined index: method_modify in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php on line 2480
[16-Jul-2019 00:35:43 UTC] PHP Notice:  Undefined index: method_read in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php on line 2481
[16-Jul-2019 00:35:43 UTC] PHP Fatal error:  Uncaught Error: Function name must be a string in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php:2483

[16-Jul-2019 02:06:57 UTC] PHP Notice:  Trying to get property of non-object in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php on line 368
[16-Jul-2019 02:07:01 UTC] PHP Notice:  Undefined index: method_modify in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php on line 1426
[16-Jul-2019 02:07:02 UTC] PHP Notice:  Undefined index: method_modify in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php on line 2480
[16-Jul-2019 02:07:02 UTC] PHP Notice:  Undefined index: method_read in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php on line 2481
[16-Jul-2019 02:07:02 UTC] PHP Fatal error:  Uncaught Error: Function name must be a string in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php:2483

[16-Jul-2019 16:10:24 UTC] PHP Notice:  Undefined index: method_modify in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.PHP on line 1426
[16-Jul-2019 16:10:24 UTC] PHP Notice:  Undefined index: method_modify in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php on line 2480
[16-Jul-2019 16:10:24 UTC] PHP Notice:  Undefined index: method_read in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php on line 2481
[16-Jul-2019 16:10:24 UTC] PHP Fatal error:  Uncaught Error: Function name must be a string in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php:2483
[16-Jul-2019 16:10:31 UTC] PHP Notice:  Undefined offset: 0 in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/salesforce_push.php on line 1330
[16-Jul-2019 16:10:33 UTC] PHP Notice:  Trying to get property of non-object in /home/*/*/cms/assets/plugins/woocommerce-one-page-checkout/woocommerce-one-page-checkout.php on line 1451
[16-Jul-2019 16:10:34 UTC] PHP Notice:  Undefined offset: 0 in /home/*/*/cms/assets/plugins/object-sync-for-salesforce/classes/salesforce_push.php on line 1330
[16-Jul-2019 16:10:35 UTC] PHP Notice:  Trying to get property of non-object in /home/*/*/cms/assets/plugins/woocommerce-one-page-checkout/woocommerce-one-page-checkout.php on line 1451

Environment (please complete the following information):

• WordPress Version: 5.2.2
• PHP 7.0.32
• Plugins:

+-----------------------------------------------+--------+-----------+---------+
| name                                          | status | update    | version |
+-----------------------------------------------+--------+-----------+---------+
| advanced-custom-fields-pro                    | active | none      | 5.8.2   |
| woocommerce-jetpack                           | active | available | 3.9.1   |
| broken-link-checker                           | active | none      | 1.11.8  |
| classic-editor                                | active | none      | 1.5     |
| gravityforms                                  | active | none      | 2.4.11  |
| object-sync-for-salesforce                    | active | none      | 1.8.6   |
| varnish-http-purge                            | active | none      | 4.8.1   |
| redirection                                   | active | none      | 4.3.1   |
| welcome-email-editor                          | active | none      | 4.9.1   |
| support-custom                                | active | none      | 1.0     |
| thermometer-for-woocommerce-crowdfunding      | active | none      | 1.0.1   |
| user-switching                                | active | none      | 1.5.1   |
| woocommerce                                   | active | available | 3.4.6   |
| woocommerce-gateway-authorize-net-cim         | active | none      | 2.10.2  |
| woocommerce-checkout-manager                  | active | available | 4.2.3   |
| woocommerce-conditional-shipping-and-payments | active | none      | 1.3.5   |
| woocommerce-google-analytics-integration      | active | available | 1.4.4   |
| woocommerce-name-your-price                   | active | none      | 2.8.3   |
| woocommerce-one-page-checkout                 | active | none      | 1.5.5   |
| woocommerce-product-addons                    | active | none      | 2.9.6   |
| woocommerce-shipping-multiple-addresses       | active | none      | 3.6.4   |
| woocommerce-single-product-checkout           | active | none      | 0.7     |
| formassembly-web-forms                        | active | none      | 2.0.4   |
| wpai-user-add-on                              | active | none      | 1.1.1   |
| wp-all-import-pro                             | active | none      | 4.5.0   |
| wp-crontrol                                   | active | none      | 1.7.1   |
+-----------------------------------------------+--------+-----------+---------+

Additional context

[jonathanstegall]

@charmoney are you pretty confident this issue is new in 1.8.6? Do you think there is work that needs to be done to see if this is an older issue, or is it just a matter of reverting some changes?

[charmoney]

Great question. The 1.8.6 is an assumption based on the `update_wp_meta()` method's line `$meta_id = $modify( $parent_object_id, $key, $value['value'] );` throwing the error. The 1.8.6 changelog line "Maintenance: Replace the various calls to create/update metadata with just one for easier management". Between the changelog and code location, 1.8.6 introducing the issue felt like a reasonable guess. I recommended reverting to 1.8.5 then re-testing and requested additional time to troubleshoot and provide actual reproduction steps.

…

On Thu, Jul 18, 2019 at 8:38 AM Jonathan Stegall ***@***.***> wrote: @charmoney <https://github.com/charmoney> are you pretty confident this issue is new in 1.8.6? Do you think there is work that needs to be done to see if this is an older issue, or is it just a matter of reverting some changes? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#277?email_source=notifications&email_token=AC324LNH3ENNH2JAB2DGRYDQABW6JA5CNFSM4IE2TZJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2IQC2Q#issuecomment-512819562>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC324LLUZDBEK5ZL55IYZMLQABW6JANCNFSM4IE2TZJA> .

[charmoney]

Following up on this issue.

We reverted to 1.8.5 and still see the fatal error, though on a different line now. My initial guess that this was due to the 1.8.6 metadata refactor was incorrect.

[22-Jul-2019 13:16:20 UTC] PHP Notice:  Undefined index: method_modify in /home/.../cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php on line 1503
[22-Jul-2019 13:16:20 UTC] PHP Notice:  Undefined index: method_read in /home/.../cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php on line 1504
[22-Jul-2019 13:16:20 UTC] PHP Fatal error:  Uncaught Error: Function name must be a string in /home/.../cms/assets/plugins/object-sync-for-salesforce/classes/wordpress.php:150```

[jonathanstegall]

Thanks for checking that.

So if I'm understanding correctly, it's like this:

1. There's a fieldmap for users.
2. There's another fieldmap for a custom post type with a lot of meta fields (are they done with ACF?)

I'm assuming the errors occur when data is changed in Salesforce, right? I'm mostly seeing the error logs mention wordpress.php, so I'm assuming it is not happening on push? However I did notice one error from salesforce_push, line 1330, in the is_push_allowed method with an undefined offset of 0. I'm thinking maybe this is still happening because of a pull though, but probably I should check for that existence first.

I'm also noticing that these are the methods where errors happen within that class:

1. update_wp_meta (method modify is missing, followed by method read missing and then function name must be a string)
2. get_wordpress_object_data (non-object)
3. post_update (same as 1)

Presumably this whole part is causing that issue in salesforce_push. But also it should definitely check for the existence of a 0 offset first.

[charmoney]

@jonathanstegall -- I got approval for time to track down the root cause. Turns out this was due to customization code on our side.

During SF Pull events, we filter the object_sync_for_salesforce_pull_params_modify to tweak and adjust values for our mapped custom post type. One of those tweaks is dynamically swapping out a different synced SF Account/WP User than the default mapped post_author. When developed, the post_author was always in the params array. We just changed out the value and returned from the filter. Now, sometimes the post_author isn't in the params array at all, even though it is mapped for SF to WP. (I'm guessing maybe it's left out because the value didn't change for efficiency?)

The code set $params['post_author']['value'] without the method_read or method_modify values.

Fixed on our side by always setting setting the method_read and methor_modify values if we touch the value at all.

I think that means either this Issue can close or it may be worth sanity checking filterable values, because integrating devs could do something unpleasant like pass a params value without the assumed method_update & method_read.

[jonathanstegall]

I think it's a good idea to sanity check the filterable values. I need to give some thought/research into how to do that well.