Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Add a machine name to free up name to be changed.
Comment | File | Size | Author |
---|---|---|---|
#8 | interdiff.txt | 1.39 KB | jhedstrom |
#8 | user-relationships-machine-name-1980610-08.patch | 30.36 KB | jhedstrom |
#1 | user_relationships-1980610-1.patch | 30.15 KB | mrded |
Comments
Comment #1
mrded CreditAttribution: mrded commentedPlease, check out my patch.
Thank you.
Comment #2
AlfTheCat CreditAttribution: AlfTheCat commentedTried the patch but it failed.
< user_relationships-1980610-1.patch
Comment #3
mrded CreditAttribution: mrded commentedTry to use GIT for applying.
git apply --index user_relationships-1980610-1.patch
Comment #4
Berdir#1: user_relationships-1980610-1.patch queued for re-testing.
Comment #5
BerdirHaven'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.
Comment #6
Andre-BI 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.
Comment #7
BerdirSure, 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.
Comment #8
jhedstromThis patch users the id for the machine name, and also updates existing permissions.
Comment #9
heylookalive CreditAttribution: heylookalive commentedThe 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
Comment #10
heylookalive CreditAttribution: heylookalive commentedPatch still applies cleanly to latest dev btw.
Comment #11
heylookalive CreditAttribution: heylookalive commentedFair warn maintainers if I don't get a response within some days and verify this patch does what it should I will commit it.
Comment #12
BerdirThere'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.
Comment #14
heylookalive CreditAttribution: heylookalive commentedThanks @berdir, appreciate I'm draining your time with all this.
Good work @jhedstrom and @mrded!