Easy-digital-downloads: Allow filtering the search args for the Customers table

[zackkatz]

Explain your enhancement (please be detailed)

I want to modify the $args variable that is passed to the EDD()->customers->get_customers( $args ); in EDD_Customer_Reports_Table::reports_data().

EDD_Customer_Query supports searching user meta, but there's no way to search user meta from the "Search Customers" form currently. Adding a filter there would allow for this behavior.

Justification or use case

I'm adding custom columns to display Customer meta-data and I want to enable searching that information. I can add a custom column to the table, but not search it!

[zackkatz]

Example code, which will return Customers based on custom meta search:

add_filter( 'edd_report_customer_args', function( $args ) {

    if ( isset( $_GET['business_type'] ) ) {

        $args['users_include'] = get_users( array(
            'meta_key'   => 'business_type',
            'meta_value' => esc_attr( $_GET['business_type'] ),
            'fields'     => 'ID',
        ) );

    }

    return $args;
});

[JJJ]

I see around 45 instances where EDD filters its arguments, which is pretty significant.

I'd like to propose instead, EDD adopt what we've done in BuddyPress and bbPress, and introduce its own edd_parse_args() helper that always filters the arguments passed into it.

This way, we have a unified way of parsing array arguments, we automatically introduce a ton of new super useful filter locations, and we avoid the problems of having inconsistent naming as we introduce new ones over time.

---

/**
 * Merge user defined arguments into defaults array.
 *
 * This function is used throughout EDD to allow for either a string or array
 * to be merged into another array. It is identical to wp_parse_args() except
 * it allows for arguments to be passively or aggressively filtered using the
 * optional $filter_key parameter.
 *
 * @since 3.0.0
 *
 * @param string|array $args Value to merge with $defaults
 * @param array $defaults Array that serves as the defaults.
 * @param string $filter_key String to key the filters from
 * @return array Merged user defined values with defaults.
 */
function edd_parse_args( $args, $defaults = array(), $filter_key = '' ) {

    // Setup a temporary array from $args
    if ( is_object( $args ) ) {
        $r = get_object_vars( $args );
    } elseif ( is_array( $args ) ) {
        $r =& $args;
    } else {
        wp_parse_str( $args, $r );
    }

    // Passively filter the args before the parse
    if ( ! empty( $filter_key ) ) {
        $r = apply_filters( "edd_before_{$filter_key}_parse_args", $r, $args, $defaults );
    }

    // Parse
    if ( is_array( $defaults ) && ! empty( $defaults ) ) {
        $r = array_merge( $defaults, $r );
    }

    // Aggressively filter the args after the parse
    if ( ! empty( $filter_key ) ) {
        $r = apply_filters( "edd_after_{$filter_key}_parse_args", $r, $args, $defaults );
    }

    // Return the parsed results
    return $r;
}

---

Unfortunately, this has been turned down as a WordPress core enhancement twice in ten years, so the odds of us needing to do this ourselves are high:

• https://core.trac.wordpress.org/ticket/11212
• https://core.trac.wordpress.org/ticket/26673

[mindctrl]

I'm not a fan of the idea of allowing arguments to be filtered all over the place. I think filters should be added very purposefully, and only when necessary, with proven use cases, and used in a way that protects certain types of data from being manipulated. Every filter is an API that we must maintain, and we have enough to maintain now. Every filter exposes developers to our implementation details, making it harder for us to fix and improve things in the future.

We already have filters in places where I think they should not be, and I'd like us to not add more without purposeful consideration.

For example, I don't think we should allow transaction amounts to be filtered between the time they're shown to the customer on the checkout, and the time they're sent to the payment gateway. If it's possible to show a customer "You're being charged $10" and it's possible they can end up being charged $20, that's not right. Same with the customer ID. Simple mistakes could result in the wrong customer being charged for a purchase.

We should work towards fixing this reality.

To illustrate what I'm saying, someone clicks the Buy Now button on the checkout page. The form data that is submitted can be manipulated there.

Then it can be manipulated at the point right before it gets sent to the payment processor.

Then it can be manipulated at the point where the webhook/IPN comes in.

