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

When creating relationship types don't munge names #14216

Merged
merged 1 commit into from
May 10, 2019

Conversation

alifrumin
Copy link
Contributor

Overview

as @agh1 points out here this change #12097 made on May 8, 2018 made it so relationship types names get munged. This change reverts back to not munging relationship type names.

Before

Creating a Relationship type for example:

create

Results in the Relationship type names being munged see name_a_b and name_b_a below:

{
    "id": "17",
    "name_a_b": "banana_peeler_is",
    "label_a_b": "banana peeler is",
    "name_b_a": "banana_peeler_for",
    "label_b_a": "banana peeler for",
    "is_active": "1",
    "is_error": 0
}

After

When creating a Relationship type names are not munged see name_a_b and name_b_a in the example below:

{
    "id": "17",
    "name_a_b": "banana peeler is",
    "label_a_b": "banana peeler is",
    "name_b_a": "banana peeler for",
    "label_b_a": "banana peeler for",
    "is_active": "1",
    "is_error": 0
}

@civibot civibot bot added the master label May 8, 2019
@civibot
Copy link

civibot bot commented May 8, 2019

(Standard links)

@alifrumin alifrumin changed the title When creating relationship types thru the api don't munge names When creating relationship types don't munge names May 8, 2019
@demeritcowboy
Copy link
Contributor

demeritcowboy commented May 10, 2019

This was tested alongside #13916, with both patches applied.

@eileenmcnaughton
Copy link
Contributor

I dug a little & still think this was done on a 'this seems like better practice' rather than a real reason so I'm happy to merge this reversal.

However, I would note that if #13916 relies on labels always matching names that could be a problem since a) the data model doesn't require it & b) any types saved 'recently' will be different

@eileenmcnaughton eileenmcnaughton merged commit 683b43c into civicrm:master May 10, 2019
@agh1
Copy link
Contributor

agh1 commented May 10, 2019

@eileenmcnaughton no #13916 doesn't require this--it just highlighted it as a potential oddity. Once @alifrumin and I were done with it, case types still use labels and only labels.

@demeritcowboy
Copy link
Contributor

@eileenmcnaughton It's broken currently anyway in that label always has to match name. You can't change the label of a case role currently in civi without problems.

@demeritcowboy
Copy link
Contributor

I mean it's fine if they're different when created, but then you can't ever change it.

@eileenmcnaughton
Copy link
Contributor

@demeritcowboy @agh1 ok cool - well it does seem like merging this will save future confusion

@eileenmcnaughton
Copy link
Contributor

@demeritcowboy doesn't the wrapping in if (empty($params['id'])) mean it still won't change?

@demeritcowboy
Copy link
Contributor

Give me a minute to write this out in english properly...

@demeritcowboy
Copy link
Contributor

Ok I tried to shorten it to remove the rambling which I tend to do. How does this sound:

Current situation, without either PR:
Name and label can be different to start.
If you then change the label it causes problems, but related to the xml not the fact that the empty check prevents $params['name_x_x'] changing. One example is described in #14001.

After 13916:
That's all still the same, but additionally you have a problem if the name and label are different even to start.

@demeritcowboy
Copy link
Contributor

Or yeah I might be wrong about the AFTER part, in which case oops 13916 doesn't rely on this. Having said that, once this was put up as a PR I did my testing with both applied locally.

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