pronamic / wp-pay-core

Core components for the WordPress payment processing library. This library is used in the WordPress plugin Pronamic Pay: https://www.pronamicpay.com/, but also allows other plugin developers to set up a payment plugin.
https://www.wp-pay.org/
GNU General Public License v3.0
27 stars 3 forks source link

Broken block editor because of uncatched fatal error in field JSON serialization #78

Closed rvdsteege closed 1 year ago

rvdsteege commented 1 year ago

I just came across a broken block editor (Classic block) because a fatal error occurred during JSON serialization of a field.

https://github.com/pronamic/wp-pay-core/blob/7baee62c4eebd8854cd9ae28dca5e6e1a45cb1b0/src/Fields/SelectField.php#L100-L112

The gateway configuration had invalid credentials and the option were retrieved through a IDealIssuerSelectField class.

The issue was triggered from WooCommerce via the JSON encode of $this->data in https://github.com/woocommerce/woocommerce-blocks/blob/2ff8573d5426ebee180e50539f113c211362d1b9/src/Assets/AssetDataRegistry.php#L348-L373

Internal Help Scout ticket: https://secure.helpscout.net/conversation/2036688566/24611

remcotolsma commented 1 year ago

A possible solution is:

$data['options'] = [];

try {
    $data['options'] = $this->get_flat_options(); 
} catch ( \Exception $e ) {
    $data['error'] = $e->getMessage();
}

return $data; 

But the question is also why is there an IDealIssuerSelectField instance in Automattic\WooCommerce\Blocks\Assets\AssetDataRegistry->data?

I know we use the field JSON for the registerPaymentMethod content property:

https://github.com/woocommerce/woocommerce-blocks/blob/060f63c04f0f34f645200b5d4da9212125c49177/docs/third-party-developers/extensibility/checkout-payment-methods/payment-method-integration.md#payment-methods---registerpaymentmethod-options-

The field.options property is directly injected in the WordPress <SelectControl options={ field.options } /> component:

We currently also use <BaseControl />, is there a default way to display errors for these control components? Should we add a <Notice status="error">An unknown error occurred.</Notice> when field.error is set? Is field.error a common property for field errors?

Apart from this I think it is correct to add a try catch in jsonSerialize?