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

PHP Warning: array_key_exists() #162

Closed remcotolsma closed 10 months ago

remcotolsma commented 10 months ago
[31-Oct-2023 09:13:52 UTC] PHP Warning:  array_key_exists(): The first argument should be either a string or an integer in /home/customer/www/example.com/public_html/wp-content/plugins/pronamic-ideal/packages/wp-pay/core/src/Payments/PaymentsDataStoreCPT.php on line 76

https://github.com/pronamic/wp-pay-core/blob/f260bc3b6cf38737260e075d7098ce679e3a3f20/src/Payments/PaymentsDataStoreCPT.php#L69-L78

CC @rvdsteege

rvdsteege commented 10 months ago

As $id is already casted to int, we can also accept string as $post_id in PaymentsDataStoreCPT::get_payment() and by re-ordering some code we can prevent calling array_key_exists() for null and false values, like so:

    /**
     * Get payment by ID.
     *
-    * @param int $id Payment ID.
+    * @param int|string $id Payment ID.
     * @return Payment|null
     */
    public function get_payment( $id ) {
+       $id = (int) $id;
+
+       if ( empty( $id ) ) {
+           return null;
+       }
+
        if ( \array_key_exists( $id, $this->payments ) ) {
            return $this->payments[ $id ];
        }

-       if ( empty( $id ) ) {
-           return null;
-       }
-
-       $id = (int) $id;

        $post_type = \get_post_type( $id );

        if ( 'pronamic_payment' !== $post_type ) {
            return null;
        }

The method PaymentsDataStoreCPT::get_payment() only seems to be called through get_pronamic_payment(). The $post_id is passed to PaymentsDataStoreCPT::get_payment() straight away, but the documented parameters are incompatible:

https://github.com/pronamic/wp-pay-core/blob/7580aaee727d3f412faef97fbc0b97ecef44ef3b/includes/functions.php#L23-L31

https://github.com/pronamic/wp-pay-core/blob/f260bc3b6cf38737260e075d7098ce679e3a3f20/src/Payments/PaymentsDataStoreCPT.php#L69-L78

I don't think it makes sense to get a payment from the data store for bool and null values in get_pronamic_payment() and suggest returning null for non int and string values instead of trying to fetch them from the data store in these cases (e.g. bool and null, but that would also prevent calling PaymentsDataStoreCPT::get_payment() for array, float values etc.). I think we can expect any type, as get_pronamic_payment() is kind of a template function and can be used by users like so.

function get_pronamic_payment( $post_id ) {
    if ( ! is_int( $post_id ) && ! is_string( $post_id ) ) {
        return null;
    }

    return pronamic_pay_plugin()->payments_data_store->get_payment( $post_id );
}
remcotolsma commented 10 months ago

Improved in https://github.com/pronamic/wp-pay-core/commit/329b4565a7b2c2cf2f8f6a35daee69761e569d80 and https://github.com/pronamic/wp-pronamic-pay-gravityforms/commit/49ced0204791fbcf05425eafa00b6cea6090c0ae.