- Issue created by @vensires
- First commit to issue fork.
- Status changed to Needs review
5 months ago 5:00pm 10 April 2024 - Status changed to Needs work
5 months ago 5:32pm 10 April 2024 - First commit to issue fork.
Addressed the superuser code needs to remove & added respective code.
Please review MR 8651, move NR
- Status changed to Needs review
2 months ago 5:40pm 3 July 2024 Addressed the superuser code needs to remove & added relevant code for test.
Please review MR 8651, move NR
- Status changed to RTBC
2 months ago 1:59pm 8 July 2024 - π©πͺGermany FeyP
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 π.
- πΊπΈUnited States smustgrave
smustgrave β changed the visibility of the branch 3439836-fix-file-tests to hidden.
- Status changed to Needs work
about 2 months ago 6:52am 10 July 2024 - π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
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
- Status changed to Needs review
about 2 months ago 7:28am 10 July 2024 - π©πͺGermany FeyP
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 so the UID does not need to be changed. It 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.
Setting back to Needs review, because the MR was rebased.
- Status changed to RTBC
about 2 months ago 8:17am 10 July 2024 - π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
OK thanks for clarifying @FeyP. Going to mark this RTBC.
- π©πͺGermany FeyP
@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
- π©πͺGermany FeyP
@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?
- π³πΏNew Zealand quietone New Zealand
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.
- Status changed to Fixed
about 1 month ago 10:09am 29 July 2024 - π«π·France nod_ Lille
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!
Automatically closed - issue fixed for 2 weeks with no activity.