[Follow-up] Selecting no roles and falling back to neutral leads to access denied on nodes

Alright, so the "hotfix" in 🐛 Selecting no roles and falling back to neutral leads to access denied on nodes Fixed , isn't just a hotfix, but the "real" fix. The field values in the node look something like this:

0 => array (3)
   ⇄role_id => null
   ⇄access => string (7) "allowed"
   ⇄_attributes => array (0)

Which is correct. BUT apparently it is legit, to set NULL as an array index in PHP!! So this code here:

$role = $entityAccessFieldDefinitionValue['role_id']; // => $role is NULL
$roles[$role] = $role;

is basically: $roles[NULL] = NULL
which leads to this:

which is NOT empty inside the empty check. So the problem isn't even [""]=> NULL it is []=> NULL (Maybe [] will be resolved to [""] internally).

I didn't know this was even possible. I'll add appropriate tests.

Alright, I added appropriate tests. The problem was, that we never set the roles (or any other config) through the UI. Instead, we used a method to directly set the role ids and access. This way, we'd never set NULL as a role id, leading to [""]=> NULL.

I added one test to check the behaviour via ui and the same test without using the ui (See failing test above, where the hotfix got removed).

Please review!

I don't get it. Using the commit "Revert changes from #3364405, so the tests fail" locally will lead to the tests failing...

For some reason, not on remote.

Thanks for the investigations @Grevil! :)

The reason, why the tests don't fail online is, that the MR doesn't contain the removal of the code change from the fix. But that's good, because when merging this, it would also mean a revert of that fix.

The best way to do such test-only fixes is to use a patch file and name it --TEST-ONLY so everyone sees, it shouldn't be committed.

In this case, the patch file would contain your changes plus the reverted fix. As the tests are green here, but fail in your screenshot, I'm fine with the fix! :) Great work!

One last point, before marking this RTBC and merging:
The screenshot only shows one failing test, but you were talking about two tests added. Did you only run one test in your screenshot, but both failed without the fix?

The reason, why the tests don't fail online is, that the MR doesn't contain the removal of the code change from the fix

No, this isn't true. I added the reverted the change, pushed and rereverted the change and pushed again. This should lead to Test fail, Test succed.

The screenshot only shows one failing test, but you were talking about two tests

Only one test should fail, as only one test is done through the UI. The other Test is setting the config values programmatically, leading to the correct outcome, as the issue ONLY appears through the UI.

No, this isn't true. I reverted the change, pushed and rereverted the change and pushed again. This should lead to Test fail, Test succed.

Thanks, got it. So I shouldn't have looked into the latest MR, but the history of changes and test results before, that should have failed.
Then I agree, I also don't understand the reasons.

Only one test should fail, as only one test is done through the UI. The other Test is setting the config values programmatically, leading to the correct outcome, as the issue ONLY appears through the UI.

We should think about this again. I think we need to see what the UI does as what's correct. The (non-UI) programmatic implementation will never be used, I guess?

But well, why not ensuring both work and just add another programmatic test representing the structure like from the UI? (With a description that both should work). Could you add another test with that array structure?