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

Update signature for searchKitTasks hook for civiimport #25232

Closed
wants to merge 1 commit into from

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Dec 29, 2022

Overview

Update signature for searchKitTasks hook for civiimport

@civibot
Copy link

civibot bot commented Dec 29, 2022

(Standard links)

@civibot civibot bot added the master label Dec 29, 2022
@@ -212,7 +212,7 @@ function civiimport_civicrm_alterTemplateFile($formName, $form, $type, &$templat
*
* @noinspection PhpUnused
*/
function civiimport_civicrm_searchKitTasks(array &$tasks, bool $checkPermissions, ?int $userId) {
function civiimport_civicrm_searchKitTasks(array &$tasks, bool $checkPermissions, ?int $userID, ?array $search = [], ?array $display = []) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not required for the fix. But it makes sense to update the signature at the same time..

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattwire do we need to allow for NULL - you are adding a default so if not passed it will be [] & since these params are new there should be no cases of NULL actively being passed in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. It should either be unset or an array. The only edge case - but it doesn't get as far as the hook - is the other bit I'm fixing in this PR where the name is passed in as a string before being "expanded" into the array.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattwire so we can change to just array $search = [] rather than ?array $search = []?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileenmcnaughton Yep, updated. I've rebased this PR so it's just a signature update now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, turns out is does need the ?array because it can be NULL

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattwire yeah - I just think we should convert it to an array before sending off to the hook - rather than this loosey goosey 'could be this could be that' approach

Copy link
Member

Choose a reason for hiding this comment

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

@eileenmcnaughton they way this hook is called from different contexts, there's no way around the fact that sometimes those variables are set and sometimes they are not. However they will always be passed in, even if NULL, so @mattwire what I'd like to do with this PR is this:

Suggested change
function civiimport_civicrm_searchKitTasks(array &$tasks, bool $checkPermissions, ?int $userID, ?array $search = [], ?array $display = []) {
function civiimport_civicrm_searchKitTasks(array &$tasks, bool $checkPermissions, ?int $userID, ?array $search, ?array $display) {

@eileenmcnaughton
Copy link
Contributor

@colemanw can I get you to take a look at this - it is a regression so we need to fix in the rc (currently 5.58). My issue is just where the values get typed along the way - it feels sloppy for the hook to have to handle it maybe being an array & maybe being null

@mattwire can you change the branch?

@mattwire mattwire force-pushed the isesearchtasks branch 2 times, most recently from c13ab6b to 879cc96 Compare February 17, 2023 20:15
@mattwire mattwire changed the title Fix internal server error when calling GetSearchTasks Update signature for searchKitTasks hook for civiimport Feb 17, 2023
@mattwire
Copy link
Contributor Author

@colemanw @eileenmcnaughton what do you want to do with this one?

@colemanw
Copy link
Member

colemanw commented Dec 8, 2023

Happy anniversary to this PR! 🎈 🎉 🥳
I'm gonna close it because the params are unused so don't need to be added & none of us need to spend more of our brainpower on this.

@colemanw colemanw closed this Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants