Problem/Motivation

When reusing the same image in different places, the alt text of the image might be different for each use. Since images could contain a lot of elements, you might want to describe different parts of the image depending on the context of how the image is used. It is currently not possible to override the canonical alt text of a media image on a per use basis. (Similarly, different tags might be needed depending on where it is used.)

As the WCAG makes a distinction between:

  • Informative Images - where the text alternative should convey the meaning or content that is displayed visually
  • Decorative Images - where a null (empty) alt text should be provided (alt="")

This is also discussed in #3081475: According to the WCAG guidelines decorative images should not have an alt-text but this is not possible

Proposed resolution

Add an map field to the media entity reference to store the field overrides. Note the override is per fieldand not per field_property.

Remaining tasks

Add more doxygen to the media item class, especially around the per field vs per property override.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#72 interdiff_3023807_69-72.txt1.53 KBankithashetty
#72 3023807-72.patch79.53 KBankithashetty
#69 3023807-9.2.x-69.patch79.96 KBalexpott
#69 67-69-interdiff.txt3.16 KBalexpott
#67 3023807_67.patch79.22 KBanmolgoyal74
#63 3023807_63.patch79.12 KBstomusic
#62 3023807_62.patch79.12 KBstomusic
#59 3023807_59.patch79.05 KBvsujeetkumar
#52 3023807-52.patch81.73 KBoknate
#52 3023807--interdiff-50-52.txt3.03 KBoknate
#49 3023807-49.patch79.78 KBoknate
#45 interdiff-3023807-40-45.txt8.33 KBchr.fritsch
#45 3023807-45.patch79.81 KBchr.fritsch
#40 3023807-40.patch79.96 KBchr.fritsch
#40 interdiff-3023807-35-40.txt15.37 KBchr.fritsch
#39 3023807-39-REBASE.patch70.52 KBbnjmnm
#35 interdiff-3023807-33-35.txt18.1 KBchr.fritsch
#35 3023807-35.patch69.99 KBchr.fritsch
#33 3023807-33.patch60.39 KBchr.fritsch
#33 interdiff-3023807-30-33.txt883 byteschr.fritsch
#31 interdiff-3023807-29-30.txt28.13 KBchr.fritsch
#30 3023807-30.patch59.53 KBchr.fritsch
#27 interdiff-3023807-25-27.txt25 KBchr.fritsch
#27 3023807-27.patch60.77 KBchr.fritsch
#25 3023807-25.patch40.08 KBchr.fritsch
#17 3023807-17.patch28 KBchr.fritsch
#15 3023807-14-wip.patch8.95 KBchr.fritsch
#12 override-metadata.jpg410.35 KBseanB
#12 wysiwyg-image-with-button.jpg142.56 KBseanB
#12 media-library-widget.jpg382.06 KBseanB
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seanB created an issue. See original summary.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

a.milkovsky’s picture

Add an alt text field to the media entity reference to store the alt text for a media item if needed.

Do you mean to extend the entity reference field a few more sub-fields? As a workaround, we can use an additional entity for that.

seanB’s picture

Do you mean to extend the entity reference field a few more sub-fields? As a workaround, we can use an additional entity for that.

We have not yet decided the best way to do this. A couple of ways I see currently:

  1. Change the existing entity reference field to:
    • A. Just add a field for Media fields that need the alt texts, and don't care about other use cases
    • B. A field that allows data to be added to the references (maybe serialised or something). Files might need a description, images need an alt text, and who knows what kind of use cases contrib can think of :)
  2. Build a new field type on top of entity reference with:
    • A. Just a added field for Media fields that need the alt texts, and don't core about other use cases
    • B. A field for serialized data like described in 1., but now in a new field type to not mess with existing entity reference fields.
  3. Create a new entity type to store arbitrary values with an entity reference field. Not sure how this will work with revisions and translations though?
  4. Other...?

All of these approaches are not perfect, so we have to choose which one is the lesser of all the evils. I would love to get some input on why we should definitely NOT choose the approaches above (I think that is the quickest way to find out which is the lesser evil). There must also be other ways, so I would love to get more input on that as well.

a.milkovsky’s picture

Some small feedbaack from me:
We should think about use-cases where this solution will be needed. Currently I am thinking about:

  • Case A: Images, which are referenced in media fields.
  • Case B: Images, which are added to CKeditor via the Embed module button.
  • Case C: It should also be so possible to easily override images, which are uploaded to Media library via Entity Browser (Maybe via Inline Entity Form).



1:As you said, we should not "not mess with existing entity reference fields.". That's why I like 2 and 3 more.

2: "A field that allows data to be added to the references" makes sense.
FYI there is a similar module for Drupal 7 Relation, it might contain interesting solutions.
But the existing Entity reference field has an own small eco-system already because it is supported by many other modules (E.g. Inline Entity Form).
If we build a new field type people will have to build some custom integrations for the new field type as well.

3: New entity sounds more interesting, seamless and native to Drupal. In this case we can easier integrate it with existing media modules like Embed, Entity Browser. I think revisions should, we can have a look what Paragraphs are doing, they are using Entity Reference Revisions field.

weseze’s picture

I'm curious as to how this problem can be "solved" currently?
Since Drupal 8.7 now ships with media in core and a fantastic widget "Media library" I assumed it would be possible to use this. But not being able to set an alt tag on an image specific as to where it is used, is just a showstopper for us. None of my coworkers working with the customer, with the content and optimising for SEO find this acceptable. For them this solution is "broken".

We are currently still using file_entity module and plain image fields to set these things up.

