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 #10033 PHP Fatal error Uncaught TypeError PHP8 #10036

Merged
merged 1 commit into from Jun 22, 2023

Conversation

chris001
Copy link
Contributor

@chris001 chris001 commented Apr 25, 2023

The $array parameter passed to implode($separator, $array) was null.
Previously in PHP 7 and less, this gave only a Notice or Warning error.
In PHP 8+ passing a null array parameter to implode now gives a FATAL error and PHP exits because the call matches a different overloaded function signature, expecting an array in first position, but it gets the separator which is a string.

Description

Fixes bug #10033 when running on PHP 8+

Motivation and Context

PHP 7 and below only emitted a Notice or Warning to the log when you call implode with a NULL for the $array.
PHP 8 and above now will throw an Exception and exit PHP because the call signature matches a different overload and the $separator is passed in where it expects the $array.

How To Test This

See issue.
Basic code example proving this is FATAL on PHP 8+

<?php
$emailsArray = null;
$emailsToLine = implode (',' , $emailsArray);
?>
// When run on PHP 7, this will exit without `Exception`.
// On PHP 8, this will throw a PHP `Fatal uncaught TypeError` Exception

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@serhiisamko091184
Copy link
Contributor

Hello @chris001 ,
thanks a lot for the contribution.

Regards,
Serhii

@serhiisamko091184 serhiisamko091184 added PR 4-8 Score given to PRs once assessed Status: Requires Code Review Needs the core team to code review Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member labels Apr 25, 2023
Copy link
Contributor

@clemente-raposo clemente-raposo left a comment

Choose a reason for hiding this comment

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

LGTM

@clemente-raposo clemente-raposo added Status: Requires Testing Requires Manual Testing Status: Passed Code Review Mark issue has passed code review reviewed and removed Status: Requires Code Review Needs the core team to code review labels May 2, 2023
Copy link
Contributor

@johnM2401 johnM2401 left a comment

Choose a reason for hiding this comment

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

LGTM

Have checked the "Send Email" action, sending to "Record Email".

Checked on PHP7.4/8.0/8.1/8.2, running the Workflow by both Cron and In-CRM-Saving

All emails sent as expected!

@johnM2401 johnM2401 added Status: Passed Testing and removed Status: Requires Testing Requires Manual Testing labels Jun 1, 2023
@jack7anderson7 jack7anderson7 merged commit de449e6 into salesagility:hotfix Jun 22, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR 4-8 Score given to PRs once assessed Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member Status: Passed Code Review Mark issue has passed code review reviewed Status: Passed Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants