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 e-notice about inability to fill up prevnext cache by setting tag and group table names #15963

Closed

Conversation

seamuslee001
Copy link
Contributor

… and group table names

Overview

This fixes an E-notice when you use the Basic Search Custom search and you filter on a group or a tag

Before

E-notice shows

After

e-notice disappears

Technical Details

It seemed like when it built the from clause and the where clause different group_contact and entity_tag table names were being generated. So this fixes it by making it a property of the class

ping @eileenmcnaughton

@civibot civibot bot added the 5.20 label Nov 26, 2019
@civibot
Copy link

civibot bot commented Nov 26, 2019

(Standard links)

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 I can't replicate the notice on 5.20 or master - this was the search I tried

Screen Shot 2019-11-28 at 11 37 38 AM

@seamuslee001
Copy link
Contributor Author

@eileenmcnaugton I was generating it when filtering using group and tags at the same time

@eileenmcnaughton
Copy link
Contributor

Hmm I didn't get it - although my search got no results - can you add a screenshot to the template?

@seamuslee001
Copy link
Contributor Author

ah @eileenmcnaughton i think i see the issue now you were doing a search from Contacts -> find contacts

This happens at hte search Contacts -> Custom Searches -> Basic Search

here is a screenshot
notice errors - basic custom search

@seamuslee001
Copy link
Contributor Author

and the search criteria

search_criteria

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Nov 28, 2019
…stall.

From civicrm#15963 this search appears to do what the main basic search does but
we can see that to make it work with sort etc we have to make changes to the BAO_Query object - doing
that to support a custom search scares me & we have a big picture position that we should move these
from core to an extension. However, an easy first step is not to add on new installs. In the case of
this search it seems like an easy call as it doesn't add anything discernable
@eileenmcnaughton
Copy link
Contributor

@seamuslee001 so looking at this has made me question what we are doing - and in particular to think that we should not treat deprecation notices as regressions unless they are popping up all the time. Where they occur in obscure places we have always used deprecation notices to flush these out & I don't think they have to be fixed as a regression.

In this case I think the right answer is to deprecate & discourage use of this report - there is already a good alternative - basic-search. Fixing it requires code changes that affect basic search & advanced search etc & all for something that ideally we would not be maintaining.

I propose we

  1. stop adding it on new installs Remove CRM_Contact_Form_Search_Custom_Basic from searches added on install. #15979
  2. leave the deprecation notice there as it accurately says the search doesn't work well.
  3. consider adding yet another deprecation notice at the whole search level
  4. maybe get more active about moving some of these searches to extensions - probably one search per extension rather than all in one - then we can decide which to keep supporting

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton i would be fine with that for this report, i still feel like e-notices are better fixed sooner rather than later so to speak. In regards to this e-notice there is only 1 other search and its the Event Aggregate one that triggers it but i dunno. i'm ok with 1 and 2 at the moment we can ponder 3 & 4

@seamuslee001
Copy link
Contributor Author

closing in favour of #15979

@seamuslee001 seamuslee001 deleted the fix_enotice_basic_customsearch branch November 28, 2019 20:24
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.

2 participants