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.Problem/Motivation
There's no usage in core and contrib
Also function is broken because trying to save immutable config, ref https://www.drupal.org/node/2407153
Proposed resolution
Remove function
Remaining tasks
- decide to remove or deprecate function
- patch/commit
User interface changes
no
API changes
Removal of update_replace_permissions()
Data model changes
no
Release notes snippet
no
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 3143605-2.patch | 1.29 KB | longwave |
Comments
Comment #2
andypostInitial stub
Comment #3
andypostComment #4
longwaveNice find!
So this was introduced in #1872876: Turn role permission assignments into configuration. as part of the original D7 to D8 upgrade path, before we dropped that and switched to only allowing migrations.
As the OP states there are no uses in core or contrib: http://grep.xnddx.ru/search?text=update_replace_permissions&filename=
However, it seems like there is a very small chance that this might have been discovered and used in custom code - even if that was only ever in an update hook, it doesn't seem like this would ever be called at runtime. Still, the BC policy doesn't exclude anything like this, and therefore I think this is public API, and we have to go through the deprecation process.
Comment #5
andypostIt was added in #1872876: Turn role permission assignments into configuration. but not used in 8.x cycle at all
Comment #6
andypostDeprecation patch
Comment #7
andypostThis function is broken because trying to save immutable config
Here's a test to show it (interdiff shows catching exception)
Comment #8
xjmThanks @andypost. Normally, I would say that even dead code should be deprecated, but if this function's only... function... is to throw an exception, then we can probably actually delete it with a CR.
Comment #9
xjmComment #10
andypostUpdated IS and CR
Comment #11
daffie CreditAttribution: daffie commentedTested that the expected exception in the added test is thrown. That proves that the function
update_replace_permissions()is broken.The function is deprecated and will be removed in 10.0.
The deprecation message has testing for it.
All changes look good to me.
For me it os RTBC.
@andypost: Good find!
Comment #12
longwave@daffie We don't even need to deprecate if the function is broken, we can just remove it. Nobody can be using it as it doesn't work - and if they had tried and failed you would hope they would post a bug report, but that hasn't been done either. There are just three issues that mention
update_replace_permissions: this one, the refactoring one where this was discovered, and the original introduction of the function.Reuploading #2 and marking that as RTBC for the simple removal.
Comment #13
xjmMaking the title a bit more clear for anyone who does a double-take at the commit log.
Comment #15
xjmConfirmed with @catch also that he's +1 on removing this since there is no possible way someone could be using it meaningfully. Committed to 9.1.x and published the CR. Thanks! And extra thanks to @andypost for the test proving the broken-ness which made it a lot easier to sign off on something outside normal policy.