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

(REF) Tidy up unused params in api_v3_ContributionSoftTest #25322

Merged
merged 2 commits into from
Feb 17, 2023
Merged

(REF) Tidy up unused params in api_v3_ContributionSoftTest #25322

merged 2 commits into from
Feb 17, 2023

Conversation

braders
Copy link
Contributor

@braders braders commented Jan 11, 2023

Overview

Bit of test cleanup here, which also helps on the cleanup of dyanmic properties for PHP8.2

Before

  1. _processorParams set but not used (and I also checked the parent class, plus for signs of it being passed around in odd ways). Looks like a copy-paste from some other tests.
  2. _softcontribution and _softcontribution2 were properties, but could have just been straight variables. This is test code, so no backward-compataiblility risk to changing these.
  3. Some variables were being set but never used.

After

The above issues resolved. This class is now PHP8.2 ready.

@civibot
Copy link

civibot bot commented Jan 11, 2023

(Standard links)

@civibot civibot bot added the master label Jan 11, 2023
@seamuslee001
Copy link
Contributor

@braders

Test failure relates

api_v3_ContributionSoftTest::testGetContributionSoft
Trying to access array offset on value of type int

/home/jenkins/bknix-dfl/build/core-25322-5lef8/web/sites/all/modules/civicrm/tests/phpunit/api/v3/ContributionSoftTest.php:108
/home/jenkins/bknix-dfl/build/core-25322-5lef8/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:235
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:1721

@eileenmcnaughton
Copy link
Contributor

Looks like a good improvement (once tests are passing)

Non blocking but it would be nice to rename softcontribution to softContribution - the more we comply with IDE spelling checkers the easier to spot real misspellings & also non-native English speakers can find it hard to parse where one word ends & another starts even when it seems easy to us

@braders braders force-pushed the unused-params-contribution-soft-test branch from 8ebdded to 766a306 Compare January 12, 2023 19:18
@eileenmcnaughton
Copy link
Contributor

test this please

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@eileenmcnaughton
Copy link
Contributor

test this please

2 similar comments
@braders
Copy link
Contributor Author

braders commented Jan 17, 2023

test this please

@seamuslee001
Copy link
Contributor

test this please

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@yashodha
Copy link
Contributor

test this please

1 similar comment
@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton eileenmcnaughton merged commit 9406f66 into civicrm:master Feb 17, 2023
@eileenmcnaughton
Copy link
Contributor

that last one seems to no longer run so I think all is passing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master merge on pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@braders @seamuslee001 @eileenmcnaughton @civicrm-builder @yashodha