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

Fix regression on CiviRules search due to exception handling #11829

Merged
merged 1 commit into from
Mar 23, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

CiviRules is currently giving a fatal error on the civirules custom search. This is an Intra-rc regression found by @danielVV #11746 (comment)

Before

Fatal error on civirules custom search

After

custom search succeeds

Technical Details

When running CiviRules there is an incorrect sql query that runs in fillupPrevNextCache - a change in exception handling turned this into a fatal error. I've used the trapException parameter to make it non-fatal again.

Comments

I would prefer to have some sort of notice / deprecation warning when an invalid query is run here. If this were fixed in CiviRules then I think we could add that (seems like a bad idea when we know it will confuse rules users).

The issues with the query are
a) the alias for the contact table is contact not the usual (in our search framework) contact_a
b) it tries to order by rule_label and that fails (not quite sure why, I didn't dig).

ping @jaapjansma @ErikHommel -

Huge thanks for @danielVV for identifying the issue while in the rc stage

Copy link
Contributor

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

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

Confirmed that this PR also fixes a DB fatal error on sorting with a custom field in Advanced Search results. Use case tested -

1/ Create a profile with custom field and add it to the search view.
2/ Search using the profile and sort with any custom field.

Before - DB error with no such field.
After - Works fine.

@adanielvv
Copy link
Contributor

Tested on master.
Search works again with this PR.

The exception is still thrown, but ignored:
[info] Ignoring exception thrown by nullHandler: -19, DB Error: no such field

@eileenmcnaughton
Copy link
Contributor Author

Thanks - the DB error was always there - the regression was that we stopped ignoring it :-)

@adanielvv
Copy link
Contributor

Figured :)

@eileenmcnaughton
Copy link
Contributor Author

Since 2 people have tested & this is a recent regression I'm going to merge based on those tests

@eileenmcnaughton eileenmcnaughton merged commit eda4db7 into civicrm:5.0 Mar 23, 2018
@eileenmcnaughton eileenmcnaughton deleted the rules branch March 23, 2018 01:13
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.

4 participants