Problem/Motivation

When adding the media module to core, it was decided that source field should be locked. This was done because we don't want them to be deleted, and in some cases they also shouldn't be changed (the brightcove media source uses an entity reference as source field where a specific entity bundle should be selected).

There are several other issues on the locked field topic:
#806102: Locked fields can change the widget but not settings
#2274433: Do not allow to alter Locked field via UI
#2289551: Clarify what 'locked' means for a config entity and whether it's okay for code to rely on a locked config entity existing
#2894271: Users unable to change a media source file/image from public to private

The problem is, locking the source field also removed our ability to change the list of file extensions for the "File" and "Image" media types.

Proposed resolution

Remove the locking on the source field. File a followup issue to prevent editing for certain source field settings. Deletion of the source field is already prevented by media_entity_access().

Remaining tasks

User interface changes

API changes

Data model changes

Comments

seanB created an issue. See original summary.

xjm’s picture

Priority: Normal » Major

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).

seanB’s picture

Thanks xjm for looking into this. The core seems to be:

  1. Creating a source field for a default media type in a module.
    This is already possible by providing default config when installing the module.
  2. Auto creating a source field when manually creating a Media type
    This is already possible by implementing createSourceField() in the MediaSource plugin.
  3. Reusing an existing source field for a manually created media type
    This allows all fields of a type (for example all entity reference fields in the case of Brightcove). This should be solved.
  4. Changing a source field after it is created
    In some cases, for instance Brightcove, some settings of the source field should not be changed. This should be solved.

So to sum it up, I think these are the actual things to fix.

  • Remove locking source fields since this prevents us from changings certain settings that should be available (most settings for most fields are no problem to change).
  • Make sure users can only select source fields that make sense for the media type. Filtering by field type is not enough.
  • Make sure some field settings or field storage settings can not be changed after the field is created. The media source should be able to lock certain settings somehow.
xjm’s picture

Status: Active » Needs review
FileSize
728 bytes

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...

xjm’s picture

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

xjm’s picture

Issue tags: +Needs tests

Um, or not.

xjm’s picture

Status: Needs review » Needs work
seanB’s picture

We had tests! Here are 2 patches, 1 that should fail only changing the locking. The other one adjusting the tests accordingly.

The last submitted patch, 9: 2894270-9-fail.patch, failed testing. View results

The last submitted patch, 9: 2894270-9.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 9: interdiff-5-9.patch, failed testing. View results

seanB’s picture

Status: Needs work » Needs review
FileSize
5.59 KB
2.54 KB

Ok let's try again...

Status: Needs review » Needs work

The last submitted patch, 13: 2894270-13.patch, failed testing. View results

xjm’s picture

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
seanB’s picture

Status: Needs work » Needs review
FileSize
5.62 KB
1.02 KB

Removed a little too much. This should be it.

xjm’s picture

FileSize
5.62 KB
1.02 KB

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

xjm’s picture

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

seanB’s picture

Haha awesome!

xjm’s picture

Status: Needs review » Needs work

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.

chr.fritsch’s picture

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?

xjm’s picture

Status: Needs work » Needs review
FileSize
1.75 KB

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.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs followup

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.

xjm’s picture

Issue summary: View changes
chr.fritsch’s picture

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
7.12 KB
1.34 KB

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

xjm’s picture

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

xjm’s picture

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.

The last submitted patch, 27: media-2894270-16-26-FAIL.patch, failed testing. View results

chr.fritsch’s picture

Ok, here with the test as BTB test.

xjm’s picture

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

Wim Leers’s picture

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…

Wim Leers’s picture

#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:

  1. File a followup issue to discuss other methods for preventing certain source field settings from being changed.
Wim Leers’s picture

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

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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.

Wim Leers’s picture

The last submitted patch, 34: 2894270-34-test_only_FAIL.patch, failed testing. View results

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Superb, looks good. Thanks!

  • Gábor Hojtsy committed 4b95aa7 on 8.4.x
    Issue #2894270 by xjm, seanB, chr.fritsch, Wim Leers: Users unable to...