There is no way to delete file entities of other users

+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -60,8 +60,8 @@ class FileAccessControlHandler extends EntityAccessControlHandler {
+      // Only the file owner and user 1 can delete and update the file entity.
+      if ($account->id() == $file_uid[0]['target_id'] || $account->id() == 1) {
         return AccessResult::allowed();

It's not a good approach.
I guess it's better to check $account->isAdmin() .

We probably can use the 'administer content' permission for this?

Here are some related issues to read over:

Uploaded files are impossible to replace
Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger
Node revisions: Have an option to delete attached files even when revisions are enabled for a node

The problem with allowing any user to delete a file that has references to it is that all references to that file would need to be removed or else you leave hanging dependencies, which if not handled properly, can cause fatal errors.

Until they figure out how to traverse every entity and remove every reference when deleting a file, this issue will remain stuck.

Imagine if you have 2 billion references to your file. Now, on file delete you need to execute queries to look up all of the references, iterate through them and delete all of those references.

Now consider revisions and translations.

This is why Drupal 8 hasn't yet figured out how to let admin's delete files. You can't do a potentially limitless number of queries on entity delete.

@oknate I get it. Just curious tho, is the "Used in" count in /admin/content/files accurate? I would assume not.

Whether that's accurate depends if you're using modules that use files without updating the file_usage table or not using the file.usage service.

Right now that field_usage table doesn't list specifically where the file is used on an entity, so if you used the same file on two different fields, I'm not sure if it would keep that count accurate or not.

Re #6: begs the question of why the user who created the file entities CAN delete them, doesn't it? The same problems still exist.

Currently we have a very bad situation about managing files in D8. We have a customer which is very confused about this situation where editors cannot delete files by others. I created a views bulk operation view with a filter on file usage. But the currently the editors need to talk to each others for just deleting files. And if an editor isn't available to delete his/her own file entity we (the maintenance company) needs to manipulate the ownership of an file entity in database. With this bug D8 is currently not usable for professional file management.

About changing status to major
Because deleting files of other users was possible in D7 and this bug "Cause a significant admin- or developer-facing bug with no workaround." I set the status to "major".

About changing title and issue summary
The fact that currently even user/1 is not able to delete file entities is true. But solving this problem only for user/1 and patch #3 is not helpful. We need a solution where user/1 can give admin or editor roles also this option and can maybe bypass this permission like it is in other places. I believe we need to define another permission which allows to block this functionality especially with the knowing problems in mind.

About the proposed solution
I know about ideas to restrict user/1 permissions which are currently mostly organized as hard coded permission bypass. I am open to start at this place to change this and only use a permission to allow file entity deletions especially as long user/1 can add herself/himself to an admin role and add this permission to this role.

Just an additional important point especially related to upcoming #GDPR Compliance in Europe. When there are data in uploaded files we need to delete because we are forced by law there is currently no way without manipulating database and/or file system directly.

When it's so difficult to get an additional permission we can maybe follow the suggestion of comment #5: "'administer content".

I think I will use a patch like this to temporary solve the problems of our customers.

Fixed patch of comment #5 (removed "docroot" from file path).

@vaplas For a first step I just fixed the #5 patch that it can be applied normally. I didn't made tests or a deeper review yet. That's on my to-do list.

Just fixed comment on patch and still not tested functionally by myself.

It needs new permission otoh if user owns file he could edit it

+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -64,8 +64,9 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+      // Only the file owner and users with "administer content" permission
+      // can delete and update the file entity.
+      if ($account->id() == $file_uid[0]['target_id'] || $account->hasPermission('administer content')) {

There's no such permission in d8, also "administer nodes" defined by node module which is not dependency for the file one

@andypost When "administer content" is still a permission of the node module it's really wrong to use this beside a custom workaround. Maybe this permission should be renamed because "content" is in D8 more than just nodes including files (see: "admin/content/files").
I hope we can get this fixed including a new permission in core system.

Before someone want to start adding a new permission (s)he should communicate about this in this issue to avoid double work. And I think if we want to get this committed we also need to extend the testing part when we introduce a new file permission.

@C_Logemann basically if your module depends on presence of permission from other module you have to add dependency on the first. Another option is use module_exists before checking for permission

I'm sure that file module needs own permissions to manipulate file entities that could be tricky like comment module doing... lots of thin places which require extra testing for permission permutation

In a light of GDPR we have to allow owner user (person who created file entity) to delete it anyway!

A serious issue, I think we should:

1. Add a permission to delete file entities.

2a.
Add a queue worker or an immediate operation (depending on the amount) that finds file usage and resolves dependencies - probably removing
field values prior to deleting the file entity.

If the deletion is implemented by a third party module, a message (in case of VBO a confirmation step) will need to be implemented where
dependencies are listed.

2b.
Another option (probably an easier one) would be checking file usage and not granting deletion access of the file entity if it is used. Then it'd be completely up to the user to handle dependencies first.

Any thoughts?

One thought I had is to replace the file itself (not the file entity) with a default file. Since it's difficult to know how to handle dependencies, if you want to delete a file from the server and it's attached to a file entity, the file entity could be left intact and adjusted so that it points to a default file. This would avoid issues where fatal errors are thrown because code in a contrib module or custom module hasn't handled that file entity being removed. Perhaps that could be an option: 1) remove file and replace with default or 2) remove references to file, where possible. As of right now, I'm not sure there's a way to do a reference lookup of all the usages of a file entity reference. The usage table just specifies the parent, not the specific field. And I'm not sure how you'd find entity embeds in text fields. That's why I think, although a bit crazy, leaving the file entity and deleting the file might be a way to allow deleting files and avoiding leaving hanging dependencies.

In a light of GDPR we have to allow owner user (person who created file entity) to delete it anyway!

That's already implemented and I think it's clear that this should not be removed. It also should be explicit included in testing.

Currently I think the possibility to delete files by owners has no UI and can be used via API like Views Bulk Operations is using. With GDPR in mind there should also an interface to delete files by its owners in core. But I think this should be handled by another issue.

Related issues: #2950127: Add helpful reason for 'update' and 'delete' access not being allowed to FileAccessControlHandler #2310307: File needs CRUD permissions to make REST work on entity/file/{id}

#37.2 in #2310307: File needs CRUD permissions to make REST work on entity/file/{id} may be a good solution to re-introduce the D7 permissions:

(user_access('delete any ' . $type . ' files', $account) || (is_object($file) && user_access('delete own ' . $type . ' files', $account) && ($account->uid == $file->uid)))

Thoughts on adding "delete own files" and "delete any files" permissions?

Adding parent item.

adding "delete own files" and "delete any files" permissions?

Good idea!

I added the following issue as child: #2936175: Add delete button to admin/content/file

Maybe we also need a UI for users to view their own orphaned files and get the possibility to delete.

The problem with allowing any user to delete a file that has references to it is that all references to that file would need to be removed or else you leave hanging dependencies, which if not handled properly, can cause fatal errors.

By any user do mean any user or any specific user (i.e. admin)? Right now this show stopper for us, since we often update files and the old files contain incorrect information. It seems the justification here is, "you don't know how you are using your files so we are going to assume you are going to screw up and not let you touch them." Which I think is the wrong approach.

One thought I had is to replace the file itself (not the file entity) with a default file. Since it's difficult to know how to handle dependencies, if you want to delete a file from the server and it's attached to a file entity, the file entity could be left intact and adjusted so that it points to a default file.

This is a reasonable solution. Another would be to return a sensible error if something attempts to access a non-existent file entity.

That said... currently we can't replace files either. Which would be another reasonable thing to add along with "delete". In fact a good "replace" could do a lot to mitigate the need for delete.

The point here is there are real business needs to be able to remove a file from our system, permanently. The justifications for not implementing this, which I accept are real, don't IMO outweigh the needs. So at the very least this should be implemented so though willing to risk the issues with removing files can do so.

I know this is not the right approach but I needed a quick fix so I updated the patch from #3 to work with the current 8.6.1

Sorry, posted wrong patch

A real patch for this should probably incorporate the approach suggested in #23 combined with re-introducing the D7 permissions as suggested in #29. And of course it needs a whole bunch of tests :)

Because I needed a workaround for a customer I tested patch #18 and figured out, that the machine name of the "Administer content" permission is "administer nodes". Because this is just a workaround on one customer system and I agree that we need a new permission for this I don't share this patch o this place and hide the buggy patch #18.

At least until a better solution comes along, such as a new permission, I think it makes a lot more sense to tie things to "administer site configuration". A user like UID 1 will have that permission and it means we aren't tied to the node module. For all the reasons discussed, file deletion shouldn't be a common action anyways. Attached is a patch along these lines.

Reroll of the patch to apply cleanly to 8.6.x.

Reroll of the patch to apply cleanly to 8.5.x.

@nbaosullivan Your patch cannot apply because it's not relative to Drupal project/web root folder. I think you built it inside a project repository e.g. composer project because it's starting with "/web/core/modules ...". This should be "/core/modules ...". This is needed to be auto applied by the d.o testing system and e.g. via composer with "cweagans/composer-patches" connected to "drupal/core".
When patch is used on this common way I think we don't need a speacial 8.5 version because the related code didn't changed.

And maybe define an exception for user/1 to delete file entities like other entities.

This is not a good option, as it essentially increases the special state for user 1. Note that there is ongoing work in core to remove the special state for that user.

Add existing or new permission to delete file entities.

There are two options defined in here. I am changing to use existing in the description for this one. This is also why I've added the third option - add a new permission.

So at the moment there is an implicit permission that anyone can delete his own file.
I am adding a new admin-level permission to manage delete any file and make the check here to respect that.

This patch is against current 8.8.x (1d29b748af).

There is new If in the code - so it needs tests :). Keeping as needs work for that.

Needs review - to trigger the bot.
After that will revert it back as per previous comment.

refactored all == to === for consistency.

Patch #47 worked for me on 8.6.15.

Just in case it's useful in the interim - Delete files

#47 let me delete files as a non-user-1 administrator, woo hoo! I haven't tested permissions settings for other user roles.

core 8.7.3
vbo 8.x-3.2

................
Might should update (or remove) the code comment at line 72, ya?

// Only the file owner can update or delete the file entity.

(And the "forbidden" message -- )

return AccessResult::forbidden('Only the file owner can update or delete the file entity.');

Any chance this could become part of core files admin view, instead of needing VBO...?

(Or, perhaps a more important question: Is this patch headed for adoption, or not likely? I was wondering if it might have become outdated / superseded, or if it's still relevant etc.)

I'm not a core maintainer but I'm sure we can get this very important functionality in core when it's ready to commit. But there is still some work to be done including testing etc.

First time contributor here - please be gentle ;) I've modified the patch to work with the latest current release.

Thanks, @Steven McCoy for contributing! A few issues with patch #55:

1. Needs work since the patch doesn't apply. Trying to do so manually results in:
   fatal: corrupt patch at line 47

2. +++ b/core/modules/file/src/FileAccessControlHandler.php
   @@ -20,7 +20,7 @@
   -    if ($operation == 'download' || $operation == 'view') {
   +    if ($operation === 'download' || $operation === 'view') {
   
   @@ -42,7 +42,7 @@
   -      elseif ($entity->getOwnerId() == $account->id()) {
   +      elseif ($entity->getOwnerId() === $account->id()) {
   

   Nice changes, but IMHO out of scope for this issue.

3. +++ b/core/modules/file/src/FileAccessControlHandler.php
   @@ -63,8 +63,14 @@
          // Only the file owner can update or delete the file entity.
   

   This comment is no longer really true. Something like:

   "Otherwise, only the file owner ..."

Cheers,
-Derek

whoops ;)

Like so. Since #55 doesn't apply, here's a raw diff of the two patch files. Actual interdiff relative to #47 also attached.

Probably still NW for failing tests and adding new tests, but let's at least get some valid testbot results to work from.

Thanks,
-Derek

Hey folks,
Thanks for the patch, the last one works great!
However, I'm changing back to "Needs work" cause we need tests.

By the way, shouldn't we also introduce "delete own files" permission? (as we have for other entities)
If so, does it make sense to do it here?

Adding a new patch with additional permissions for delete/edit operations.

I'm also thinking about hook_update_N() that will add "edit own files", "delete own files" permissions for existing users to avoid unexpected issues in existing installations. What do you think?

I would like to receive some feedback before going forward and fixing other tests.
Are we going in the right direction here?

...

This makes patch #59 work on the current 8.7.x

Patch #66 adds the new permission but does not seem to be checking it anywhere. I think it is missing that piece from the previous patch.

Additionally, #59 still applies properly for me in Drupal 8.9, so I'm not sure there is even a need to re-roll that.

#62 looks like a good change to me, but I haven't had a chance to try it out yet.

working on it.

Kindly review a patch.

Patch in #59 is working for me on 8.9.1.

Using it defensively on a site with a custom view for 'Delete files' that has the bulk options but only shows temporary files, and only admins can access it. The bulk options are not available on the files overview to prevent anyone from doing anything too destructive :)

This is partly to get around the access checking issues with bulk actions. As @dww reminded me, these will allow you to use an action even if you don't have permission. So by removing the bulk options from the overview page and only adding them to a custom view that uses the 'Delete any file' permission, we can make sure the bulk action is only used by those with the correct permissions.

While the above patch adds the permission, it's still not useful OOTB without an additional module like VBO. Shouldn't we also add the delete-form link template and form class to the entity like file_delete_ui does?

This is a bit of a side track, but if you're in the situation that you need to delete *all* your files, you can do it with Drush: drush edel file. A lot easier than VBO and the other suggestions I found.

Removing reference to user 1 from title. We're actually trying very hard to de-specialize uid 1 in #540008: Add a container parameter that can remove the special behavior of UID#1.

is this slated for 9.3 release?

is this slated for 9.3 release?

It depends on folks moving it forward

Thanks @iarowlan, fingers crossed. Thanks everyone for the work here.

Attached patch for latest 9.3.x

Credit to #55
I just modified it a bit to work with Drupal Core 9.3.9

We have found the patch in #89 working.

But there seems to be a bug with Drupal Core itself because we a user can remove their own Media Files when they are being used before applying this patch.

And after applying this patch, an admin can remove files by any user whether they are being used or not, which does not seem prudent to us.

However, it is very nice to finally be able to remove Media Files uploaded by another user!

Hello

Just adapted #72 for working in 9.3.16

9.3.x is only security releases now.

Looks like patch at #94 is missing the newline at end of file, so is considered a corrupt patch file.

Rerolled #94

Fixing custom commands failure issue in #97, please review it.

I will work on this.

Rerolled the patch for 9.5.x.
Please review.

Resolved the custom command failed issue in #103.
Please review.

+++ b/core/modules/file/file.module
@@ -585,6 +587,31 @@ function file_theme() {
+function file_file_access(FileInterface $file, $op, AccountInterface $account) {

As per feedback on the MR, there's no reason this should be in a hook instead of the FileAccessControlHandler class.

Just as a tought we propably need schema update because most of the sites have already some file fields enabled so uninstall is propably out of the question?

Disregard https://www.drupal.org/project/drupal/issues/2949017#comment-14706968 permissions are in config....

As per @acbramley's comment hook_file_access has been removed in favor of FileAccessControlHandler. Also Kernel/AccessTest.php was failing so it has been fixed.

Removed left overs from file.module

We're going to need an update path to grant the delete the files you own permission to all the roles. There's an interesting question about the anonymous role because currently if a file is owned by user 0 then the anonymous user can delete it?!?!!?

Patch attached:

• Uses the same logic as node_node_access() and does the same with cacheability
• Fixes the tests
• Uses more modern PHP in some cases - due to the update path this can only be 10.1.x

Here's more test coverage of the cacheability metadata so we can be sure it stays correct.

Fixing UserCreationTrait/PHPStan over in #3319683: Fix PHPStan errors from UserCreationTrait and AssertMailTrait... will upload a patch with an update path once that lands.

Here's a tested upgrade path and some more tidy up of the tests. The biggest change here is the of the "edit any files" and "delete any files" permissions. I think the correct english here is "edit any file" and "delete any file".

We're going to need a change record and release note to announce this change.

Hmmm #119 highlights 2 issues..

1. user_post_update_update_migrated_roles_followup() needs to run before our update - and post updates do not have ordering. Thinking about it now user_post_update_update_migrated_roles_followup() needs to be a hook_update_N because it fixes something that will cause an exception.
2. We need to change \update_invoke_post_update() so that severity_level is unset when using the output of \Drupal\Core\Utility\Error::decodeException() to provide placeholders for TranslatableMarkup.

Fun, not fun.

Created issues for #121 see:

1. #3319839: Exceptions in post updates are very broken since #3295625
2. #3319845: user_post_update_update_migrated_roles_followup() needs to be a hook_update_N

1. +++ b/core/modules/file/tests/src/Functional/FileAddPermissionsUpdateTest.php
   @@ -0,0 +1,58 @@
   +      $permissions = $role->toArray()['permissions'];
   +      $this->assertNotContains('edit own files', $permissions);
   +      $this->assertNotContains('delete own files', $permissions);
   

   any reason not to use Role::hasPermission here? (and below too)

2. +++ b/core/modules/file/tests/src/Functional/FileAddPermissionsUpdateTest.php
   @@ -0,0 +1,58 @@
   +    $role = Role::load('administrator');
   

   we should assertTrue($role->isAdmin()) here too so there's at least one positive assertion

3. +++ b/core/modules/file/tests/src/Kernel/AccessTest.php
   @@ -51,37 +35,74 @@ protected function setUp(): void {
   +    $this->assertSame(2, (int) $user_any->id());
   

   Should we assertGreaterThan(1 instead because the fact it's 2 isn't important, the fact its > 1 is

4. +++ b/core/modules/file/tests/src/Kernel/AccessTest.php
   @@ -51,37 +35,74 @@ protected function setUp(): void {
   +    $file1 = File::create((array) $test_files[0]);
   

   sneaky

5. +++ b/core/modules/file/tests/src/Kernel/AccessTest.php
   @@ -112,15 +133,27 @@ public function testCheckFieldAccess() {
   +    $this->assertFalse($file->access('create'));
   ...
   +    $this->assertFalse($file->access('create'));
   

   Is this actually correct?

   Shouldn't create access be checked against the access handler and not an entity? (realise this is an existing issue)

6. +++ b/core/phpstan-baseline.neon
   @@ -1615,6 +1615,21 @@ parameters:
   +		-
   +			message: "#^Method PHPUnit\\\\Framework\\\\Assert\\:\\:assertSame\\(\\) invoked with 4 parameters, 2\\-3 required\\.$#"
   +			count: 1
   +			path: modules/file/tests/src/Kernel/AccessTest.php
   +
   +		-
   +			message: "#^Method PHPUnit\\\\Framework\\\\Assert\\:\\:assertTrue\\(\\) invoked with 3 parameters, 1\\-2 required\\.$#"
   +			count: 1
   +			path: modules/file/tests/src/Kernel/AccessTest.php
   +
   +		-
   +			message: "#^Method PHPUnit\\\\Framework\\\\Assert\\:\\:fail\\(\\) invoked with 2 parameters, 0\\-1 required\\.$#"
   +			count: 1
   +			path: modules/file/tests/src/Kernel/AccessTest.php
   +
   

   Is this intentional?

Thanks for the review @larowlan.

1. Yes - because hasPermission checks if the role is an admin role and we are not adding the permission to the admin role. This allows us to test that.
2. Well we're asserting that the role exists (by calling methods on it). If the admin role got the isAdmin flag changed then the test would fail.
3. Sure why not. - will be fix on next patch.
4. I copied this from somewhere.
5. Yes - Files are always created programatically and create access is not check. Funky and existing stuff :)
6. Yeah it has to be there for tests to run - see #3319683: Fix PHPStan errors from UserCreationTrait and AssertMailTrait - once that lands we can remove this.

The blockers (so far) are in... rerolling the patch to remvoe the PHPStan stuff and address #123. Hopefully green.

Oops forgot to commit removal PHPStan baseline changes... ignore #125

Let's fix a comment and remove unrelated change.

So the patch is working and allows people using VBO to delete a file. And it works nicely with the Media FIle Delete module. However, with vanilla core I think we might need to consider adding a delete button to the files view as this patch currently does not give you a UI to delete a file with Drupal Core alone. Alternatively we could handle the UI considerations in a follow-up.

I would vote for leaving the UI to a separate follow up so we can engage the UX team. I'll try to review this again in the next seven days

I'm OK to push that to a follow-up but then we don't really solve the problem here, seems a bit weird. More like a blocker for this then that would need to get committed first and then we'd get back to a UI here.

But a default bulk operations and views entity operations field should be simple enough to add, so we could combine it. I'd say our default UI patterns don't really need a UX review?

I need to have a closer look as well, but I do have two thoughts already:

* This is going to conflict/overlap with existing permissions and logic in the file_entity module. It's not used much anymore, but I expect it will need to be updated for this.

* It's a bit unclear to me why this includes edit permissions. Core has no UI for editing files and I don't see us ever adding that and if we're not careful and open that up through REST/JSON API then that could result in really weird problems or even security issues (for example, being able to edit the filename).

I think I like @berdir is suggesting here. Add a delete form and button to the files listing and remove the edit permission as out of scope. Will work on this next week if no one beats me to it.

Sounds good, Berdir is right - we have patterns for this stuff

For #131

Added a release note snippet

Added a draft change record

Updated the issue summary

I used the patch of #127 about the usage of "Fancy File Delete" module but on the step "Update Database" I received the following error:

$ drush updb -y

PHP Fatal error:  Cannot use Drupal\Core\Config\Entity\ConfigEntityUpdater as ConfigEntityUpdater because the name is already in use in /var/www/web/core/modules/file/file.post_update.php on line 13

Fatal error: Cannot use Drupal\Core\Config\Entity\ConfigEntityUpdater as ConfigEntityUpdater because the name is already in use in /var/www/web/core/modules/file/file.post_update.php on line 13
 [warning] Drush command terminated abnormally.

I'll investigate more and (I hope) will post more info about the issue... We are on Drupal 9.5.0 release & PHP 8.1

There is no issue when we use the correct for Drupal 9.5.x core #114 patch.

@vuil it looks like use Drupal\Core\Config\Entity\ConfigEntityUpdater; already exists in the D9 version of file.post_update.php but it's gone in D10 and somehow the patch applies cleanly but duplicates that line. Removing it allows the update to apply cleanly.

Here's what I get in D9.3 when the patch applies:

use Drupal\Core\Config\Entity\ConfigEntityUpdater;
use Drupal\Core\File\FileSystemInterface;
use Drupal\field\Entity\FieldConfig;
use Drupal\file\Plugin\Field\FieldType\FileItem;

use Drupal\Core\Config\Entity\ConfigEntityUpdater; <-- duplicate
use Drupal\user\RoleInterface;

No change necessary for the D10 version, this is just information for folks trying to backport the patch.

I've attached a backport of #127 for D9.5 (and back to 9.3 at least)

Exactly i ran in to this issue, thanks for backporting to D9 @froboy

Removed the edit file permissions from the patch - this can be dealt with in a separate issue - if we need to deal with it at all.

Here's a UI for deleting files based upon entity defaults. Unfortunately we can't use \Drupal\views\Plugin\views\field\EntityOperations for two reasons - file does not have a list handler - and if we did then we'd have to work out what to do with the update access as file owners are allowed to do that but we have no UI for it.

Ok re-read EntityListBuilder again - that won't add an update operation because we have no update link template for File (because no form). So that's good. So I've added a list builder and added the routes necessary for a file listing without views and added tests.

Thinking about all this code - it might be possible to use the default list builder and not provide any routes to it. Then we wouldn't have the complexity of maintaining a non-views UI but we'd be able to add the operations field in views.

Yeah I don't think this issue should be adding a non-views UI if it doesn't have to. And it does not.

+++ b/core/modules/file/file.post_update.php
@@ -13,3 +16,17 @@ function file_removed_post_updates() {
+    $role->grantPermission('delete own files');

I think we need to update the default roles in umami/standard to add this permission to keep the existing functionality

Also I'm seeing PHP warnings in Umami when I install and visit the files overview

Warning: Undefined array key 0 in Drupal\file\FileAccessControlHandler->checkAccess() (line 67 of core/modules/file/src/FileAccessControlHandler.php).

There's not change to that line in this patch, so it might be an existing issue with Umami and how it creates the files for default content (i.e. without adding a UID). Looking at the File entity, it appears as though the UID field is not required. I did search for an existing issue and couldn't find one - should we tidy this up here whilst we're at it - or alternatively add another bug to resolve it.

Adding some screenshots for the UI changes

@larowlan great catch!

I've added test coverage for files with no related user entity. Yes it kind of is an existing bug but we trigger it due to views checking 'update' access when we get the available operations - it eventually calls out to \Drupal\Core\Entity\EntityListBuilder::getDefaultOperations() and this does if ($entity->access('update') && $entity->hasLinkTemplate('edit-form')) { - there is definitely an argument for those conditions being flipped as the link template getting is undoubtedly cheaper - but hey it revealed the bug here.

I've also added the correct perms to each role for Umami.

Fixing the interdiff...

Should we be adding that permission to the roles in standard too?

It has user.role.content_editor and user.role.authenticated.

Looks like there were 6 fails in the umami tests in that test run, not sure why its not being surfaced here.

Fixed umami and added permissions to standard roles.

I think the patch at #149 (not the MRs) is RTBC

Thanks @alexpott for keeping this moving

Updated the release notes and added the release highlights tag.

Warning for users that try to apply the patch from #149 on Drupal 9.5: that will cause the following error:

Cannot use Drupal\Core\Config\Entity\ConfigEntityUpdater as ConfigEntityUpdater because the name is already in use in /web/core/modules/file/file.post_update.php on line 13

For Drupal 10.0 this problem is not present, because there file.post_update.php has no class imports: https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/modules/fil....

In case anyone needs it, here's #149 re-rolled to 9.5.x. The only change is the updated imports mentioned by @MegaChriz.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Bot was tripped up by the last patch for a different branch than the issue's.

Let's hide the patch that's not relevant for the commit.

Excluding this issue from bot review since
1. The last patch is for a different branch and no tests were run on it (as such I'm not able to know it's for a different branch and to ignore it)
2. There is an open MR more recent than the actual patch that needs to be committed.

Few notes:

1. Was surprised to see that File didn't have a list builder and why are we adding it here. This is explained in #143.
2. The install profile changes seem fine. I like that the default config is built based on what makes sense rather than just applying the post update. Re-installed with Standard and Umami to make sure the defaults make sense and #145 has been addressed.
3. Confirmed that sorting the table is sufficient for ensuring deterministic tests. Looks like there aren't any files with random names meaning this is fine.
4. Dropbuttons with a single option in Claro look a bit strange but eventually this will be addressed by #1899236: Add new Splitbutton render element to eventually replace Dropbutton.

5. +++ b/core/modules/file/file.post_update.php
   @@ -13,3 +16,17 @@ function file_removed_post_updates() {
   +  /** @var \Drupal\views\ViewsConfigUpdater $view_config_updater */
   

   Removed this comment on commit.

Committed 0d58b3c and pushed to 10.1.x. Thanks!

Not backporting to 10.0.x or 9.5.x because of the post update hook.