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.

Sign up for GitHub

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

Jump to bottom

dev/core#1001 Fix count notice warnings in php 7.2 [Import] #14989

Merged
merged 2 commits into from
Aug 21, 2019
Merged

dev/core#1001 Fix count notice warnings in php 7.2 [Import] #14989

merged 2 commits into from
Aug 21, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fixes php 7.2 count warnings on import class

Before

Under some circumstances (I saw it & replicated it in the test but not consistently in the UI) php 7.2 count warnings appear

After

Warnings fixed. Code less silly, more tested

Technical Details

Comments

@eileenmcnaughton
Update test to pass null instead of array
c0c7b72 
Per https://lab.civicrm.org/dev/core/issues/1001 we want to test NULL is correctly handled &
does not  give a warning in php7 since we seem not to have eliminated it & also the usage of count is actually
really dumb and / or php4 oriented
@civibot
Copy link

civibot bot commented Aug 8, 2019

(Standard links)

@civibot civibot bot added the master label Aug 8, 2019
@eileenmcnaughton eileenmcnaughton mentioned this pull request Aug 15, 2019
Closed
@eileenmcnaughton eileenmcnaughton changed the title dev/core#1001 Fix count notice warnings in php 7.2 dev/core#1001 Fix count notice warnings in php 7.2 [Import] Aug 18, 2019
@jitendrapurohit
Copy link
Contributor

With php 7.2.20 version, I wasn't able to replicate the count warning on import screen, nor does it display any error to me on unit test execution. But the code changes do make sense and I haven't found any problems with the update/optimization made in this PR. Approving it from my side 👍

@eileenmcnaughton
Copy link
Contributor Author

thanks @jitendrapurohit

@eileenmcnaughton eileenmcnaughton merged commit f60ca01 into civicrm:master Aug 21, 2019
@eileenmcnaughton eileenmcnaughton deleted the import_sc branch August 21, 2019 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jitendrapurohit jitendrapurohit jitendrapurohit approved these changes

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eileenmcnaughton @jitendrapurohit