Users unable to add an extension to the file upload field

Discussed with @seanB. We added the locking originally to solve a couple things:

• To disallow deleting the field and messing up your media entity references. Later, we added hook_entity_operations_alter()
  and hook_entity_access() implementations that also prevent that.
• To disallow changing the selected bundles for specific media (e.g. Brightcove):
  @Boobaa wrote:

  Regarding the defaults: for complex stuff (like our Brightcove Video entities), MediaSource plugins might want to have their own defaults for their fields instead of relying on the defaults the field type provides. Particularly in our case, we prepared a custom inline_entity_form for handling our brightcove_video entities when they're linked eg. from a node via an entity reference field. While the generic, core-provided defaults for entity reference fields might be sufficient for most cases, it's not not the best for our admittedly complex case.

  But hey, as you already suggested on IRC, the MediaInterface does not need any changes for this to work – more details on this can be found in the followup (which has some copy-pasted code that actually works, btw).

  @seanB replied:

  So after discussing this on IRC it seems the following is the case (@boobaa/@slashrsm please correct me if I'm wrong)

  • MediaSources are responsible for create a source field through configuration and providing a Media Type using this, the settings should not be a problem there.
  • When create a new Media type, users can choose to use a source field of their own. This is where we might want to validate some settings on the source field as mentioned by boobaa.
  • Add validation for source field settings. At a minimum there should be validation when saving the Media type, but since you can edit the source field you might want to validate every time the source field is saved, or prevent changing some settings on known source fields.

  We just discussed the issue of boobaa in #291 on IRC. The following is a summary of what was discussed. Please correct me if I'm wrong!

  1. Creating a source field for a default MediaType in a module can be done by providing default config. This is already possible.
  2. Auto creating a source field for a manually created MediaType can be done by implementing createSourceField() in the MediaSource plugin. This is already possible.
  3. Reusing existing source field for an manually created MediaType currently allows all fields for a type provided in the annotation of the MediaSource plugin. It would help to allow something like getSourceFieldCandidates() on the MediaSource plugin, to allow plugins to limit the source fields a user can select. Maybe there are alternatives since we want to reduce the surface of MediaSourceInterface?
  4. We need to lock source fields to make sure users can't change the settings afterwards. I think this can already be done through createSourceField()?

  Boobaa suggested adding settings through annotations. Allowing field settings for the source field through an annotation could remove the need to override createSourceField() in #2. The settings could at the same time be used to filter the list of source fields and remove the need for #3. Locking auto created source fields by default could remove the need for #4.

  The annotation's look like the easier solution for module developers. Any thoughts?

  We can lock all source fields storage configuration the module creates, but you can re-use existing fields as well. Maybe we can lock storage config for all fields selected as a source field when saving a media type? Since not locking fields could cause issues we should probably do this.

  Locked storage config for all fields selected as a source field when saving a media type.

However, in this case, I think the solution (locking the field, and thereby preventing settings like file extensions to be edited, as well as #2894271: Users unable to change a media source file/image from public to private) is worse than the bug (being able to edit things you should probably not when you install a media module).

I think we should remove the auto-locking for 8.4.x, and then look into other ways to address the bug. @seanB agreed and suggested we might look at the different things that the locking was intended to solve separately:

Media sources should be able to lock certain thing in the field settings and in the field storage, and also should be able to prevent deleting a field. I think these are 3 separate issues.

However, we wanted to get additional feedback on that before proceeding. Unfortunately, I do think we need to fix this before 8.4.0 ships, but I think simply unlocking the field would be fine until we possibly come up with better solutions later (as a should-have).

My preference would be to remove the locking first, and then to add followup issues to prevent users from breaking their media-related field settings. Here's a starter patch in either case...

Also, some tests for #5 should hopefully fail, because we added tests for the locking when we added it, right? :D

Um, or not.

The fail is:

1) Drupal\KernelTests\Config\DefaultConfigTest::testModuleConfig with data set "media" ('media')
Exception: field.storage.media.field_media_file: Drupal\Component\Diff\Engine\DiffOpAdd::__set_state(array(
   'type' => 'add',
   'orig' => false,
   'closing' => 
  array (
    0 => 'locked: false',
  ),
))

/var/www/html/core/tests/Drupal/KernelTests/AssertConfigTrait.php:81
/var/www/html/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php:142
/var/www/html/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php:109

I think we need to keep the locked lines there to comply with the schema.

DefaultConfigTest is running locally, but testbot might finish before it does. :P

LOL well, reassuringly, @seanB and I uploaded the exact same patch at the same time. Seems a good sign.

Cool, so that's working. So now we just need a web or BTB test confirming that the file extension list can be configured for media fields, both without and with data in the field. Edit: the test could probably also have two instances of the field on two different content types or such, and have test for the extension list as allowed on both of them.

We probably want one media contributor other than Sean or myself to confirm that this is an OK idea, but I would be comfortable with the patch above plus the tests I mention to make this shippable for 8.4.

We would open a required followup for discussing whatever other strategy we want to use to restrict access to some of the field and field storage settings for media fields. @seanB and @phenaproxima were brainstorming about alternative solutions a bit on Friday.

We could also add tests for changing the file storage to this issue as well, and close the postponed issue as a duplicate.

I am also fine with the solution to unlock the fields and try to prevent changing of some settings later.

Now, when fields are not locked anymore, media_entity_operation_alter and media_entity_access will be executed. media_entity_operation_alter is not needed anymore since #2836384: Field UIs operations array is broken for entities with restricted access was committed. Should we remove this hook in this patch?

Thanks @chr.fritsch. It could make sense to add test coverage for the deletion here since we're removing the other (UI) protection against deleting the fields. Checking to see whether we have test coverage for that already by removing both hook implementations in a test-only patch, which should fail to show what coverage we have already if any.

If we don't already have test coverage for the operations, then I'd suggest a separate issue for it.

Okay, well, so there isn't test coverage for ensuring the thing can't be deleted. So I'd recommend a separate issue, "Add test coverage ensuring that the media source field cannot be removed from a media type (and remove unneeded media_entity_operations_alter())".

And then let's keep the scope here to just removing the locking and adding tests to verify the list of allowed extensions can be changed for media source fields.

Here is the follow-up: #2895581: Add test coverage ensuring that the media source field cannot be removed from a media type

Here is a new patch that verifies that it's possible to change the allowed file extensions.

Just reuploading @chr.fritsch's test to expose the coverage.

I don't think this needs to be a JS test. JS tests are extremely slow compared to web/BTB tests, so we should probably use a BTB test.

Ok, here with the test as BTB test.

@chr.fritsch, please make sure to provide test-only FAIL patches as well to expose the coverage, as I did in #27. Thanks!

I'm confused why this issue was opened instead of continuing in #806102: Locked fields can change the widget but not settings. I guess this was opened to work around the problem for Media, so Media can be shipped in 8.4.0, meaning that we don't want to fix the root cause?

I did some digging into the root cause, and posted that over at #806102-34: Locked fields can change the widget but not settings, at the same time that this issue was created (both this issue and my comment were due to last week's Media meeting). That's how I didn't notice this issue until two days ago.

The comment I posted there:

Let's get this back on track.

1. 
   This is what happens in Field UI when you lock a FieldStorageConfig entity (source: #2831936-98: Add "File" MediaSource plugin).
2. When there's no lock, this is what happens:

   

3. But … the locking actually only affects \Drupal\field\Entity\FieldStorageConfig — \Drupal\field\Entity\FieldConfig does not even have the concept of locking!
4. Consequently, what should happen, is that the "Storage settings" option in the dropdown in step 2 should have been removed. But what actually happens is that everything gets removed.

This is caused by \Drupal\field_ui\FieldConfigListBuilder::buildRow() doing this:

    $field_storage = $field_config->getFieldStorageDefinition();
…
    if ($field_storage->isLocked()) {
      $row['data']['operations'] = ['data' => ['#markup' => $this->t('Locked')]];
      $row['class'][] = 'menu-disabled';
    }

Rather than just some operations becoming inaccessible.

I tried to find the root cause. This code was introduced in #1963340: Change field UI so that adding a field is a separate task, but there it was just moved from the now-removed core/modules/field_ui/src/FieldOverview.php. This was also touched in #2287727: Rename FieldConfig to FieldStorageConfig (D8 alpha 14), #1953408: Remove ArrayAccess BC layer from field config entities (D8 alpha 4), #1549506: Edit and delete links should be hidden for locked fields (D8 alpha 1) and finally, #1792600: Refactor field_ui so common behavior for fields and display overview screens are extracted (October 5, 2012). Except, nope, that's also not the one, because it just moved it from field_ui_field_overview() in the now-removed field_ui.admin.inc. Before that … drumrolls … it was #516138: Move CCK HEAD in core — the patch that brought CCK to core! That very first commit includes:

if (!empty($instance['locked'])) {

So it seems this was simply an oversight from August 19, 2009?

Now reading the rest of this issue…

#3

This mentions Media module's hook_entity_operations_alter() implementation. That points to #2836384: Field UIs operations array is broken for entities with restricted access. Adding that as a related issue, because that alter hook implementation will be removed once that issue lands. And actually… that issue already landed. So we can remove it. Opened #2895857: Remove media_entity_operation_alter() as planned earlier.

The thing that Boobaa was suggesting that seanB was referring to is #2865184: Allow MediaSource plugins provide default field form/view display settings.

I think the solution is worse than the bug

This is possibly true.

but I think simply unlocking the field would be fine until we possibly come up with better solutions later (as a should-have).

Sounds good!

#21 Hah, I already did that! Hope that's okay.

#25: Great — #2895581: Add test coverage ensuring that the media source field cannot be removed from a media type is about adding test coverage for media_entity_access(), #2895857: Remove media_entity_operation_alter() as planned earlier is about removing something that we already know to be safe (because #2836384: Field UIs operations array is broken for entities with restricted access added test coverage), that should have been removed already.

Added #2895581 to IS.

I manually verified that #30 removes all locked => TRUE cases.

I also manually tested the patch: uninstalled Media, installed with this patch applied, the default File and Image Media Types no longer have their Source field locked, nor does a newly created Media Type have its Source field locked. I was able to change the allowed file extensions configuration in all cases.

Finally, the test coverage looks fine.

I think that the only remaining task is:

2. File a followup issue to discuss other methods for preventing certain source field settings from being changed.

Fixing a few nits in #30 and at the same time addressing #31 by uploading a test-only patch.

I'd RTBC per #33 (in #34 I'm only making superficial changes), but we still need that follow-up issue I mentioned at the bottom of #33. We already have a bunch of those issues in the Entity/Field system components, but we want a particular one for Media.

So, created that follow-up: #2895879: Discuss (and agree upon) method(s) for preventing certain Media Source field settings from being changed.

No more remaining tasks. Patch that's ready. So: RTBC.

Superb, looks good. Thanks!