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
Add tests that demonstrate that the source field allowed extensions can be changed for file fields.File a followup issue to discuss other methods for preventing certain source field settings from being changed: #2895879: Discuss (and agree upon) method(s) for preventing certain Media Source field settings from being changedFile a separate issue for test coverage ensuring the source field cannot be deleted: #2895581: Add test coverage ensuring that the media source field cannot be removed from a media type.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#34 | 2894270-34.patch | 8.17 KB | Wim Leers |
#34 | 2894270-34-test_only_FAIL.patch | 2.06 KB | Wim Leers |
#5 | media-2894270-5.patch | 728 bytes | xjm |
#9 | 2894270-9-fail.patch | 3.05 KB | seanB |
#9 | 2894270-9.patch | 5.64 KB | seanB |
Comments
Comment #2
seanBComment #3
xjmDiscussed with @seanB. We added the locking originally to solve a couple things:
hook_entity_operations_alter()
and
hook_entity_access()
implementations that also prevent that.@Boobaa wrote:
@seanB replied:
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:
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).
Comment #4
seanBThanks xjm for looking into this. The core seems to be:
This is already possible by providing default config when installing the module.
This is already possible by implementing
createSourceField()
in the MediaSource plugin.This allows all fields of a type (for example all entity reference fields in the case of Brightcove). This should be solved.
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.
Comment #5
xjmMy 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...
Comment #6
xjmAlso, some tests for #5 should hopefully fail, because we added tests for the locking when we added it, right? :D
Comment #7
xjmUm, or not.
Comment #8
xjmComment #9
seanBWe had tests! Here are 2 patches, 1 that should fail only changing the locking. The other one adjusting the tests accordingly.
Comment #13
seanBOk let's try again...
Comment #15
xjmThe fail is:
Comment #16
seanBRemoved a little too much. This should be it.
Comment #17
xjmI 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
Comment #18
xjmLOL well, reassuringly, @seanB and I uploaded the exact same patch at the same time. Seems a good sign.
Comment #19
seanBHaha awesome!
Comment #20
xjmCool, 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.
Comment #21
chr.fritschI 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?
Comment #22
xjmThanks @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.
Comment #23
xjmOkay, 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.
Comment #24
xjmComment #25
chr.fritschHere is the follow-up: #2895581: Add test coverage ensuring that the media source field cannot be removed from a media type
Comment #26
chr.fritschHere is a new patch that verifies that it's possible to change the allowed file extensions.
Comment #27
xjmJust reuploading @chr.fritsch's test to expose the coverage.
Comment #28
xjmI 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.
Comment #30
chr.fritschOk, here with the test as BTB test.
Comment #31
xjm@chr.fritsch, please make sure to provide test-only FAIL patches as well to expose the coverage, as I did in #27. Thanks!
Comment #32
Wim LeersI'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:
Now reading the rest of this issue…
Comment #33
Wim Leers#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.
This is possibly true.
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:
Comment #34
Wim LeersFixing a few nits in #30 and at the same time addressing #31 by uploading a test-only patch.
Comment #35
Wim LeersI'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.
Comment #36
Wim LeersComment #38
Gábor HojtsySuperb, looks good. Thanks!