After uninstalling the module the Anonymous and Authenticated user roles where deleted!

I don't see how this module could possibly be deleting roles. This module does absolutely nothing with roles, and it would take explicit code to create or delete roles.

I suppose this could be something done by core as a result of the change made in #2571235: [regression] Roles should depend on objects that are building the granted permissions → . If that's the case, then votingapi_widgets/src/FieldPermissions.php would need to be altered in a manner similar to how node/src/NodePermissions.php was changed in that issue.

Today I de-installed the Rate module, it did the same.
If you don't remove a permission and deactivate the module, every related role will be warned and then deleted.
Seems to be something general.

Yes I believe the problem and solution for this module is what I said in the second paragraph of my post #3 above

IMO. Drupal core should not delete the Anonymous user and Authenticated user roles, even when a module that defines a permission that is assigned to those roles is uninstalled. It is not even possible to delete those roles from IMO. Drupal core should not delete the Anonymous user and Authenticated user roles from /admin/people/roles.

I created this issue that might be a contributing factor or the reason for this issue: https://www.drupal.org/project/drupal/issues/3367770 🐛 Deleting a field that uses a permission_callback does not delete the permission Active

I checked this module in the Drupal 9.5.9 and add config dependencies to each dynamic permission. Here is a patch.

#8 Could not apply patch!

Hi @erdm

Thank you so much for taking the time to check this out.

If you could let us know the version you tried this patch on, that would be useful. Despite I had experienced these problems with Drupal 9.5 as well, I yet provided a patch to address them. Please let us know if it doesn't work in Drupal 9.5.9.

Hi Shifali Baghel ,
Thanks for patch.
I tested with Drupal 10.1

@erdm: please pardon me for asking but are you sure you tested the patch with Drupal 10? Just asking because there is no Drupal 10 release of this module.

Sorry, my mistake...
I'm testing patches one by one in different versions. You could call it a kind of hobby.
I mixed up the issues. I don't know how it happened but I posted it here.
I tested this patch ( issue-3333559: D10 compatibility) 📌 [9.2] Remove jQuery dependency from the once feature RTBC for D10 which didn't work.

Sorry again,
Shifali Baghel & gillesbailleux .Thanks for effort....

@erdm: let's hope a D10 release will soon be available

You need to set the issue status to "Needs review" to get the testbot to test your patch and to let people know there's a potential patch available...

The last submitted patch, 8: votingapi_widgets_field_permission_25-07-2023.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The testbot shows "Build successful" because there are no tests - all it's really doing in this case is proving the patch can be applied and there are not syntax errors. But it does also show some coding standards problems introduced by the patch - those should be fixed.

Regardless, this patch should really have a test case, which would be similar to the test case added for NodePermissions in the issue I linked above. Test cases are especially important for permission issues and for issues that cause site damage like the removing of the Anonymous and Authenticated permissions.

The reason for the test is to 1) demonstrate the problem that was reported, and 2) demonstrate that when the patch is applied, the problem is fixed, and 3) prevent this or a similar problem with permissions from re-occurring.

And as an additional benefit, when we have at least one test the testbot will stop improperly changing the issue status to "Needs work" after a "Build successful" test.

And here's another good reason why we need a test. This patch has a line:
+ $dependecies = [
Note that the variable name is spelled wrong - it should be $dependencies. Because this variable is used later, and is the key to defining the permission properly, this patch CAN'T be working properly.

Setting to "Needs work" to:
1) Add a test case
2) Fix coding standards and spelling problem

After the PHPCS bugs and the spelling error were fixed, the patch is added once again for the time being.

Here is a test case that uninstalls votingapi_widgets and checks to see if the Authenticated and Anonymous roles are still present after the module is uninstalled.

According to the original post, this test should FAIL if these roles have been deleted.

Thus, this test gives us a way to confirm the problem exists AND to confirm that the patch solves the problem, because if the patch solves the problem then the test will PASS.

It looks like the test needs to be made a little more complicated. This test only installs/uninstalls the module, but the OP said a field was added to a node type and configured, THEN the field was deleted and the module uninstalled. Adding a field, configuring the field, then deleting the field needs to be added to this test.