How can we get around this as of now (Drupal 8.7 with media library) Which contribs do we need, which field setups for images?
I would imagine that at the technical level we would need an entity_reference field that can store additional meta data? Does such a thing already exist? Would this be the path forward for Drupal core? Or should it remain in contrib?

Wim Leers’s picture

This not yet being solved for Media entity reference fields makes it much harder for the WYSIWYG equivalent over at #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` to solve this.

I don't yet understand why #2834729: [META] Roadmap to stabilize Media Library lists per-use metadata as a MUST-HAVE for WYSIWYG uses and a SHOULD-HAVE for entity reference uses.

andrewmacpherson’s picture

Priority: Normal » Major
Issue tags: +atag

Re #7 - We discussed must-have vs. should-have for the per-use metadata in comments 8-10 at #2994702-8: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`. The points there apply equally to this one. I'm happy to treat both issues as a11y should-haves for the media library stabilization roadmap.

Setting priority as major, to reflect the awesome-sauce impact it will for authors and our ATAG coverage.

phenaproxima’s picture

This was discussed at Dev Days Cluj between @alexpott, @amateescu, @seanB, @Wim Leers, and myself. I'm pleased to report that we hatched a plan to address this.

We are going to create a new field type, called 'media', which will be similar to the 'file' and 'image' field types in that it will extend the base entity reference field type with an additional property. That additional property will be of the Map data type, to store a simple set of key/value pairs: the name of the metadata property to override, such as 'alt', and the override value itself.

This means that there will be an update path which will convert all existing media reference fields to this new type. According to @amateescu, this is feasible, although it does involve changing the type of an existing field, which is not quite ultra-kosher...but doable, and the best option we have at our disposal to accomplish this task. The dangers of the update path are significantly mitigated by the fact that we will be adding a database column to existing fields, but not migrating or changing existing data in any way. The only thing that is a bit questionable is the fact that we are changing a field type, but @amateescu does not believe this will pose a problem. In our discussion, @alexpott agreed with that assessment.

So much, then, for the data layer.

In considering the UI of this, @seanB and I drew up some sketches on paper -- of which we will soon post pictures -- with @ckrina (UX maintainer) and @Gábor Hojtsy (product manager) present to validate our ideas. We also presented the sketches to the UX team (including @webchick and Gábor) on the weekly hangout to get wider feedback. Generally, it was very well received and we reached a consensus (!) on most of the proposed UX.

The overall idea is that, when you have added media items to the field via the media library widget, you will see the grid of thumbnails of the chosen items, as you do now -- and each one will have two small icons attached: one to remove the item from the set (as we have now), and one which will open a modal that allows you to override particular metadata values. Exactly which values can be overridden is something that will be left up to the media source plugin. (For example, the Image plugin will allow overriding the alt and title text; the Remote Video plugin, by contrast, may allow one to override the default thumbnail.)

Each overriddeable value will follow what we termed the "machine name pattern", in that the default value will be displayed as a label, with a link or button that says "Edit" (or "Override" -- the jargon is not fleshed out yet) next to it. When you click that button, the value will be instantly displayed in a form field instead, allowing you to change it, and the Edit/Override button will disappear and be replaced by a "Restore default" (or "Revert" or something...again, don't worry about terminology yet) button that, if clicked, will disappear the form field and display the original value as text.

So, this means that:

  • When you choose an item from the media library, it is using its default metadata values and does not need further action from the user.
  • You need to click an additional button to get into the override stuff. This is an explicit choice we made, in order to reduce user confusion: it is advanced functionality and therefore it should be hidden behind a click.
  • In order to prevent further confusion/potential data loss, and to make it clear what is overridden and what's not, each piece of overriddeable metadata requires an explicit action from the user in order to override it.

The WYSIWYG variant of the media library will follow the same overall workflow. First, you choose an item from the media library, and it is instantly embedded in the editor with default metadata. There is an additional button to click if you want to override that metadata, and it calls up a similar modal window that works in just the same way.

seanB’s picture

Here is a link to the usability meeting where the designs were discussed: https://www.youtube.com/watch?v=MwNa2nNCQr0

Wim Leers’s picture

Can we still add photos or scans to this issue of the paper sketches? 😊

seanB’s picture

I took some pictures as promised :)

ER widget proposal:
ER widget sketch.

WYSIWYG proposal:
WYSIWYG media with button sketch.

Override metadata modal proposal:
Override metadata modal sketch.

Also made a proposal on what the image with the buttons could look like:
Edit and delete example

Wim Leers’s picture

The screenshot of a hacky mock-up at the bottom of #12 is becoming reality: see #3039829-35: Remove link to media item from media library view.!

chr.fritsch’s picture

I think #2568289: Not possible to reuse field formatters between entity_reference, file, and image fields is highly related if not a blocker for this. Because when we introduce a new field type "media", we are not able to use the "Rendered entity" formatter anymore.

chr.fritsch’s picture

FileSize
8.95 KB

Posting my first WIP patch. This patch adds a new field type "media" and an update path to switch to that type for existing media entity reference fields.

chr.fritsch’s picture

Ok, #2568289: Not possible to reuse field formatters between entity_reference, file, and image fields is not a blocker for us. I thought it's about making entity_reference formatters dynamically available on entity_reference sub field types but we want to stick with the opt-in behavior. So we can make the entity reference formatters available for the media field type with hook_field_formatter_info_alter().

chr.fritsch’s picture

Status: Active » Needs review
FileSize
28 KB

A lot more changes. The good thing about the new field type is, we can remove some hacky code :)

So let's see how many tests will fail.

johnwebdev’s picture

Haven't really looked at the patch yet; though I'm wondering about:

+++ b/core/modules/media/src/Plugin/Field/FieldType/MediaItem.php
@@ -0,0 +1,86 @@
+          $entity->set($key, $value);

