Fix File tests that rely on UID1's super user behavior

Fixed test, and removed deprecations.

Appears to have some test failures.

Addressed the mentioned changes in issue summary.

Addressed the superuser code needs to remove & added relevant code for test.

Please review MR 8651, move NR

Thanks for working on this issue!

Okay, so this was interesting. The test fails, because we validate the file in two cases and the default user created in the base test with the uid 1 is not active (status != 1), so it can't be referenced, which triggers an extra violation that we're not expecting. The UserCreationTrait on the other hand always creates the user with status = 1, so it is not affected by this.

Of course it is probably not a good idea to just add status = 1 to the base test. So I think creating a new active user as it is implemented in the MR is the correct fix. Technically, it would have been enough to just use this new user id for the two files where we call validate() later and leave the rest as is, but I think it doesn't hurt here to use the active users to create the other files as well, so I won't block on this minor nit.

The IS looks good. Updating remaining tasks and adding MR to commit as we have multiple. The issue scope seems appropriate. The issue title could be improved, but it seems acceptable as a git message and I guess it makes sense to keep that in line with the other similar issues that have already been committed, so going along with that.

Code review looks good. Tests pass with the changes. No further tests requiring super user in file module left.

The issue is RTBC 🎉.

Forgot to add the tag, sorry for the noise.

Rebased MR with latest code, seems working fine.

I found a lot of other examples in file module where we are using uid => 1 Are we sure we can't remove these special behaviours either?

❯ grep -nri "'uid' => 1"
tests/src/Kernel/Migrate/d6/MigrateUploadTest.php:43:        'uid' => 1,
tests/src/Kernel/Validation/FileValidatorTestBase.php:48:      'uid' => 1,
tests/src/Kernel/FileManagedUnitTestBase.php:40:    $user = User::create(['uid' => 1, 'name' => $this->randomMachineName()]);
tests/src/Kernel/FileManagedUnitTestBase.php:170:      'uid' => 1,
tests/src/Kernel/FileManagedAccessTest.php:50:      'uid' => 1,
tests/src/Kernel/FileManagedAccessTest.php:65:      'uid' => 1,
tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:33:        'uid' => 1,
tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:43:        'uid' => 1,
tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:53:        'uid' => 1,
tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:68:        'uid' => 1,
tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:78:        'uid' => 1,
tests/src/Kernel/FileUriItemTest.php:27:      'uid' => 1,
tests/src/Functional/FileManagedTestBase.php:157:      'uid' => 1,
tests/src/Functional/FileListingTest.php:235:      'uid' => 1,
tests/src/Functional/FileListingTest.php:268:      'uid' => 1,

This issue is specifically related to the remove the superuser dependency code, so there is only one file detect where with superuser code exist:
protected bool $usesSuperUserAccessPolicy = TRUE;

After removing the superuser code, this further leads to test failure for uid attribute in File::create() API, so updated uid variable only in the : file/tests/src/Kernel/SaveTest.php

setting back to RTBC

I found a lot of other examples in file module where we are using uid => 1 Are we sure we can't remove these special behaviours either?

Without using the super user policy, these tests don't use any special behavior for user 1 so the UID does not need to be changed. User 1 is then just a regular user without any special access. That's also why those tests didn't have the flag added in the first place and why I said in my review in #13 that it would have been enough to change the user in just one place basically to complete this issue.

So yes, we could change the UID for the sake of not using UID 1, but it is not required (edit to clarify: and also not in scope, so it shouldn't be done).

Setting back to Needs review, because the MR was rebased.

OK thanks for clarifying @FeyP. Going to mark this RTBC.

@kim.pepper Thanks for taking the extra review off my list, I appreciate it :)

@pooja_sharma Unless I'm missing something here, I think the last rebase was not really required. So for next time: When you rebase an MR, the issue needs to go back to Needs Review. A second review is usually quicker than the first one, because the reviewer is already familiar with the issue and your changes, but it still takes time for you to do the rebase and for the reviewer to (re-)review. So once an issue is RTBC (or even Needs Review, you don't want to rebase while someone is in the middle of reviewing your issue and then they have to start all over again from scratch) you want to rebase only, if it is really necessary. As long as there are no merge conflicts, GitLab will do a rebase automatically, once a core committer merges the MR. A merge conflict usually happens, if there are more recent changes in the main branch that modify one or more of the same files that you are modifying in the MR. GitLab will usually also show you, if there is a merge conflict and a rebase is required. You can see this by looking at the overview page of the MR. So if you are unsure whether a rebase is really needed, it is usually enough to open the overview page of the MR in GitLab and it will tell you: Instead of "Ready to merge by members who can write to the target branch." there will be a message "Merge blocked: 1 check failed" and "Merge conflicts must be resolved.". Note that you must have access to the issue fork for this to work, so if you don't have access yet, you need to request it via the issue.

@FeyP Thanks for reviewing, there is already displaying conflicts errors on MR with error message: need to resolve conflicts locally, due to which I have resolved the conflicts & updated MR as well , not aware about someone is in-between of reviewing the MR.

I believe need to Update IS as well that includes the file names which are updating in it, so updated IS as well for clarity

I keep it in RTBC as it only included rebased code, no any additional changes added. however thanks for highlighting your concerns , 'll keep address those things as well while looking in to issues.

@pooja_sharma Alright. Then I missed something in the upstream changes that caused the conflict. Sorry for that. But why did you now update the remaining tasks again? You completed the update for this file, so this is not a remaining task, it is done now. Can you please change back the IS?

Ah, about to add files name under the steps , by mistaken included in remaining tasks.

thanks for flagging it out & updated IS back.

I read the issue summary, comments and the MR. The issue summary is up to to date, thank you, that is real help to reviews and committers. Also, thanks to @FeyP, for the detailed comment in #13 and for mentoring on this issue.

All questions are answered here, #13 was a great help with that. Leaving at RTBC.

Committed and pushed 35dbb2d498 to 11.x and dd994edd20 to 11.0.x and ea6a45edd6 to 10.4.x and 9eb5ee79eb to 10.3.x. Thanks!