Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PHP8 compatibility issues #27

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Fix PHP8 compatibility issues #27

wants to merge 13 commits into from

Conversation

Pebblo
Copy link
Member

@Pebblo Pebblo commented Mar 25, 2024

This PR just prevents fatal's from PHP8

Swaps out instances of each() for foreach()
Adjusts the parameter order passed to implode()
Updates FPDF to the latest version so its works with PHP8.
Fixes a couple of notices.
Patches the Stripe SDK with a couple of fixes.
Uses the 'new' style constructor within PayPal Pro

@Pebblo Pebblo requested a review from tn3rb March 25, 2024 14:50
@garthkoyle
Copy link

Thank you.

Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a var_dump() of potentially sensitive data that needs to be removed, and had a few other minor suggestions, but overall good stuff

@@ -72,6 +73,7 @@ function Espresso_PayPal($DataArray)
$this -> NVPCredentials .= $this -> APISubject != '' ? 'SUBJECT=' . $this -> APISubject : '';
$this -> NVPCredentials .= $this -> APIMode == 'Signature' ? '&SIGNATURE=' . $this -> APISignature : '';

var_dump($this->NVPCredentials);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOPS!

going to go out on a limb here and say it's maybe not a good thing to dump credentials like this 🤔

Comment on lines 41 to 45
if(method_exists($intent, 'toArray')) {
$intent_array = $intent->toArray(true);
} else {
$intent_array = $intent->__toArray(true);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not critical, but it's preferable to use ternaries:

$intent_array = method_exists($intent, 'toArray')
    ? $intent->toArray(true)
    : $intent->__toArray(true);

Comment on lines 20 to 21
//var_dump($_POST);
//var_dump($_REQUEST);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe remove?

$multi_reg = FALSE;
$event_id = isset($_POST['event_id']) ? absint( $_POST['event_id'] ) : isset($_REQUEST['event_id']) ? absint( $_REQUEST['event_id'] ) : '';
$registration_id = isset( $_POST['registration_id'] ) ? sanitize_text_field( $_POST['registration_id'] ) : isset( $_REQUEST['registration_id'] ) ? sanitize_text_field( $_REQUEST['registration_id'] ) : FALSE;
$event_id = isset($_REQUEST['event_id']) ? absint( $_REQUEST['event_id'] ) : '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOLZ! MUCH better 💯

Comment on lines +43 to +44
'start_date' => !empty($_POST['recurrence_start_date']) ? sanitize_text_field($_POST['recurrence_start_date']) : '0000-00-00',
'event_end_date' => !empty($_POST['recurrence_event_end_date']) ? sanitize_text_field($_POST['recurrence_event_end_date']) : '0000-00-00',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@@ -399,7 +399,7 @@ private function _requestRaw($method, $url, $params, $headers)
$hasFile
);

if (array_key_exists('request-id', $rheaders)) {
if (isset($rheaders['request-id'])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subtle but impactful change 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants