Add a machine name to free up name to be changed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrded’s picture

Status: Active » Needs review
FileSize
30.15 KB

Please, check out my patch.
Thank you.

AlfTheCat’s picture

Tried the patch but it failed.

< user_relationships-1980610-1.patch

patching file user_relationship_privatemsg.test
Hunk #1 FAILED at 36.
Hunk #2 FAILED at 46.
Hunk #3 FAILED at 220.
Hunk #4 FAILED at 230.
4 out of 4 hunks FAILED -- saving rejects to file user_relationship_privatemsg.test.rej
patching file user_relationships.admin.inc
patching file user_relationships.install
patching file user_relationships.module
patching file user_relationships.test
patching file user_relationships_ui.module
Hunk #1 FAILED at 228.
1 out of 1 hunk FAILED -- saving rejects to file user_relationships_ui.module.rej
patching file user_relationships_ui.test
Hunk #1 FAILED at 29.
Hunk #2 FAILED at 38.
Hunk #3 FAILED at 48.
Hunk #4 FAILED at 58.
Hunk #5 FAILED at 83.
Hunk #6 FAILED at 99.
Hunk #7 FAILED at 145.
Hunk #8 FAILED at 166.
8 out of 8 hunks FAILED -- saving rejects to file user_relationships_ui.test.rej
mrded’s picture

Try to use GIT for applying.
git apply --index user_relationships-1980610-1.patch

Berdir’s picture

#1: user_relationships-1980610-1.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Needs work

Haven't reviewed in detail, but not sure that the update works like this. Don't we need to make sure that the machine names are valid, otherwise you'd be required to change them when you try to edit the form?

I think it's more common to use the id as the machine name in this case, see what was done for e.g. filters.

Will also require to update existing permission assignments then.

And coming from #1335950: Permissions not updated when relationship renamed, this should implement option #2 from there, which means a description on the machine name that explains that changing it results in a loss of permission configuration. Or update them accordingly, but see discussion over there about that.

Andre-B’s picture

I guess changing the machine name of an element is and always will be an issue the site-maintainer has to deal with himself then, take views for instance, they do not use any ids for machine names but a generated one from the actual view name (which can be modified). after initial save you can still change the machine name which would break other configurations and references that rely on it but the main problem (changing the relationship name/ value) is solved from this point without breaking other stuff.

Berdir’s picture

Sure, that's why the agreed upon option #2 in the other issue just said that we should add a description to whatever field is used for the permissions, so that administrators are aware of the implications of changing it.

And, the upgrade path of adding a machine name is different, there we should make sure to update existing permissions if necessary, and it will be if we make sure that machine names are valid/use the id's.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
30.36 KB
1.39 KB

This patch users the id for the machine name, and also updates existing permissions.

heylookalive’s picture

Issue summary: View changes

The patch will result in permission names changing which could cause trouble for anyone using features to export permissions.

Looking at module usage the majority of users are on 7.x Alpha 5, it being an Alpha we can make a change like this but the concern would be messing things up for the 99% of people using this module. That said this patch will enable user relationships to be committed to features, without this we're stuck in the past.

With that in mind I'll ping the maintainers for their opinions but from my point of view we'd commit this patch, get features patch in and tag Alpha 6 with the release notes explicitly stating as per Alpha status we've made a change which will mean work for them. If users are willing to put in the effort great, if not they can stick with Alpha 5.

https://twitter.com/heylookalive/status/625972685386960896

heylookalive’s picture

Patch still applies cleanly to latest dev btw.

heylookalive’s picture

Fair warn maintainers if I don't get a response within some days and verify this patch does what it should I will commit it.

Berdir’s picture

There's an upgrade path, so this shouldn't mess things up too much. Fine to go ahead I think but would be good to make sure that the upgrade path works well with permission changes and so on.

heylookalive’s picture

Status: Needs review » Fixed

Thanks @berdir, appreciate I'm draining your time with all this.

Good work @jhedstrom and @mrded!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.