Then it can be manipulated right before it's saved to the database, for example when creating a payment record.

Then when you fetch it again to display it to the buyer or the site owner, it can be manipulated on the way out before you get it... now or at any time in the future. What you see now might not be what you see 2 seconds from now, or 2 years from now.

Literally every level can be manipulated. This is a pretty insane reality in an ecommerce software.

When a platform allows its data to be manipulated in so many places, the results are unpredictable behavior and unreliable data, which is exactly what we're trying to fix in EDD 3.0.

In the context of financial transactions, I think filters should be used very carefully, with purpose, with proven use cases, and in ways that prevent certain types of crucial data from being manipulated at all.

The original request from @zackkatz is more targeted and maintainable. I haven't looked to see if there are other ways to do it. If there's not, this seems like a nice focused use case for the requested filter.

[JJJ]

All of those filters being mistakes, doesn’t make what’s being proposed here a mistake.

The problem with actions and filters, after 15 years of sprinkling them everywhere, is that there is literally no rhyme or reason to them.

What’s being proposed above does not manipulate prices or amounts, and introduces a partially obfuscated pattern with which we gain both flexibility in the system _and_ less obvious misuse.

600k active BuddyPress and bbPress installs haven’t imploded from including the above approach. Most people don’t even see it as an option, and it reduces code churn and developer effort by _not_ needing to take a vote on every new filter we want to add in.

It actually directly addresses all of your concerns in a single utility.

[mindctrl]

600k active BuddyPress and bbPress installs haven’t imploded from including the above approach.

That's great, but we operate in a different context than those two fine plugins. We deal with people's credit cards and bank accounts as a core feature of our product, and introducing things that can make that less predictable feels like a big step in the wrong direction.

reduces code churn and developer effort by not needing to take a vote on every new filter we want to add in.

That's exactly the opposite of what we should be doing. We should carefully consider every single filter we add, and discuss with the team that has many years of experience dealing with the software and its behaviors, has to maintain it going forward, has to support it, may have insights into why we shouldn't add a filter, and so on. This makes it very easy to just gloss over the due diligence process and add filters as if they have no consequences.

It actually directly addresses all of your concerns in a single utility.

It doesn't address any of my concerns. It does the opposite. It makes it super easy to increase the number of filters exponentially. It does it in a dynamic way that's harder to search for, harder to track down as a support person and a developer, and it makes the software less predictable and reliable.

Predictability and reliability are paramount in e-commerce, and exactly what we're striving for with EDD 3.0. That's basically the entire reason the 3DD project is happening right now.

The proposed utility adds two new filters to each call where it's used and enabled, and we literally have no way to guarantee the data we send into a function is the data that gets used. Because of the context we operate in, I don't think this is something we should do.

Not including our extensions, EDD core itself currently has over 100 uses of wp_parse_args(). We use it in all sorts of places, such as order queries, during product creation, in the db insertion layer, in checkout processing, payment gateways, etc. It's a pervasive low level utility that we use all over the place. We may or may not replace all instances of wp_parse_args with edd_parse_args. The instances we do replace, we may or may not enable the filter. Now we have a fragmented code base where we sometimes use wp_parse_args, sometimes we use edd_parse_args with the filter and sometimes without.

And in cases where it's not enough to filter the arguments before and after the merge, because we need more context or some data that's not available at the time of the merge, we would end up having to add more filters manually anyway. The scope of concern for anyone dealing with troubleshooting issues is now expanded considerably with the usage of these.

Above all else we're striving to create software that is secure, predictable, and reliable for the customers that put their trust in us. Reducing those three things by increasing the number of filters and making it easier for developers to manipulate the data that gets passed around inside the plugin doesn't help with any of those. It's a move in the opposite direction. To my knowledge we have no history of requests for such low-level filters and no proven use cases that outweigh the concerns of security, predictability, reliability, and maintainability.

I circled back and read the Trac tickets you linked, and I agree with the sentiments and reasoning of those who decided against these filters in WP core, which are effectively the same concerns I have here. This is a cool idea, and in other contexts I think it could be a powerful addition, but given the context and the direction we've purposely taken with 3DD I don't think it's a good fit.