Simplify entry existence checks in source filters?

[remcotolsma]

I was working on the Pronamic Pay 9.8 release and noticed the changes in https://github.com/pronamic/wp-pronamic-pay-gravityforms/commit/08cfbe8e7311eab0ae9aebbcb4373a33d1796961. I couldn't quite follow what has been simplified here, and whether counting entries for a subscriptions source URL makes sense in terms of performance. Can you supplement this issue with what we solve and open a PR for review @rvdsteege? For now i have reverted the changes in c42318b5aaa40eed6903287d9a61c36869cb81d5.

[rvdsteege]

RGFormsModel::get_lead() constructs a full entry, while we only need to know if an entry exists (to decide whether to add a link).

For example, it seems there currently is an issue in the Wordline Open Banking gateway resulting in remote requests for form fields, which increases the load time of the payments overview page (+/- 10 seconds for 10 payments using that gateway; I might not be up-to-date yet with all repositories / local environment). This illustrates the issue, as we shouldn't need to bother about the fields on this page at all in the first place and this issue would not have occurred with simply counting the entries. The gateway might need a fix, but we could have prevented the increased load time of the payments overview page too.

Call stack of remote requests for each row in payments overview table

``` Pronamic\W\H\F\Http::request() Pronamic\W\P\G\O\Client->request() Pronamic\W\P\G\O\Client->get_access_token_data() Pronamic\W\P\G\O\Gateway->maybe_update_access_token() Pronamic\W\P\G\O\Gateway->get_ideal_issuers() Pronamic\W\P\G\O\Gateway->Pronamic\W\P\G\O\{closure}() Pronamic\W\P\F\CachedCallbackOptions->get_callback_options() Pronamic\W\P\F\CachedCallbackOptions->get_transient_options() Pronamic\W\P\F\CachedCallbackOptions->getIterator() Pronamic\W\P\F\IDealIssuerSelectField->get_options() Pronamic\W\P\E\G\IssuersField->get_gateway() Pronamic\W\P\E\G\IssuersField->set_choices() Pronamic\W\P\E\G\IssuersField->__construct() GF_Fields::create() GFFormsModel::convert_field_objects() GFFormsModel::get_form_meta() GF_Query->get_entries() GF_Query->get_entry() GFAPI::get_entry() GFFormsModel::get_lead() Pronamic\W\P\E\G\Extension->source_url() ```

[remcotolsma]

These seem to be issues that we would like to solve.

[rvdsteege]

These seem to be issues that we would like to solve.

By reverting https://github.com/pronamic/wp-pronamic-pay-gravityforms/commit/c42318b5aaa40eed6903287d9a61c36869cb81d5 or any other suggestions?

[remcotolsma]

I see the problem actually originated in https://github.com/pronamic/wp-pronamic-pay-gravityforms/commit/fe6438a40da3784b5b38fb662eb305977c5c93c1, we can always link to entry even if it was deleted? I think this happens very often in the Pronamic Pay plugin and for efficiency reasons we don't check whether the source still exists?

[rvdsteege]

we can always link to entry even if it was deleted? I think this happens very often in the Pronamic Pay plugin […]

Yikes, that seems indeed to be the case except for EDD < v3, Give, (Gravity Forms), Ninja Forms and WooCommerce.

Intentionally adding dead links does not contribute to a positive user experience and sounds like bad design. I think it is better then to completely remove the source links from the payments overview instead of a hit-and-miss approach. We can continue to use links with proper source existence checking on the payment details page.

[remcotolsma]

We don't intentionally add dead links, we just link, there is a difference? A 404 message or something similar. is not necessarily a big deal, then it is also clear to users that the source has been removed. Shall we restore the situation as before https://github.com/pronamic/wp-pronamic-pay-gravityforms/commit/fe6438a40da3784b5b38fb662eb305977c5c93c1? This is simple in terms of code, performance and still gives users a quick link to the source. In a new issue we can then investigate whether we should check the existence of the source?

[rvdsteege]

Links to non-existing pronamic_gf_lid currently lead to a completely white page; no message whatsoever. We should then at least add something like:

if ( false === $lead ) {
    \wp_die( __( 'The requested Gravity Forms entry could not be found.', 'pronamic_ideal' ) );
}

https://github.com/pronamic/wp-pronamic-pay-gravityforms/blob/b32731734eb1da83565110cc7f6ad40ff391a605/src/Admin.php#L204-L232

But still, I'm not a fan of intentionally adding dead links (ending up at a 404 message or something similar is a dead link).

[remcotolsma]

Reverted in 4c7ecb34c61607c204f394230d90372dd4f6ef2b, improved in cddab3b4ba8f284c47a0a6ac58aeb4ce027d2b74, new issue in:

• https://github.com/pronamic/wp-pronamic-pay/issues/377

I still don't see this as intentionally adding a dead link, but simply as linking to the source. The fact that this link may no longer work over time should not be a major problem. I know from Moneybird that they also have the option to save a source URL with an external invoice. I assume that Moneybird will not check whether this source URL still works. It's not quite the right comparison, but it has some similarities. The fact that this issue exists indicates that improvements are possible with regard to the source (URL). The only question is how we can best implement this. Let's explore this further in https://github.com/pronamic/wp-pronamic-pay/issues/377.