- Issue created by @jhuhta
- Issue was unassigned.
- Status changed to Needs review
about 1 year ago 11:46am 26 October 2023 - Open on Drupal.org →Core: 10.1.4 + Environment: PHP 8.1 & MySQL 5.7last update
about 1 year ago Waiting for branch to pass - 🇫🇮Finland jhuhta
This should provide a solution. Sorry for the mostly bogus diff for the schema.yml, the original had dos newlines.
- 🇧🇪Belgium BramDriesen Belgium 🇧🇪
Wouldn't it be better to do the opposite of what you're trying to do with the patch?
- Create a checkbox "Don't display for users with the admin role"
- Remove the is User 1 check
- Wrap the code similar to like what you're doing with the current patchOpen to discussion of course 😇. I'm not a fan of having user 1 checks.
- 🇫🇮Finland jhuhta
Me neither. I just wanted to create as little intrusive change as possible, to spare the need for warning users or creating update hooks. I don't really get the point of hardcoding uid 1 there, so if you think, as a maintainer, that refactoring that would be a better approach, I can probably take another look at it later.
- 🇧🇪Belgium BramDriesen Belgium 🇧🇪
I think it makes much more sense to replace the
exclude_uid_1
withexclude_administrator_role
or even more dynamic that you could select multiple roles. It will indeed take a bit more effort (updating the schema and an update hook to process it) but I think that's the more future proof way to go 😇. - Open on Drupal.org →Core: 10.1.4 + Environment: PHP 8.1 & MySQL 5.7last update
about 1 year ago Waiting for branch to pass - 🇫🇮Finland jhuhta
How about this then? I decided to abandon the permission based approach completely and use role based one. This is maybe not an optimal way to do Drupal features, but I do think this is way better than before anyway (thanks for the suggestion!) and the permission based one just can't work for admins. This is clean and straightforward, and the admin can decide the roles which are affected, if any.
Also, the update path should work. Please review.
- Status changed to RTBC
about 1 year ago 9:02am 6 November 2023 - Open on Drupal.org →Core: 10.1.4 + Environment: PHP 8.1 & MySQL 5.7last update
about 1 year ago Waiting for branch to pass The last submitted patch, 9: 3396895-cookiebot-for-admins-9.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- Status changed to Needs work
about 1 year ago 12:12pm 10 November 2023 - 🇫🇮Finland pontus.talvikarhu
Patch addressing the very small coding standards errors the tests brought up.
- Status changed to Needs review
about 1 year ago 12:18pm 10 November 2023 - Open on Drupal.org →Core: 10.1.4 + Environment: PHP 8.1 & MySQL 5.7last update
about 1 year ago Waiting for branch to pass - Status changed to RTBC
about 1 year ago 8:46pm 10 November 2023 - 🇫🇮Finland jhuhta
I'm RTBC'ing the last change by @pontus.talvikarhu - maintainer consideration is ofc needed too as I wrote the original patch.
- 🇫🇮Finland sokru
Apparently this is not so rare usecase. +1 for RTBC. Included a related issue that should be closed if this gets fixed.
- 🇧🇪Belgium BramDriesen Belgium 🇧🇪
Looks good to me. Will leave it a RTBC for a bit in case another maintainer wants to chime in. Closed the other issue as well as this one has the fix I suggested there as well.