Easy-digital-downloads: Force http_build_query to use `&` as the arg_seperator

[chriscct7]

HostEurope and a couple other bad-actor hosts change the arg_seperator.output PHP ini setting from the default & to &. This setting is used in PHP core functions like http_build_query and a couple other functions and the change breaks the function from creating a redirectable url.

When using http_build_query and related functions, the third param (optional) is the arg seperator, and only if left unused it will default to the PHP ini setting. When using it, you should always force the third param to be & for good measure.

2 of 3 EDD core uses of it do not force it:

• https://github.com/easydigitaldownloads/easy-digital-downloads/blob/b37acf76690486409e50f5542ae430e6b9c6f405/includes/gateways/paypal-standard.php#L287
• https://github.com/easydigitaldownloads/easy-digital-downloads/blob/ad537475d57fffb880a4619501c1f6dd59d6ddb1/includes/api/class-edd-api.php#L1786

Importantly the first one contains the following line:

$paypal_redirect = str_replace( '&', '&', $paypal_redirect );

This sorta fixes the issue, but we should fix it properly and then remove this line (as it would no longer be needed for the intended purpose and keeping it would breaks products or more likely fees containing an escaped ampersand for display purposes).

https://github.com/easydigitaldownloads/easy-digital-downloads/blob/b37acf76690486409e50f5542ae430e6b9c6f405/includes/gateways/paypal-standard.php#L290

[chriscct7]

Submitting a pr for this in a second

[chriscct7]

PR: https://github.com/easydigitaldownloads/easy-digital-downloads/pull/5257

[pippinsplugins]

PR looks good but let's get some more tests on it to be sure.

To test, enable PayPal Standard and do some checkouts. If everything works properly, it's working.

[SDavisMedia]

All instances of http_build_query() should be hardcoded to use &.

edd_get_php_arg_separator_output() should stay exactly as it is, returning ini_get( 'arg_separator.output' ).

[pippinsplugins]

PR is invalid, per comments above.

[pippinsplugins]

Not going to close this right now but going to punt to a future release as it's a "not broken so why fix it" scenario.

[SDavisMedia]

@pippinsplugins Isn't a host changing the arg_separator.output value the reason why it _is_ broken and the issue was opened?

[chriscct7]

Yep, and there's a line in the current version that also breaks the use of escaped ampersands in the display

[chriscct7]

Will submit an updated pr that fixes both issues later this week or next.

[SDavisMedia]

I remember when we talked about this issue in Slack. @mindctrl mentioned if it ain't broke, don't fix it. @pippinsplugins, you agreed with that statement with a :100: reaction. But John wasn't talking about the issue itself. He was saying just not to change the edd_get_php_arg_separator_output() function, as @chriscct7 attempted to do here: https://github.com/easydigitaldownloads/easy-digital-downloads/pull/5257/commits/f0d46825e4bee4bf94db05a4e065f998fc2e5b07

That function isn't broken and should remain the same. But we should stop using it as a guaranteed value/parameter in http_build_query() since a host could use & instead of the & we need. If we need & every time, why not set it?

[pippinsplugins]

@SDavisMedia we already account for hosts that use &:

$paypal_redirect .= http_build_query( $paypal_args );
// Fix for some sites that encode the entities
$paypal_redirect = str_replace( '&', '&', $paypal_redirect );

That was implemented specifically to solve this problem. The only change this PR does is change _how_ the problem is solved. Since it's not solving anything new, or something not already solved, it's making a change just for the sake of making a change.

[SDavisMedia]

@pippinsplugins Got it. I do see the validity in Chris' opening comment (the bottom), but of course I don't know the fine details. 👌

[pippinsplugins]

This part?

Yep, and there's a line in the current version that also breaks the use of escaped ampersands in the display

[SDavisMedia]

@pippinsplugins This:

This sorta fixes the issue, but we should fix it properly and then remove this line (as it would no longer be needed for the intended purpose and keeping it would breaks products or more likely fees containing an escaped ampersand for display purposes).

[chriscct7]

Also the line above is only implemented for PayPal's redirect. There are other uses that are not fixed, as specifically pointed out in the ticket above.

[pippinsplugins]

That's a fair point. Re-milestoning for shortly after 2.7. There are no known cases of this failing so it's not a priority at the moment but a good idea to take care of.

Note: one change requested on PR.

[chriscct7]

Yep the PR is in need of change, per the discussion about how we wanted to handle is in Slack. Sean made a summary above: https://github.com/easydigitaldownloads/easy-digital-downloads/issues/5256#issuecomment-263380483

[pippinsplugins]

Going to go ahead and close this as wontfix for now.