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

Avoid count()ing NULL #180

Merged
merged 2 commits into from Oct 11, 2023
Merged

Avoid count()ing NULL #180

merged 2 commits into from Oct 11, 2023

Conversation

jensschuppe
Copy link
Collaborator

Fixes #178 by not trying to count() potentially not initialized $this->_contactIds in task forms and use $this->getContactIDs() instead which has a null-coalesce guard with a default value of [].

@jensschuppe jensschuppe added bug status:needs review Code needs review and testing. labels Oct 4, 2023
@jensschuppe jensschuppe added this to the 2.3 milestone Oct 4, 2023
@Detsieber
Copy link

@jensschuppe #180 is absolutely fine for me, and it's pretty clear that you know your codebase much better than I do!
Therefore, I have closed #179

When will release 2.3 be published?

@jensschuppe
Copy link
Collaborator Author

@bjendres a quick review? And any concerns releasing this as a 2.2.1 point release instead of waiting for a 2.3?

@jensschuppe jensschuppe modified the milestones: 2.3, 2.2 Oct 4, 2023
@Detsieber
Copy link

I have tested #180 and it works well. And, releasing as 2.2.1 would be perfect!

@jensschuppe jensschuppe added status:reviewed and tested Code has received thorough review and testing and is ready to be committed/merged. and removed status:needs review Code needs review and testing. labels Oct 11, 2023
@jensschuppe jensschuppe merged commit 24f8ccd into master Oct 11, 2023
jensschuppe added a commit that referenced this pull request Oct 11, 2023
@jensschuppe jensschuppe added status:fixed The issue has been resolved (usually by committing/merging code). and removed status:reviewed and tested Code has received thorough review and testing and is ready to be committed/merged. labels Oct 11, 2023
@jensschuppe
Copy link
Collaborator Author

Released with version 2.2.1.

CRM_Utils_System::url(
'civicrm/donrec/task',
'sid=' . $result['snapshot']->getId()
. '&ccount=' . count($this - $this->getContactIDs())
Copy link

Choose a reason for hiding this comment

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

@jensschuppe I think this calculation is not intended and will cause a type error in PHP 8.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for spotting that, I released 2.2.3 with that being fixed.

jensschuppe added a commit that referenced this pull request Nov 7, 2023
jensschuppe added a commit that referenced this pull request Nov 7, 2023
jensschuppe added a commit that referenced this pull request Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug status:fixed The issue has been resolved (usually by committing/merging code).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP 8.1 compatibility - do not attempt to count() NULL
3 participants