Even with a test, the patch only solves the problem for this module. I think the issue should be solved for the general case (#3378038). That would involve finding the code that deletes the roles and then special casing the two roles as undeletable.

@TolstoyDotCom: I agree with you 100% - this is a core bug and needs to be fixed in core.

However, I have no hope that will be fixed in core in a reasonable time. It's amazing to me that this behavior, which is opposite of how we treat other dependencies in Drupal, was committed after 6 years.

In every other case in Drupal, we always forbid the deletion if something depends on it - for example we don't allow a field to be deleted if a node type depends on that field. But this behavior for Roles is the equivalent of deleting the node type when a field it needs is deleted. That's just wrong. Depending on contributed modules to do some very specific and poorly documented things to prevent this is just bad engineering - contributed module should not be able to accidentally delete Roles, especially when they are not using Roles at all! Deleting any entity should require specific coding, should never be a side-effect of something unrelated, and should fail unless you've done it exactly right. With Roles, they are being deleted by default unless you take actions that most people have no way of knowing are needed.

Regardless, since I don't have any hope for a quick fix in core, I would like to see the work-around here put in. Adding a test has the additional benefit in that it will serve as an example to use for further testing of this (untested) module to ensure that all the other parts of the module continue to work in the future when core changes underneath us, as well as allowing Drupal CI to function properly (this requires at least one test, which we don't currently have).

One challenge in creating tests for this module is that we don't use the "standard" module installation process - this is because the library we need for the widgets is not contained in a composer repository known by Drupal core. That is why we have special instructions on the project page and in our README where we describe how to add that repository and install that external library manually.

We need to have a similar process for our tests, because we still need that external library for our tests.

This patch is an experiment - it adds a drupalci.yml file which is used to customize the installation process on Drupal
CI. I expect a few iterations here before I hit on the magic combinations of commands and syntax that will work. Unfortunately I can't test this locally - it only works on Drupal CI - so bear with me if it takes a couple of failures before
I get it right.

The last submitted patch, 25: 3265224-25-drupalci.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Well I'm shocked that actually worked first time. That proved the build process of including the external library. Now lets add back the testing process in addition to the build. If this is successful then I can commit the drupalci.yml which contains the instructions for the testbot.

OK, I improved the test to follow all the steps in the issue summary.

I have attached two patches here:

1. A patch that only contains the new test. This should FAIL, because it is seeing the same bug as originally reported.
2. A patch that contains the new test AND the patch from #19 . If the patch from #19 fixes the problem, then this patch should PASS.

The last submitted patch, 28: 3265224-28-test-only.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 28: 3265224-28-field-permissions.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Minor changes to the test since the patch seems to also fix 🐛 Deleting a field that uses a permission_callback does not delete the permission Active and the test was assuming that it didn't.
Same expectations with these two patches as #28.

The last submitted patch, 31: 3265224-31-field-permissions.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 33: 3265224-33-field-permissions.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

OK, After looking at this, I think my test-only patch in #33 is doing the right thing and failing because of the bug reported in the original post. The test output shows that after uninstalling votingapi_widgets, the 'anonymous' role doesn't exist anymore.

But the test-only + #19 patch shows that #19 doesn't solve the problem because the test still fails in the same way - after uninstalling votingapi_widgets the 'anonymous' role doesn't exist anymore.

Clean up of 33. This should still fail because it demonstrates a bug in the code, as described in the issue summary.

I'm inclined to just commit this patch because we desperately need at least one test for this module to make the automated testing system work properly.

The last submitted patch, 37: 3265224-37-test-only.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Committed #37.

Leaving this issue open because we still need to address the problem in the issue summary. At least we now have a test. Any patch needs to ensure that this test will PASS when the patch is applied. The module test currently fails because of this bug.

Re-visiting this.

It looks like the change I made between #36 and #37 introduced an unintended test failure that has persisted to this day. (There's an interdiff! Thank you me!)

Drupal CI was turned off on 1 July, and it took me until 7 July to get the testing working for this module on GitLab CI 📌 Turn on GitLabCI testing Fixed .

So now that this module is being tested again, and now that I have (a little) time to devote to this, I've switched this issue to the 2.0.x branch and opened a MR so we can get this issue fixed with the new testing system.

First step is to restore the changes made between #36 and #37 to get the test back to where it demonstrates the bug. Then I will try to fix this issue the way I suggested in #3.