do entities have any read-only mode to ensure that the entity is not saved with the overrides?

Status: Needs review » Needs work

The last submitted patch, 17: 3023807-17.patch, failed testing. View results

phenaproxima’s picture

I discussed this on Wednesday with @daniel.bosen, @chr.fritsch, @seanB, and @amateescu.

We were leaning towards making this into a meta-issue, since there are at least two very clear things we need to do in order to implement this feature.

First, we need to create the new "media" field type and its update path, plus API additions in the media system to allow source plugins and media types to define which pieces of metadata are overridable at the per-reference level. To do this, we're thinking we should leverage the Typed Data system, since this would allow us to integrate cleanly with the entity field system and JSON:API. Media sources will get a new method allowing them to expose their overridable metadata (and/or certain configurable fields on the media type) as typed data definitions, kinda like so:

<?php

namespace Drupal\media;

use Drupal\media\MediaTypeInterface;

interface MediaSourceInterface {

  /**
   * @return \Drupal\Core\TypedData\DataDefinitionInterface[]
   */
  public function getOverridablePropertyDefinitions(MediaTypeInterface $media_type);

}

The "media" field type plugin will be responsible for using this information to actually apply the overridden values to referenced media items.

We're hoping that using Typed Data will smooth out the integration with JSON:API, but we're still not entirely sure how we want to expose the overridden data to it. This will also give contrib and custom code an extension point, and hopefully make it easier to create/modify the form where overridden values are set.

After that, we need to implement the actual UI outlined in #12 in the existing media_library field widget.

Wim Leers’s picture

I like #20 :)

MediaSourceInterface::getOverridablePropertyDefinitions()

So #17 includes a overwritten_property_map field. I think the intent here is that getOverridablePropertyDefinitions() specifies which keys are allowed to exist (and their corresponding values), for the media source associated with this media field. Is that correct?

We're hoping that using Typed Data will smooth out the integration with JSON:API, but we're still not entirely sure how we want to expose the overridden data to it. This will also give contrib and custom code an extension point, and hopefully make it easier to create/modify the form where overridden values are set.

JSON:API (or REST or GraphQL) will not be calling MediaSourceInterface::getOverridablePropertyDefinitions(), they will simply normalize what is stored in overwritten_property_map for reads and a corresponding validation constraint will ensure that during writes only properties returned by that method will be allowed to be written. Meaning that none of them need to change a single line of code as long as this is implemented in MediaItem and includes a validation constraint.

Ghost of Drupal Past’s picture

Apparently a lot more is possible here than just overriding the alt text.

Ghost of Drupal Past’s picture

Title: Override alt text per reference for image media items » Override media fields from the reference field
Issue summary: View changes
Issue tags: -Needs issue summary update

