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

CRM-19545: Revert CRM-18776 to get back missing custom fields #417

Merged
merged 1 commit into from Jan 28, 2017

Conversation

@aydun
Copy link
Contributor

aydun commented Jan 14, 2017

As noted on CRM-19545, this PR fixes the symptoms described here

@eileenmcnaughton
Copy link
Contributor

@aydun - should I read that as 'I have reviewed this, please merge it' or 'I am making encouraging noises but it still needs review'

@aydun
Copy link
Contributor

aydun commented Jan 25, 2017

Hi @eileenmcnaughton - somewhere in between! It means I have reviewed this change and tested that it fixes the issues I was seeing. But I have not reviewed CRM-18776 to understand what it was attempting to fix and so presumably there are things that were fixed by it that will be broken again.

If we just want to revert to the situation prior to CRM-18776 then this PR does that. If we want to preserve the good changes of CRM-18776 then this does not do that.

@eileenmcnaughton
Copy link
Contributor

Ok - so we really need @adixon to comment as I think he did the original

@JKingsnorth
Copy link
Contributor Author

Yeh I wasn't able to recreate the issues with the reversion applied. But a second opinion would be needed!

@adixon
Copy link
Contributor

adixon commented Jan 26, 2017

This is pretty tricky code, and I'm not convinced about any patch that doesn't really understand what's going on here - i.e. a few tests is not enough, there are too many different combinations of things that are affected (specifically, custom fields on different entities, different subtypes, and different view types). My patch only fixed the specific case I was running into and was definitely inadequate because it broke some other cases, but there was a followup patch that I believe improved it and might be the right answer. It really needs someone who understands custom field handling in all it's gory details as well as how views works. The code that I patched was very legacy, and I'm not at all sure that changes in how subtypes and/or custom fields since then might suggest a very different way of doing things.

@JKingsnorth
Copy link
Contributor Author

JKingsnorth commented Jan 26, 2017

I'm proposing to revert #386 / CRM-18776 because it breaks views with custom fields extending other entities.

The follow-up patch @adixon refers to is #405 , which also will not fix the problem (for the reasons mentioned on that PR).

After reverting #386 I wasn't able to recreate the issues originally raised in that issue. @adixon can you confirm whether there are issues in your installation after applying this reversion? My testing on this was included here: https://issues.civicrm.org/jira/browse/CRM-19545?focusedCommentId=97524&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-97524

@adixon
Copy link
Contributor

adixon commented Jan 27, 2017

I just left a comment on the issue: https://issues.civicrm.org/jira/browse/CRM-19545?focusedCommentId=99253&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-99253

Short answer, yes, I think this reversion is the correct answer. I'm still concerned about what we don't understand (i.e. what other cases there might be that break this), particularly because the outcome of the breakage was so severe (i.e. unable to access the view admin page!). The views code is woefully under-cared for - all the code I looked at assures us that extends_entity_column_value is only used for subtypes of contacts.


@param $sub_type

  • (optional) Integer Id of the Sub-Type, ie Contact Sub Type

@eileenmcnaughton
Copy link
Contributor

Thanks for weighing in @adixon - I'm merging it now - it will be in 4.7.17

@eileenmcnaughton eileenmcnaughton merged commit d06488f into civicrm:7.x-master Jan 28, 2017
@JKingsnorth
Copy link
Contributor Author

Thanks all. I'd be keen and happy to review any further work that might get done in this area. Give me a shout if this is the case.

adixon referenced this pull request Feb 2, 2017
* Updated call to civicrm_views_custom_data_cache as per CRM-18776

* More readable version, thanks GinkoFJG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants