Problem/Motivation
Followup for and postponed on #2831274-372: Bring Media entity module to core as Media module.
As createSourceField()
only creates the field_config but does not save it, there's no way to change the MediaSource entity form display for this field to a more sane default from code (like a custom derivative of inline_entity_form in my case, see below). As this method is called from MediaTypeForm::save()
(and not from anything in MediaSourceBase), there is no way to even intervene. There's not even a hook there. In the contrib era, this was doable via hook_media_bundle_insert()
/hook_ENTITY_TYPE_insert()
, but this isn't available either.
We should allow the MediaSource plugin (or at least the module providing it) to change the form display settings so they can be "good enough". In the contrib era, brightcove.module solved this by changing the form display settings from hook_ENTITY_TYPE_insert()
:
function media_entity_brightcove_media_bundle_insert(MediaBundle $bundle) {
$field_name = $bundle->type_configuration['source_field'];
$form_display_settings = [
'type' => 'brightcove_inline_entity_form_complex',
'settings' => [
'form_mode' => 'default',
'allow_new' => 1,
'allow_existing' => 1,
'match_operator' => 'CONTAINS',
],
'third_party_settings' => [],
'weight' => 0,
];
// Create (or update) the entity form display for this new media bundle to
// include this new field with some more sane defaults.
/** @var \Drupal\Core\Config\Entity\ConfigEntityStorage $entity_form_display_storage */
$entity_form_display_storage = \Drupal::getContainer()->get('entity_type.manager')->getStorage('entity_form_display');
/** @var \Drupal\Core\Entity\Entity\EntityFormDisplay $entity_form_display */
$entity_form_display = $entity_form_display_storage->load('media.' . $bundle->id() . '.default');
if (!$entity_form_display) {
$values = [
'status' => TRUE,
'targetEntityType' => 'media',
'bundle' => $bundle->id(),
'mode' => 'default',
'content' => [
$field_name => $form_display_settings,
],
];
$entity_form_display = $entity_form_display_storage->create($values);
}
else {
$entity_form_display->setComponent($field_name, $form_display_settings);
}
$entity_form_display_storage->save($entity_form_display);
}
As you can see, that approach wasn't the best one either, because the field wasn't created with appropriate form display settings (honestly, I don't really know if it's even possible), but those settings were only changed afterwards.
OTOH, it's clearly visible that it's only about providing that $form_display_settings.
Proposed resolution
Add a new method to MediaSourceInterface which can provide default display settings for a new media type, for both the form and view entity displays of said media type.
Remaining tasks
Write a patchReview itSmile upon it- Commit it
User interface changes
None.
API changes
MediaSourceInterface will get a new method, and MediaSourceBase will provide the default implementation.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#47 | interdiff-2865184-44-47.txt | 7.62 KB | chr.fritsch |
#47 | 2865184-47.patch | 12.39 KB | chr.fritsch |
#44 | interdiff-2865184-36-44.txt | 8.99 KB | chr.fritsch |
#44 | 2865184-44.patch | 12.63 KB | chr.fritsch |
#36 | interdiff-2865184-34-36.txt | 2.85 KB | chr.fritsch |
Comments
Comment #2
BoobaaOh, and the same thing could be done for the field view display settings as well.
Comment #3
BoobaaI did some more investigation and I found
hook_ENTITY_TYPE_insert()
/hook_media_type_insert()
. However, it doesn't really help us to solve the problem, because this hook runs during theparent::save()
call inMediaTypeForm::save()
(which is the very first line of it, of course). Once the source field is created and saved (later inMediaTypeForm::save()
), its form and view displays are immediately overwritten with the defaults for the actual field type.So it looks like we can't use this hook for changing the default display modes, so it looks like OP is still the best proposed resolution (so far, with the current state of the core patch).
Comment #4
BoobaaMedia patch is in, so this is no longer postponed.
Comment #5
Wim LeersSee #2831943-119: Use "rendered media" (not links) as default media field formatter; add modal to configure the used media view mode.
Comment #7
chr.fritschIn #2863648: Consider providing a mechanism for media source field validation we decided against adding field settings to the annotation and rather use the existing API. So this should probably be solved by introducing some new methods on the MediaSourceInterface.
Comment #8
chr.fritschHere is a first draft
Comment #9
BoobaaFirst, this is an API change, and IDK if it's acceptable in this phase.
Second, there might be field widgets/formatters that MUST have non-empty default settings. IOW, the fallback should be the default settings provided by the widget/formatter, not an empty array.
Comment #10
chr.fritschI think adding new methods to an interface should be possible by https://www.drupal.org/core/d8-allowed-changes#minor
setComponent
calls\Drupal::service('plugin.manager.field.widget')->prepareConfiguration($field_type, [])
internally. So it's save to provide an empty array.Comment #11
phenaproximaThis is a cool idea, and thanks for the initial patch!
It's definitely an API addition, but if policy allows it, then I think we can go for it. I can't imagine too many scenarios where media source plugins will not be extending MediaSourceBase, so as long as we add a base implementation to accompany the new interface method, I think we have our bases covered.
I think we should pass the initial formatter settings ($component, in MediaTypeForm) to this method by reference and allow it to alter that, rather than ask it to return complete settings.
I also question if we need two methods. Maybe we can do something like this:
public function alterDisplayOptions(array &$component, $display_context)
...where $display_context is either 'form' or 'view'. What do you think?
Comment #12
phenaproximaAnother thought occurs to me -- what if the media source wants to hide the component entirely?
My first thought is that the method(s) should return a boolean to indicate if the component should actually be visible. If it's false, we don't add the component at all; otherwise, we do. Thoughts?
Comment #13
chr.fritschI think then it’s fine to return type => hidden
Comment #14
phenaproximaThat's not the same thing as hiding the component, though. Calling
$display->removeComponent('foo')
will add the component name to a special 'hidden' array, but completely remove it from the 'content' array. Returning['type' => 'hidden']
would be quite ambiguous. If we're going to alter the component by reference, it seems reasonable to allow the return value to indicate the absolute status of the component.Comment #15
chr.fritschI don't think we should alter the default options. Just return whats important for you and everything else will be added by
$this->pluginManager->prepareConfiguration
inEntityDisplayInterface::setComponent
Comment #16
phenaproximaI love this patch. I have only one request to make:
Can we pass the media type entity into this method as well, so that it can perform more advanced logic if it wants to?
Apart from that, let's get a couple of tests in here! :)
Comment #17
chr.fritschHere are the tests
Comment #18
phenaproximaFabulous work. I have nothing but minor things to point out.
Nit: "Get" should be "Gets".
Let's improve this description a bit -- maybe "The media type which is using this source"?
Needs to be explained a bit more. Maybe "The display context which will use these options. Can be 'view' or 'form'"?
Let's remove "as array", and add a comma after "The display options".
"Set's" should be "Sets" (no apostrophe).
As per convention, let's snake_case the parameter name, not camelCase.
Can't we use the entity form builder service here, rather than creating the form object statically?
Rather than call $display->getComponents() twice, can we re-use its return value?
Same here.
Same here.
Comment #19
chr.fritschThanks for reviewing @phenaproxima
I fixed all the nits.
One thing about #18.7: I was not able to use the entity form builder service, but i use
$this->container->get('entity_type.manager')->getFormObject('media_type', 'add')
instead now. This also simplified the code a bit.Comment #21
chr.fritschBack to NR because of testbot hickup
Comment #22
phenaproximaOnly two more things that I can see.
$media_type_id looks like it's the source plugin ID, so this should be $source_plugin_id or something. Also, not an int :)
Should say "...options on an entity display".
Comment #23
chr.fritschFixed the nits.
Comment #24
phenaproximaOne last thing:
Should be a string, not an int.
Comment #25
chr.fritschDamn, you said that already before....
Comment #26
phenaproximaCode looks good to go. Thank you for all you've done, @chr.fritsch!
Since we're adding something to Media's API, I think a change record is in order here, so I'm kicking this back to NW for that. Once we have a change record, this is RTBC.
Comment #27
chr.fritschHere is the CR: https://www.drupal.org/node/2928238
Comment #28
phenaproximaI think we're ready.
Comment #29
phenaproximaWe've got the change record :)
Comment #30
xjmThis should at least define a little more clearly what "the display options" are. It looks like it requires knowledge of a particular array structure for the display options that would be defined elsewhere.
I also find myself if this interface is the right place to put this method. It seems to add a coupling that
MediaSourceInterface
didn't have previously?Comment #31
phenaproximaI'm not sure what a better place for it would be. As part of its interface, the media source is already responsible for defining the source field itself, which couples it to the Field API anyway. By design, media is quite coupled to the field system. So, to me, this does not introduce any new, significant coupling. I'm open to opinions on what might be better, though. Would an alter hook be preferable? Either way, this is definitely something we need to solve.
Agreed.
Comment #32
chr.fritschI agree with @phenaproxima. We are already injecting the
MediaTypeInterface
into ::getSourceFieldDefinition() and ::createSourceField().The new method ::getDisplayOptions() is in the same scope. It's used during the source field creation.
So i just updated the docs a bit.
Comment #33
phenaproximaThis is good, but let's move it above the parameter descriptions, just below the short one-line description of the method. Also, we can remove the arguments from setComponent(), and EntityDisplayInterface should be fully qualified.
If we want to get even clearer, we can explain that if the $display_context is 'view', then the returned array should be a set of formatter options (i.e, for an entity view display), and if the $display_context is 'form', the returned array should be a set of widget options (i.e., for an entity form display). IMHO it can't hurt to document this stuff as clearly as possible, seeing as how view and form displays are already pretty confuzzling.
Additionally, I wonder if we should add an additional parameter to the method -- in this case, the view or form mode (which would default to 'default'). It would provide additional flexibility for implementations. What do you think? Something like this is what I'm envisioning:
Comment #34
chr.fritschI added the view_mode to :: getDisplayOptions. Currently there is no benefit from it, but we are a bit saver for the future.
Comment #35
phenaproximaLooks excellent! But let's rename $view_mode to $display_mode, since that's the more generic term. Also, we need to add the parameter to the default implementation in MediaSourceBase.
Comment #36
chr.fritschOk, fixed that.
Comment #38
phenaproximaLooks good, but MediaTypeForm needs to call the method will that argument too :)
Comment #39
chr.fritschNo, because we use the default 😉
Comment #40
phenaproximaOK, I guess we're back to RTBC.
Comment #41
xjmThat answers my question, thanks @phenaproxima and @chr.fritsch!
Comment #42
larowlanI agree this is needed, I just have a couple of observations that I think would make this more robust
This concerns me, mixed returns always do
What happens if someone submits it over REST? I suspect that the source plugin doesn't get the same opportunities as it does via the UI. We should avoid logic in the form. So that would probably indicate that it belongs in
MediaType::postSave()
In my opinion, it would be more powerful if the method on MediaSourceInterface was
That way the plugin has the power to call
::removeComponent
or::setComponent
as necessary.In addition the,
$display_context
argument isn't needed - as$display->getMode()
is available.But it also opens up the possibility for more flexible options too, such as ensuring the label is output by default, or modifying the weight of other fields. Not saying there is a concrete use case for that, but we can leave the door open.
Also, it gets rid of the mixed return and
MediaTypeForm::setDisplayOptions
can go too.There is also the case for splitting it into two methods.
::prepareViewDisplay
and::prepareFormDisplay
which would avoid the need for a switch. But that is just myare you sure you really want a switch?
filter kicking in and I realise this is contrived for a test....so ignore me unless you feel strongly about itMoving it to
MediaType::postSave
would also allow us to just use the::create
method here. So the fact we need to use a form in this test is another indicator that we have the code in the wrong place I thinkComment #43
phenaproximaThis is already the case. The source field is created automatically only in the UI; this display stuff should, IMHO, be handled similarly. That keeps a clean, or at least clear, separation between the way things work in UI (which will create/alter several things while creating a new media type) and the API (in which you have to create the source field, and prepare any displays, in separate calls). This was done because having the media source permanently alter config at the API level (i.e., in preSave or postSave) turned out to be fraught with problems, largely having to do with config dependencies. So I think we should stay away from that here.
That said, I agree that passing the display entity is a better idea than mixed returns, so let's do that. And I also agree that we can split this out into separate methods (which can then each receive the more specific variant of EntityDisplayInterface as their parameter).
Comment #44
chr.fritschThanks for reviewing @larowlan.
I splitted up
getDisplayOptions
intoprepareViewDisplay
andprepareFormDisplay
as you suggested and left the functionality inMediaTypeForm
because of @phenaproxima's explanation.I also think we could remove the
$field_name
fromprepareViewDisplay
andprepareFormDisplay
because it could be retrieved directly inside. What's your opinions on that?Comment #45
phenaproximaMy only question:
Do we really need to pass $field_name? The source plugin is aware of its own source field, so can't it simply use $this->getSourceFieldDefinition($type)->getName()?
Comment #46
phenaproximaWe'll need to update the change record before we can re-RTBC...
Comment #47
chr.fritschSince we are both asking this question, I think we should remove the
$field_name
.I also updated the CR
Comment #48
marcoscanoGreat work here! This will be a very much appreciated feature for modules providing source plugins.
I have manually tested this, tweaking the
Image.php
source plugin with something like:and it works beautifully.
I had some doubts about types/displays being created programmatically or through imported configuration, but I see that this concern was already answered in #43, and it is also true that if a module/distribution provides config for a display we should assume it is expected to be a "good" config for that plugin.
Also checked the change record. Fixed a minor typo and added an example snippet to make more explicit the new functionality.
IMHO this should be very close to ready.
Comment #49
phenaproximaThe change record looks great. +1 for RTBC.
Comment #51
chr.fritschTestbot hickup
Comment #52
marcoscanoPostponed #2924631: Media sources for local video and audio support on this issue.
Comment #53
larowlanAdded review credits
Comment #55
larowlanCan you open a followup to explore that?
Committed as de9e2d7 and pushed to 8.5.x
Published the change record.
Nice work all.
Comment #56
phenaproximaDone: #2930181: Media types should create their source field during pre/postSave.
Thanks, @larowlan!