Let me take a stab at it. Also, do we want to do field properties instead of whole fields? Right now:

        foreach ($this->values['overwritten_property_map'] as $key => $value) {
          $entity->set($key, $value);

and that means you can only override the entire image field including target_id et al and overriding just alt is not possible.

Wim Leers’s picture

Thanks for updating the title and issue summary, @Charlie ChX Negyesi! The issue summary had indeed not yet been updated for #9.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
40.08 KB

Ok, here is what I have so far.

I introduced a new "read-only" wrapper entity around the media entity; the new media field type returns that. It passes most of the method calls to the original media entity but prevents saving.

I tried to implement @phenaproxima's proposal about the MediaSourceInterface::getOverridablePropertyDefinitions(), but I really don't know how that could work.
So what I did, is the following. The MediaType can now return an array of allowed property overwrites in MediaType::getOverridableProperties(). Additionally the new field type got a constraint validator OverwrittenMetadata. This validator checks which properties are allowed to overwrite and then checks the overwritten values against the typed data of the original fields.

Note, the MediaReference widget can be ignored. It is just to set some values and make testing, especially of the validator more comfortable. The media library will take over that in the future.

Status: Needs review » Needs work

The last submitted patch, 25: 3023807-25.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
60.77 KB
25 KB

So I had to implement the MediaInterface for the ReadOnlyMedia entity because other parts of Drupal expect that. That meant, that I had to implement every single method of that interface and it's parent interfaces. not nice, but I don't see another option.

Status: Needs review » Needs work

The last submitted patch, 27: 3023807-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

  1. +++ b/core/modules/media/src/Entity/MediaType.php
    --- /dev/null
    +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    
    +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    @@ -0,0 +1,847 @@
    
    @@ -0,0 +1,847 @@
    +<?php
    +
    +namespace Drupal\media\Entity;
    

    Does this need to be be in the reserved entity namespace. It doesn't have an entity annotation.

  2. +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    @@ -0,0 +1,847 @@
    +    throw new \Exception("It's not allowed to save a media item fetched from a reference field");
    

    Should be a RuntimeException.

  3. +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    @@ -0,0 +1,847 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function __set($name, $value) {
    +    $this->overwrittenPropertyMap[$name] = $value;
    +  }
    

    I'm not sure this should work. I think the overwritten properties should only be settable on construction.

  4. +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    @@ -0,0 +1,847 @@
    +    if (!empty($this->overwrittenPropertyMap[$name])) {
    

    Should we ensure that we only override existing properties?

  5. +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    @@ -0,0 +1,847 @@
    +      $original = $this->media->get($name);
    +      if (is_array($this->overwrittenPropertyMap[$name])) {
    +        $values = $original->getValue();
    +        $original->setValue(array_merge($values[0], $this->overwrittenPropertyMap[$name]));
    +      }
    +      else {
    +        $original->setValue($this->overwrittenPropertyMap[$name]);
    +      }
    +      return $original;
    

    So this code result in the original actually changing - this seems a bit dangerous.

  6. +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    @@ -0,0 +1,847 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function load($id) {
    +    return Media::load($id);
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function loadMultiple(array $ids = NULL) {
    +    return Media::loadMultiple($ids);
    +  }
    

    I'm not sure these should work.

  7. +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    @@ -0,0 +1,847 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(array $values = []) {
    +    return Media::create($values);
    +  }
    

    This creates a Media item... seems interesting behaviour.

  8. +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    @@ -0,0 +1,847 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setOriginalId($id) {
    +    return call_user_func_array([$this->media, __FUNCTION__], func_get_args());
    +  }
    

    Having setters that work on an object called ReadOnly seems odd.

  9. +++ b/core/modules/media/src/Entity/ReadOnlyMedia.php
    @@ -0,0 +1,847 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function toArray() {
    +    return call_user_func_array([$this->media, __FUNCTION__], func_get_args());
    +  }
    

    I think it would be better if this was return $this->media->toArray(); instead. Same in all the other places.

  10. +++ b/core/modules/media/tests/src/Kernel/MediaItemTest.php
    @@ -0,0 +1,79 @@
    +    $this->assertEquals('Mr. Jones', $entity->field_media->entity->getName()->value);
    +    $this->assertEquals('', $entity->field_media->entity->field_media_file->entity->description);
    +
    +    $entity->field_media->first()->overwritten_property_map = [
    +      'name' => 'Overwritten name',
    +      'field_media_file' => ['description' => 'Nice description!'],
    +    ];
    +    $entity->save();
    +
    +    $this->assertEquals('Overwritten name', $entity->field_media->entity->getName()->value);
    +    $this->assertEquals('Nice description!', $entity->field_media->entity->field_media_file->description);
    

    It would be good to assert that the file itself is not overridden.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
59.53 KB

Thx @alexpott for a first review. I adjusted most of our remarks.
Regarding #29.4:
I think we already check that in the OverwrittenMetadataConstraintValidator. Or do you think about something else?

chr.fritsch’s picture

FileSize
28.13 KB

Here is the interdiff..

Status: Needs review » Needs work

The last submitted patch, 30: 3023807-30.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
883 bytes
60.39 KB
alexpott’s picture

  1. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,840 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function enforceIsNew($value = TRUE) {
    +    return $this->media->enforceIsNew($value);
    +  }
    

    This should be prevented on read only entities.

  2. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,840 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function preCreate(EntityStorageInterface $storage, array &$values) {
    +    return Media::preCreate($storage, $values);
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function postCreate(EntityStorageInterface $storage) {
    +    return call_user_func_array([$this->media, __FUNCTION__], func_get_args());
    +  }
    

    I think this should throw an exception too. Not sure why we need them. Tricky.

  3. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,840 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function postLoad(EntityStorageInterface $storage, array &$entities) {
    +    return Media::postLoad($storage, $entities);
    +  }
    

    If you can't load these - I think is not necessary either.

  4. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,840 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function createDuplicate() {
    +    return $this->media->createDuplicate();
    +  }
    

    This doesn't create a duplicate. Probably should be prevented. Not 100% sure.

  5. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,840 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function toArray() {
    +    return $this->media->toArray();
    +  }
    

    This one is interesting. Should this return the overridden values?

  6. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,840 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function onChange($field_name) {
    +    return $this->media->onChange($field_name);
    +  }
    

    Nothing should change right?

  7. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,840 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function addCacheContexts(array $cache_contexts) {
    +    return $this->media->addCacheContexts($cache_contexts);
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function addCacheTags(array $cache_tags) {
    +    return $this->media->addCacheTags($cache_tags);
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function mergeCacheMaxAge($max_age) {
    +    return $this->media->mergeCacheMaxAge($max_age);
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function addCacheableDependency($other_object) {
    +    return $this->media->addCacheableDependency($other_object);
    +  }
    

    Not sure these should be allowed on a read only entity. Again not 100% sure at all.

  8. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,840 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function updateLoadedRevisionId() {
    +    return $this->media->updateLoadedRevisionId();
    +  }
    

    Feels like this should be prevented.

  9. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,840 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function preSaveRevision(EntityStorageInterface $storage, \stdClass $record) {
    +    return $this->media->preSaveRevision($storage, $record);
    +  }
    

    As a preSave hook - probably should throw an exception.

  10. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,840 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function addTranslation($langcode, array $values = []) {
    +    return $this->media->addTranslation($langcode, $values);
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function removeTranslation($langcode) {
    +    return $this->media->removeTranslation($langcode);
    +  }
    

    This is not part of read-only ness. However thinking about this does make me wonder about translations and how we plan to handle that. We definitely need multi-lingual test coverage.

chr.fritsch’s picture

So again preventing more stuff and I also added a test for the validator and for translation support.

amateescu’s picture

Status: Needs review » Needs work

The patch looks really good to me, I like that we're able to simplify a lot of code because we don't need to check for the entity reference field type anymore :)

Here's my initial review:

  1. +++ b/core/modules/media/media.install
    @@ -205,3 +207,64 @@ function media_update_8700() {
    + * Change media entity reference fields to field type media.
    

    How about "Change entity reference fields targeting media to use the 'media' field type."?

  2. +++ b/core/modules/media/media.install
    @@ -205,3 +207,64 @@ function media_update_8700() {
    +function media_update_8800() {
    

    This update function only covers configurable fields, we need to take care of base fields as well, which might be stored in the shared tables or in dedicated ones if their cardinality is higher than 1.

    Also, the database schema updates rely on the table structure defined by core's default SQL content entity storage, so we need to bail out if the entity type that contain these entity reference fields does not use SqlContentEntityStorage.

  3. +++ b/core/modules/media/src/Plugin/Field/FieldType/MediaItem.php
    @@ -0,0 +1,84 @@
    + *   default_widget = "entity_reference_autocomplete",
    
    +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaReference.php
    @@ -0,0 +1,40 @@
    + *   id = "media_reference_autocomplete",
    

    The default widget for the new media field type is entity_reference_autocomplete but we are also adding a new media-specific widget below. Why don't we use that as the default widget?

  4. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaReference.php
    @@ -0,0 +1,40 @@
    +      $values[$key]['overwritten_property_map'] = ['name' => 'Overwritten name', 'field_media_image' => ['alt' => 'Overwritten alt tag']];
    

    I don't think we should hardcode a configurable field shipped by the Standard profile in here.

    Also, I'm not sure what's the purpose of this code and why we need a new widget just for it...

  5. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,838 @@
    +class ReadOnlyMedia implements ReadOnlyMediaInterface {
    

    This should be marked as @internal in case we decide to generalize the concept of read only entities.

  6. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,838 @@
    +        $values = array_merge($this->media->get($name)->getValue()[0] ?? [], $this->overwrittenPropertyMap[$name]);
    

    Is there a reason we're restricting the overwritten properties to just a single (first) value of a field type?

  7. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,838 @@
    +    throw new \RuntimeException("Read-only media entities should not be used for migrations.");
    

    Why do we mention migrations here? Shouldn't this use the same "It's not allowed ..." message as above?

  8. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,838 @@
    +  public function getTypedData() {
    +    return $this->media->getTypedData();
    +  }
    

    The typed data object returned by this method won't contain the overrides, which means it will be inconsistent with the actual values of the read-only media and it's very likely to cause subtle and hard to track bugs down the line..

  9. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,838 @@
    +  public function toArray() {
    +    return $this->media->toArray();
    +  }
    

    Same here :/

  10. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,838 @@
    +  public function onChange($field_name) {}
    

    Shouldn't we throw an exception here?

  11. +++ b/core/modules/media/src/ReadOnlyMediaInterface.php
    @@ -0,0 +1,14 @@
    +interface ReadOnlyMediaInterface extends MediaInterface ,\IteratorAggregate, TranslationStatusInterface {
    

    Is this empty interface really needed?

  12. +++ b/core/modules/media/tests/src/Kernel/MediaItemTest.php
    @@ -0,0 +1,132 @@
    +    // Create a random file that should fail.
    

    It doesn't seem like it fails :)

  13. +++ b/core/modules/media/tests/src/Kernel/MediaItemTest.php
    @@ -0,0 +1,132 @@
    +    $entity->field_media->overwritten_property_map = [
    +      'name' => 'Overwritten name',
    +      'field_media_file' => ['description' => 'Nice description!'],
    +    ];
    +    $entity->save();
    +
    +    $this->assertEquals('Overwritten name', $entity->field_media->entity->getName());
    +    $this->assertEquals('Nice description!', $entity->field_media->entity->field_media_file->description);
    +    $this->assertEquals(1, $entity->field_media->entity->field_media_file->entity->id());
    +    $this->assertEquals('test.patch', $entity->field_media->entity->field_media_file->entity->getFilename());
    

    Can we also add these assertions before saving the entity? We need to ensure that changing overwritten_property_map has an "instant" effect on the entity property of the media reference field.

  14. +++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
    @@ -92,11 +92,8 @@ public function checkAccess(MediaLibraryState $state, AccountInterface $account)
    +      throw new \LogicException('Expected the media library to be opened by an media field.');
    

    "by an media" -> "by a media" :)

geek-merlin’s picture

This may have synergies with the preview-in-editor use case.

bnjmnm’s picture

Rebase of #35. Keeping this at Needs Work since the feedback in #36 still needs to be addressed.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
15.37 KB
79.96 KB

Addressing #36:

1. Done
2. Do we have any example for that? Base fields are defined in code. How to change that?
3. + 4.. That widget was only for testing purposes. It will not be in the final patch.
5. Done
6. You are right. Fixed that and added a test for that.
7. Done
8. Fixed that and added a test for that.
9. Fixed that and added a test for that.
10. Done
11. When I added the interfaces directly to the ReadOnlyMedia class I got Class Drupal\media\ReadOnlyMedia must implement interface Traversable as part of either Iterator or IteratorAggregate. With the ReadOnlyMediaInterface it's fine.
12. Copy/paste leftover...
13. Done
14. Done

Also fixed the ContentModerationTest test.

Status: Needs review » Needs work

The last submitted patch, 40: 3023807-40.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

alexpott’s picture

  1. +++ b/core/modules/media/src/Entity/MediaType.php
    @@ -235,4 +235,11 @@ public function setFieldMap(array $map) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getOverridableProperties() {
    +    return array_merge($this->getSource()->getOverridableProperties(), ['name' => []]);
    +  }
    +
    
    +++ b/core/modules/media/src/MediaTypeInterface.php
    @@ -98,4 +98,12 @@ public function getFieldMap();
    +  /**
    +   * Returns an array of allowed property overrides.
    +   *
    +   * @return array
    +   *   Array with field name as key and properties as values.
    +   */
    +  public function getOverridableProperties();
    

    We're adding a method to an interface to a stable module. I think this is okay in a minor under the 1 to 1 rule as this is implemented only by \Drupal\media\Entity\MediaType and that's really the only implementation we expect.

  2. +++ b/core/modules/media/src/MediaSourceBase.php
    @@ -356,4 +356,11 @@ public function prepareFormDisplay(MediaTypeInterface $type, EntityFormDisplayIn
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getOverridableProperties() {
    +    return [];
    +  }
    
    +++ b/core/modules/media/src/MediaSourceInterface.php
    @@ -193,4 +193,12 @@ public function prepareFormDisplay(MediaTypeInterface $type, EntityFormDisplayIn
    +  /**
    +   * Returns an array of allowed property overrides.
    +   *
    +   * @return array
    +   *   Array with field name as key and properties as values.
    +   */
    +  public function getOverridableProperties();
    

    We're adding a method to an interface to a stable module. I think this is okay in a minor under the 1 to 1 rule as this is implemented by \Drupal\media\MediaSourceBase and that's really what we supporting extending.

  3. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaReference.php
    @@ -0,0 +1,40 @@
    +      $values[$key]['overwritten_property_map'] = ['name' => 'Overwritten name', 'field_media_image' => ['alt' => 'Overwritten alt tag']];
    

    I'm not sure why this is here. Needs a comment. It seems to be assuming there will be an alt tag.

  4. +++ b/core/modules/media/src/ReadOnlyMediaInterface.php
    @@ -0,0 +1,14 @@
    +/**
    + * @file
    + * Contains
    + */
    

    This doesn't need to be here.

  5. +++ b/core/modules/media/src/ReadOnlyMediaInterface.php
    @@ -0,0 +1,14 @@
    +namespace Drupal\media;
    +
    +
    +use Drupal\Core\TypedData\TranslationStatusInterface;
    

    Too much space

  6. +++ b/core/modules/media/src/ReadOnlyMediaInterface.php
    @@ -0,0 +1,14 @@
    +
    +interface ReadOnlyMediaInterface extends MediaInterface ,\IteratorAggregate, TranslationStatusInterface {
    

    Let's add some docs also formatting of the commas.

  7. +++ b/core/modules/media/tests/src/Kernel/ReadOnlyMediaTest.php
    @@ -0,0 +1,116 @@
    +        ['name' => 'Dr. Dolittle'],
    ...
    +          'name' => [['value' => 'Dr. Dolittle']],
    ...
    +          'name' => 'Dr. Dolittle',
    ...
    +          'name' => [['value' => 'Dr. Dolittle']],
    ...
    +          'name' => 'Dr. Dolittle',
    ...
    +          'name' => [['value' => 'Dr. Dolittle']],
    

    Doolittle (then PHPStorm won't complain about spelling :D)

  8. FILE: ...drupal8alt.dev/core/modules/media/src/ReadOnlyMediaInterface.php
    ----------------------------------------------------------------------
    FOUND 6 ERRORS AFFECTING 4 LINES
    ----------------------------------------------------------------------
    1 | ERROR | [x] The PHP open tag must be followed by exactly one
    | | blank line
    2 | ERROR | [x] Namespaced classes, interfaces and traits should
    | | not begin with a file doc comment
    7 | ERROR | [x] There must be one blank line after the namespace
    | | declaration
    12 | ERROR | [x] Expected 0 spaces between "MediaInterface" and
    | | comma; 1 found
    12 | ERROR | [x] Expected one space after the comma, 0 found
    12 | ERROR | [x] Expected 1 space before "IteratorAggregate"; 0
    | | found
    ----------------------------------------------------------------------
    PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    ----------------------------------------------------------------------
alexpott’s picture

Good progress here. However there seem to be some rough edges.

  1. Apply patch
  2. Install standard
  3. Enable media, media library
  4. Remove the image field from the article content type
  5. Add a media field to article and allow it to use unlimited images
  6. Create an article and add a media image
  7. When you view the node after saving I get an error:

Warning: Invalid argument supplied for foreach() in Drupal\media\Plugin\Validation\Constraint\OverwrittenMetadataConstraintValidator->validate() (line 53 of core/modules/media/src/Plugin/Validation/Constraint/OverwrittenMetadataConstraintValidator.php).

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
79.81 KB
8.33 KB

Thanks again for reviewing, @alexpott!

I fixed all the comments from #43 and #44.

alexpott’s picture

Do we need a new testcase for #44 or was that covered by the fails in #40?

Status: Needs review » Needs work

The last submitted patch, 45: 3023807-45.patch, failed testing. View results

Wim Leers’s picture

Glad to see this making progress! 👍

oknate’s picture

Status: Needs work » Needs review
FileSize
79.78 KB

Reroll. The MediaLibraryTest file wasn't applying. All the rest works, even with #3082690: Mark Media Library as a stable core module applied.

oknate’s picture

All through the patch I'm seeing the word 'overwritten' while the issue title says 'Override'. When talking about field values, I would think overwrite is more for writing to the database and override is more for overriding it in other contexts. I think we might want to change the wording of this in variables, classnames, etc. added by this patch to be "OverriddenMetadata", "overridden_property_map", "OverriddenMetadataConstraint", allowed to be overridden", etc.

I suggest we replace 'overwrite' with 'override' and 'overwritten' with 'overridden'.

Update:

Now that I think about it, it is writing it. It's writing it at the field level. So it's both overridding the original written data (on the original media field) and writing it at the reference field level. So in this way, I'm contradicting myself, and I think now 'Overwrite' and "overwritten' are appropriate.

Status: Needs review » Needs work

The last submitted patch, 49: 3023807-49.patch, failed testing. View results

oknate’s picture

Fixing tests. For this one, I'm not sure why but it was evaluating true here: if (empty($value->overwritten_property_map)).

I fixed it this way, although, it'd probably be better to fix the Mock object.

diff --git a/core/modules/media/src/Plugin/Validation/Constraint/OverwrittenMetadataConstraintValidator.php b/core/modules/media/src/Plugin/Validation/Constraint/OverwrittenMetadataConstraintValidator.php
index 182fb5a210..9339f7b9e0 100644
--- a/core/modules/media/src/Plugin/Validation/Constraint/OverwrittenMetadataConstraintValidator.php
+++ b/core/modules/media/src/Plugin/Validation/Constraint/OverwrittenMetadataConstraintValidator.php
@@ -50,10 +50,11 @@ public function validate($value, Constraint $constraint) {
     $media = $target->getValue();
 
     $overridable_properties = $media->bundle->entity->getOverridableProperties();
-    if (empty($value->overwritten_property_map)) {
+    $overrides = $value->overwritten_property_map;
+    if (empty($overrides)) {
       return;
     }
-    foreach ($value->overwritten_property_map as $field => $properties) {
+    foreach ($overrides as $field => $properties) {
       if (!array_key_exists($field, $overridable_properties)) {
         $this->context->addViolation($constraint->notAllowedFieldToOverwrite, ['%field_name' => $field]);
         continue;
oknate’s picture

What are the next steps here? Do we need to add a UI to update this within this issue? Or should that be in another issue?

chr.fritsch’s picture

Re #53: I wouldn't add the UI in this issue. It's already quite huge. Better do it separately.

Belshi’s picture

Really glad to see progress here, but since my lack of skill doing it by myself I have to ask: Is there any chance to get this working in current Drupal stable release?

phenaproxima’s picture

Unfortunately not, due to the deadlines in core development. Once a beta has been released, there is almost zero chance of getting new features (and this counts as a new feature) into the forthcoming stable release. Since the next feature release is (as far as I know) 9.1.x, this feature will be targeted for 9.1.0, I think.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

johnwebdev’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
79.05 KB

Re-roll patch created, Please review

Status: Needs review » Needs work

The last submitted patch, 59: 3023807_59.patch, failed testing. View results

Berdir’s picture

  1. +++ b/core/modules/media/media.install
    @@ -179,3 +181,64 @@ function media_requirements($phase) {
    +/**
    + * Change entity reference fields targeting media to use the 'media' field type."
    + */
    +function media_update_8800() {
    +  $configFactory = \Drupal::configFactory();
    +
    +  $changed_fields = [];
    +  foreach ($configFactory->listAll('field.storage.') as $field_storage) {
    +    $field_storage = $configFactory->getEditable($field_storage);
    +
    +    if ($field_storage->get('type') !== 'entity_reference' || $field_storage->get('settings.target_type') !== 'media') {
    +      continue;
    +    }
    +    $field_name = $field_storage->get('field_name');
    +    $entity_type = $field_storage->get('entity_type');
    +
    +    $changed_fields[] = sprintf('%s.%s', $entity_type, $field_name);
    +
    

    drive-by-review, without reading up on the issue in depth. And sorry for that, but this is super complex with a lot of cases that haven't really been throught through I fear.

    This looks super, super scary to me. We never had a field schema change API because it is very complex, with a change like this, any claim of supporting pluggable entity storage is out of the window, we should at least make sure we only do it for the default storage.

    Also, this is going be a breaking change for custom formatters, widgets and custom code that checks for the entity reference field type.

    It would also not work on non-configurable fields.

    And because that's not enough fun yet, here are some other fun things that this might break:

    * multilingual: usually entity reference fields are untranslatable, but if you would want to translate alt/title/description or whatever. We could use the column grouping stuff that we have on image field.
    * normalization/API-first, we'd need to make sure to have normalizers and stuff in place to support that new field type
    * the field type only seems to override __get(), that wouldn't work for more generic code that works through properties, would need to happen lower on the property level
    * Render caching. The overridden entity would end up being cached the same way for multiple usages. we'd need to disable that/include the information from the parent.
    * This seems to include several changes that would break existing regular entity reference fields pointing to media, like no longer supporting that field type for formatters and so on. Also removing the common target breaks tests as visible here with the necessary changes.

    To be honest, I'd strongly suggest to not enforce this change and make this available as a separate, optional field type. Possibly experimental, considering all the complexity mentioned above and that's just what I could think of right now. If we want we could offer the conversion as a manual operation that people can trigger, maybe even in a contrib module.

    In fact, the whole thing seems like something that should maybe mature in contrib first? I get that it is a common use case but not everything can be in core. But it's easier to say in contrib that this first version doesn't support multilingual/normalization or whatever yet.

  2. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,857 @@
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function save() {
    +    throw new \RuntimeException("It's not allowed to save a media item fetched from a reference field");
    +  }
    

    you would be able to bypass this easily with $storage->save($media)

stomusic’s picture

FileSize
79.12 KB

Fin for fields without revision table

phenaproxima’s picture

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

dejan0’s picture

We needed the solution for `entity_reference_media` and the Caption field so we created a small contrib module.
https://www.drupal.org/project/entity_reference_media - just in case someone needs it

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
79.22 KB

Fixed self-deprecation notices.
1) Declaring ::setUp without a void return typehint
2) $modules property must be declared protected

Status: Needs review » Needs work

The last submitted patch, 67: 3023807_67.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.16 KB
79.96 KB

Fixing the test and coding standards issues.

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/media/media.install
    @@ -179,3 +181,64 @@ function media_requirements($phase) {
    +/**
    + * Change entity reference fields targeting media to use the 'media' field type.
    + */
    +function media_update_8800() {
    

    We need to change this number to be 9200.

  2. +++ b/core/modules/media/src/Plugin/Field/FieldType/MediaItem.php
    @@ -0,0 +1,94 @@
    +class MediaItem extends EntityReferenceItem {
    

    We need to account for a tricky situation. We need to cope with what happens when a module is providing a media field. I think we should implement a field storage and field config pre save hook to fix the data. They should also emit deprecation errors.

    We could change the update functions to only detect what fields need fixing and trigger a save so we don't have to duplicate code.

  3. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,857 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function &__get($name) {
    

    This is not inheriting any documentation.

    Should be Implements the magic method for getting object properties.

  4. +++ b/core/modules/media/src/ReadOnlyMedia.php
    @@ -0,0 +1,857 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function urlInfo($rel = 'canonical', array $options = []) {
    +    return $this->media->urlInfo($rel, $options);
    +  }
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function url($rel = 'canonical', $options = []) {
    +    return $this->media->url($rel, $options);
    +  }
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function link($text = NULL, $rel = 'canonical', array $options = []) {
    +    return $this->media->link($text, $rel, $options);
    +  }
    

    These methods no longer exist in D9 and can be removed.

alexpott’s picture

We also need an update path test for the update hook.

ankithashetty’s picture

Status: Needs work » Needs review
FileSize
79.53 KB
1.53 KB

Updated the patch in #69 addressing #70.1, #70.3 and #70.4. Kindly review.

Thank you!

phenaproxima’s picture

Just got off the phone with @chr.fritsch and @alexpott to discuss this dragon of an issue.

@Berdir's points in #61 are valid, and he's right that this poses some very scary technical challenges from a runtime, operational standpoint. To quote (emphasis mine):

And because that's not enough fun yet, here are some other fun things that this might break:

* multilingual: usually entity reference fields are untranslatable, but if you would want to translate alt/title/description or whatever. We could use the column grouping stuff that we have on image field.
* normalization/API-first, we'd need to make sure to have normalizers and stuff in place to support that new field type
* the field type only seems to override __get(), that wouldn't work for more generic code that works through properties, would need to happen lower on the property level
* Render caching. The overridden entity would end up being cached the same way for multiple usages. we'd need to disable that/include the information from the parent.
* This seems to include several changes that would break existing regular entity reference fields pointing to media, like no longer supporting that field type for formatters and so on. Also removing the common target breaks tests as visible here with the necessary changes.

To be honest, I'd strongly suggest to not enforce this change and make this available as a separate, optional field type. Possibly experimental, considering all the complexity mentioned above and that's just what I could think of right now. If we want we could offer the conversion as a manual operation that people can trigger, maybe even in a contrib module.

Personally, I'm far less worried about the update path than I am about these questions. The update path would probably work well enough, considering how much badass brain power we could throw it it. But we don't know how well this would work day-to-day for authors and editors, and what kind of technical problems (up to and including data loss) might result from those unknowns. So this still feels extremely risky.

Nonetheless, we need this feature. It's important not just for accessibility, but also for content strategy. So I see two possible paths right now, both of which are longer-term projects:

  1. We take this to contrib, specifically the Entity Reference Override module, and prove it there. When it's mature and (hopefully) widely used, and hard problems have been found and fixed, we can maybe port parts of that to core.
  2. We implement this in a new core experimental module, specifically for use with the media system, and iterate on that for a while. We drop the update path, and allow sites to opt into this functionality on a field-by-field basis.

I think I'm partial to the first option, mostly because contrib is not bound by the core processes and gates and can therefore iterate faster. But I'm certainly amenable to the second option too, precisely because of core development's high quality standards.

The one thing I think we shouldn't do is put this functionality directly into stable core modules and enforce it with an update path.

Thoughts?

John Pitcairn’s picture

My 2c worth - yes, contrib.

marcoscano’s picture

+1 for contrib, opt-in.

johnwebdev’s picture

I also think that moving the effort to contrib would make sense, Berdir's point are very compelling and highlights some complexity that needs to be addressed. I'm wondering if by doing that it also makes sense to still target the use case of media rather than solving it generically? Faster progression perhaps.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

chr.fritsch’s picture

The last couple of weeks, the Thunder team (mainly @alexpott and me) were working on a solution for this in contrib. So we came up with https://www.drupal.org/project/media_library_media_modify.

The module provides a new field type, that extends entity_reference and an extended media library widget. The modifications that can be made to the referenced media items, are stored in the field, next to the target_id. The referenced media items will never be changed. All the modifications only exist in the context of the referencing entity.
The fields, that are allowed to be edited, can be defined with a form mode. So we also have the flexibility to define in which widget these fields should be rendered.

Theoretically, the new field type can hold any entity type as a reference. We currently limited it to only work with media, because we only have a nice working widget for media.

We also provide a drush command, to migrate existing entity reference fields, to the new field type.

joachim’s picture

I had a try of https://www.drupal.org/project/media_library_media_modify.

I really like the edit button that shows in the media library widget on the host entity's edit form. Very nice and clean UI!

But I wonder if this pop-up is going to clash with the need to edit media entities in a modal: #2985168: [PP-1] Allow media items to be edited in a modal when using the field widget.

Also, I'm wondering why the overwritten_property_map field column uses big text format rather than the map data type. Is that because of issues with the map type?

Filed some issues over at that module. I seem to be a magnet for bugs, sorry! :/

joseph.olstad’s picture

@joachim, I also tried that module, it conflicted with 2985168, seemed to mess up the configuration too.
For now I'm going with
#2985168-106: [PP-1] Allow media items to be edited in a modal when using the field widget

joachim’s picture

Status: Needs review » Postponed

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joachim’s picture

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Martijn de Wit’s picture

Issue summary: View changes

Added some extra context into the issue summary regarding WCAG specs