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

[REF] Cleanup BAO_ActionSchedule::getlist() signature #20239

Merged
merged 1 commit into from
May 18, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

[REF] Cleanup BAO_ActionSchedule::getlist() signature

Before

This is only called from 3 places. Only one passes variables. We have

  • php 5.4 support with the & before the function name
  • an unused parameter that is never passed in
  • either NULL or an array returned. There is handling in 2/3 of the places that call this function for the possibility of it returning NULL which we can remove by always returning an array (view the PR with w=1). The last place is civirules which expects an array and doesn't handle anything else (civirules calls this function but really should be calling the v4 api as it is only doing a db lookup not using the magic in this function)

image

After

  • An array is always returned, handling for something different removed
  • no more php 5.4 support
  • the unused variable is gone

Technical Details

The place where civirules calls this is unaffected as it does not pass parameters and it already expects an array

Comments

This is only called from 3 places. Only one passes variables.

Of the 3 2 check if the results is an array - civirules does not
(civirules doesn't really use this function - it would be better to
do a v4 api get)
@civibot
Copy link

civibot bot commented May 6, 2021

(Standard links)

@civibot civibot bot added the master label May 6, 2021
@@ -35,32 +35,30 @@ public function preProcess() {
$mapping = CRM_Utils_Array::first(CRM_Core_BAO_ActionSchedule::getMappings([
'id' => ($this->_isTemplate ? CRM_Event_ActionMapping::EVENT_TPL_MAPPING_ID : CRM_Event_ActionMapping::EVENT_NAME_MAPPING_ID),
]));
$reminderList = CRM_Core_BAO_ActionSchedule::getList(FALSE, $mapping, $this->_id);
if ($reminderList && is_array($reminderList)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If clause removed - view with w=1

@@ -126,27 +126,25 @@ public function browse($action = NULL) {
// Get list of configured reminders
$reminderList = CRM_Core_BAO_ActionSchedule::getList();

if (is_array($reminderList)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If clause removed - view with w=1

@demeritcowboy
Copy link
Contributor

demeritcowboy commented May 18, 2021

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • Did a light run on the admin scheduled reminders page and the events scheduled reminders tab.
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
      • Agree I don't see where the pass-by-ref is used.
    • [r-maint] ?
    • [r-test] Pending It's been ages in civitime since it ran (two weeks) so running again. Jenkins retest this please.

@eileenmcnaughton
Copy link
Contributor Author

thanks @demeritcowboy

@demeritcowboy demeritcowboy merged commit b9d0770 into civicrm:master May 18, 2021
@eileenmcnaughton eileenmcnaughton deleted the act branch May 18, 2021 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants