Problem/Motivation

Using media basically reverts #2825812: [PP-1] ImageItem normalizer should expose all public image style URLs. We should figure out a nice way to retrieve media in REST.

Proposed resolution

TODO figure out the behaviour.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

dawehner created an issue. See original summary.

Gábor Hojtsy’s picture

So #2825812: [PP-1] ImageItem normalizer should expose all public image style URLs is not even in core, so we media would not revert it :) Would be important for this to be figured out whether it is a requirement or not for 8.3.0.

Wim Leers’s picture

Well, Drupal 8 wants to be "API-first", so adding new entity/field stuff without normalization support would violate that.

Gábor Hojtsy’s picture

@Wim Leers: ok, do we have a standard core modules follow that media should?

Also should this be categorized under serialization.module or media system (which we recently added)? Would serialization.module be the place to implement this or the media module(s)?

Wim Leers’s picture

The media module. Each module is responsible for providing its own normalizers if it provides FieldItems that have special needs. Normalizers are tagged services.

(HEAD at the time of writing does not yet have such FieldItem-specific normalizers, but several are in the process of being added.)

Gábor Hojtsy’s picture

Component: serialization.module » media system

(HEAD at the time of writing does not yet have such FieldItem-specific normalizers, but several are in the process of being added.)

Thanks that is informative. We need to also define if this issue is SHOULD/MUST or nice to have for media because that will affect when/how it can get committed.

dawehner’s picture

@Gábor Hojtsy
For me this is not a blocker, unless its installed by default and used, but certainly something to think about.

dawehner’s picture

Issue summary: View changes

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

We discussed that today on the API FIRST call and thought about disabling it somehow maybe by default.

Wim Leers’s picture

Title: Deal with Media and REST/normalizers » Deal with Media and REST/normalizers: opt out Media & Media Type entity types from REST support
Priority: Normal » Major
Status: Active » Needs review
FileSize
635 bytes

We discussed that today on the API FIRST call and thought about disabling it somehow maybe by default.

+1, let's do this. This ensures that when we do work on this in the future, that we do not regress/cause BC breaks. By limiting the API surface now, we reserve the freedom to implement this at a later time, rather than the burden of continuing to support whatever incomplete/inconsidered normalization is currently auto-generated.

(@justafish brought this up during the API-First Initiative meeting as being especially painful/Drupalism-y.)

xjm’s picture

I'm fine with this for 8.4.x, but do we need an issue to reimplement it later along the lines of #2825487: Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field?

xjm’s picture

+++ b/core/modules/media/media.module
@@ -48,6 +48,13 @@ function media_theme() {
 /**
+ * Implements hook_rest_resource_alter().
+ */

Let's add an explanation of why we're disabling it to the docblock, and @todo linking that hypothetical followup issue to reimplement it in the preferred way.

xjm’s picture

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

For #12 and #13.

dawehner’s picture

Should we have some level of test coverage for that?

+++ b/core/modules/media/media.module
@@ -48,6 +48,13 @@ function media_theme() {
+ * Implements hook_rest_resource_alter().
+ */
+function media_rest_resource_alter(&$definitions) {
+  unset($definitions['entity:media'], $definitions['entity:media_type']);
+}

Does the media module in core ensure that you cannot install the media_entity module at the same time? In case it doesn't validate that, there would be problems anyway :)

Gábor Hojtsy’s picture

Agreed it is fine to disable this :)

Berdir’s picture

So I might be missing something, but beside our broken and messy image/file item and file upload support in REST, what exactly are we missing?

As far as Entity Field API is concerned, media entity are the most boring and standard thing ever. There is *zero* difference between a node entity with an image field and a media image entity: it is an entity with a certain bundle, has an image field that references the file entity.

The only difference is that when you actually use that media entity on a node, you just have another layer of indirection:

Node [image field] -> File

vs
Node [entity reference field] -> Media [image field] -> File

Yes, it means you need another request to get the media entity and then the file, but that's it.

Media doesn't replace anything at all. It is 100% on top of our existing file/image handling and it uses 100% standard entity field API for storage. There are plugins, but at least the image and file plugins don't do anything beside automatically creating a bunch of (100% standard) fields and storing some metadata in there.

See #1927648-348: Allow creation of file entities from binary data (comment 348 if the direct link doesn't work), I wrote essentially the same thing there. It's overhead but anything that works for image/file fields directly attached to nodes works for media entity. We can discuss about how to improve DX as mentioned over there.. like exposing useful metadata directly on the media entity or even the media reference (but that's tricky as it is a normal entity reference and normalizers works based on classes only, which is a bad match for our data structures :() and providing optimized endpoints for creating media entities but that's just improvements over what we have by default.-

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs followup
FileSize
2.56 KB
2.71 KB

#12: Yep, and unlike how we shipped Drupal 8.0.0 with Serialization + REST, we'll then actually have an issue filed to work this out. Created issue: #2895573: Agree on normalization of Media module entities.

#13: Done.

#15: Done.

#17: We are missing a consciously designed normalization. We never did that in the past, but that doesn't mean we need to keep making that mistake. And yes, messy image_field and file_field normalizations are enough reason to not yet expose it today, as well as the inability to upload files at all.
We're already hearing that Media+REST in 8.3 contrib is a PITA to use (and it's going to be pretty much the same in 8.4 in core), as mentioned in #11, and so rather than just blindly shipping what kinda works today, and pretending it's fine as is, this issue is about not enabling REST support right from the start, but explicitly adding it only once thought has been put into it and there is explicit test coverage.
Even if all of that doesn't convince you, there's an even simpler answer: because new code in core should not make the problem of making Drupal 8 actually be API-First even bigger. Either it pays the integration cost (in the form of test coverage to prove that it's all working), or it doesn't. If it doesn't, then it doesn't get to be exposed via REST, because otherwise it'd increase our technical debt.
IOW: this is first and foremost about managing technical debt.

Berdir’s picture

It might be a pain, yes, and uploading doesn't work at all without some contrib modules or core patches.

But with media_entity, it was *possible*, however ugly it was, to get the data out. And now we remove that completely in 8.4.x, which means 8.3+media_entity sites can not update to 8.4? (Beside all the other problems that we still need to resolve, like update that/API updates in e.g. entity_browser and other additional modules that will remain in contrib).

Again keep in mind that decoupled sites is just one use case, there might also be sites that use deploy or similar functionality to sync data between sites, we also break that.

As far as I understand, the problems are also mostly about normalization being tedious and involving multiple references. But this patch doesn't remove normalization, which also means normalization is still there and "supported". And absolutely required, for example for default_content integration, which is important for various install profiles like thunder (http://cgit.drupalcode.org/thunder/tree/modules/thunder_demo/content/media). So either we do commit to supporting normalization, but then we can't make breaking changes there anyway or we also break installation profiles like thunder in 8.4 and make updating even harder.

Media is not a new module/functionality, it is something we moved from contrib and has 15k 8.x installations.

That is, unless we keep maintaining media_entity in 8.4 and support it fully there and only provide an update for 8.5 or even later. But I think that would be pretty bad for everyone. Development in media_entity already basically stopped since work started on bringing it into core.

Wim Leers’s picture

#19:

  1. Sites that were using this in Drupal <8.4 + Media in contrib would have to update their code already anyway, because many things have been renamed. See https://www.drupal.org/node/2863992
  2. A contrib module can bring back normalization/REST support until it lands in core.

Media is not a new module/functionality, it is something we moved from contrib and has 15k 8.x installations.

The module's code has been changed quite significantly.

More importantly: AFAICT no attention was paid at all to REST support/normalization/etc during the development of https://www.drupal.org/project/media_entity:

  1. searching for "normalizer": 0 hits
  2. searching for "REST": 3 hits, of which only 1 relevant: #2884175: Media Bundle entity type does not have an access controller

I'm just being realistic. Either Media becomes stable in Drupal 8.4, or it doesn't. Given that everything indicates no time was spent at all on the normalization of Media entities nor their security implications (as #2884175: Media Bundle entity type does not have an access controller indicates, which does also apply to core: there's no access control handler for Media Type entities yet). I don't think we want to block Media becoming stable, which then implies that we need to disable this for now. If we don't, we're knowingly creating security problems, BC problems, DX problems

Wim Leers’s picture

@amateescu pointed out at the very similar #2843753-18: Prevent ContentModerationState from being exposed by REST's EntityResource that it could be a kernel test. The same is true here.

Berdir’s picture

I know the change record, but most of those changes are about the bundle/plugin structure and their API. The media entity and its field were actually kept basically identical on purpose, to avoid complex data migrations. The bundle field is still "bundle" for example and was not renamed to "type", despite the fact that we renamed MediaBundle to MediaType. You can actually diff the two Entity classes and see that we now generate some fields and changed some labels and descriptions and other settings, but those don't really affect the structure/storage.

That also means that there are not many changes as far as REST/Normalization is concerned.

And media_entity did not pay attention to REST/normalization because it didn't have to. It uses standard entity/field API, special field types (e.g. for video) are provided by other modules and normalization is their concern. It's not well suited for decoupled sites, but it is there and it works just as well as normal image/item fields. As mentioned and linked, thunder and I'm sure many other install profiles use media entities in combination with default_content and content synchronization.

You didn't yet respond to my comment/feedback on normalization. This only removes REST integration, but REST is little more than the HTTP interface to the normalization API. Does that mean we *do* commit to the normalization structure that we have now and support that? If yes, then I personally don't care that much about whether we remove those rest plugins or not, but especially then I don't really see the point of doing that. If not, then this patch doesn't go far enough but going far enough would break a lot of other things.

You misunderstood #2884175: Media Bundle entity type does not have an access controller, there is an admin permission for the media_bundle/media_type config entities, which means it is perfectly secure, the issue is about allowing more access. So nobody without administer media bundles can do anything to those entities, in the UI or REST. So media_type is realistically already pretty locked down and that's OK.

Status: Needs review » Needs work

The last submitted patch, 21: 2835767-21.patch, failed testing. View results

seanB’s picture

The upgrade from contrib to core involves updating the media_entity module. Technically the media_entity module could support cases like these that involves more discussion to get them into core for 8.4. How about the new media_entity 2.x module contains a upgrade path and possible missing features?

Wim Leers’s picture

#23

That also means that there are not many changes as far as REST/Normalization is concerned.

Good point!

It's not well suited for decoupled sites, but it is there and it works just as well as normal image/item fields.

I'd say just as terribly.

The point of Media is to get a single standardized approach that works well for all kinds of media, and allows for reuse. The point of this issue is that if we are finally doing this big overhaul of the asset/media handling in Drupal core, that we should ensure the API-First aspects of it are also taken care of. In other words:

  1. file_field and image_field to Media is a big leap forward
  2. the normalization/API-First capabilities should make a similar step forward, and should not stay stuck with all the problems of the past

You didn't yet respond to my comment/feedback on normalization. This only removes REST integration, but REST is little more than the HTTP interface to the normalization API.

There's no way to remove a normalization that relies 100% on the default generic normalizers, that's why this issue doesn't do that.
Actually, while working on this comment, I realized that there is a way. Updated patch attached that provides a normalizer for Media entities that explicitly throws an exception.

Does that mean we *do* commit to the normalization structure that we have now and support that?

Without test coverage, there is no way to commit to a normalization structure. Test coverage is a hard requirement for that. Otherwise it's impossible to know whether a regression/BC break occurred. Therefore no test coverage = no guarantees = no commit to a normalization structure.

If not, then this patch doesn't go far enough but going far enough would break a lot of other things.

I think this patch is going far enough — if modules like default_content and distributions like Thunder wish to rely on normalizations of entity types without any test coverage whatsoever, that's their right. But it doesn't mean they also get the right to claim that there is a BC break. There can't be a BC break if there isn't yet a official/formal (explicitly supported) normalization of a particular entity type.

(That being said, I think it's totally possible to provide a BC layer for those normalizations in the future, when core does ship with an official normalization.)


#24

How about the new media_entity 2.x module contains a upgrade path and possible missing features?

Exactly! And with the newly added normalizer here, that contrib module can just remove the serializer.normalizer.media service that this patch adds.


@Berdir, I completely understand your concerns and frustrations. I really do!

But:

  1. We cannot ignore long-term consequences. If we ship a future Drupal 8 release with a complete Media solution (including a library etc), but then the API-First aspects of it are completely unusable, then we'll have shot ourselves in the foot: not only will fixing/improving that normalization be a BC nightmare, it will also have given a very negative impression to those who start building decoupled sites that use Media.
  2. Similarly, shipping Drupal 8.4 without Media altogether would be a huge blowback.
  3. That's why I am proposing (and I think @dawehner and @seanB are too, but I don't want to put words in their mouths) to strike a nice balance that maximizes on our joint standardization efforts while minimizing risk: we ship Media as stable but hidden in 8.4 without normalization/REST support, so the ecosystem can standardize on it. For normalization/REST support, a contrib module will need to be installed. (We can even have the UnsupportedException message point to that module.)

I totally agree this is not ideal. Ideally, the Media Entity module in contrib would have already dealt with this. It's totally understandable that they didn't though — for starters, because the Serialization/REST modules were not remotely mature when they worked on Media! But I think it's important we find a good balance between what is very necessary for the ecosystem (Media stable but hidden in 8.4.0) and what is supportable/maintainable.

Wim Leers’s picture

Wim Leers’s picture

FileSize
1.69 KB

And this is the interdiff for #26. Sorry for all the noise.

Both #26 and #27 (this comment) should've been part of #25.

Wim Leers’s picture

(That being said, I think it's totally possible to provide a BC layer for those normalizations in the future, when core does ship with an official normalization.)

I would in fact volunteer to write that Media REST API BC code!

seanB’s picture

Also posted this in #2825215: Media initiative: Essentials - first round of improvements in core:
Just discussed this with Wim Leers. I totally agree that we should not expose a media API in core yet. We are probably going to want to fix a whole bunch of things. Exposing the API will prevent us from changing certain things because of BC.

Media entity in contrib could contain an upgrade path and add support for REST while we are working on this, and could theoretically keep providing support for the "broken" version of the API for as long as people need it. Upgrading to the new and improved core API can be done in their own pace.

Berdir’s picture

Note that even though you now also remove media entity normalization, the entity reference fields are still there, which means that REST exposes media references and points to URL's that then won't exist.

But Ok. I think I voiced my "concerns and frustrations" sufficiently and even though I still can't really see the argument, I'll let the maintainers/committers make the decision. I guess it is acceptable to have an contrib module that does nothing but override those services/hooks again. Alternatively, we could also have this based on a setting.

I think it would make sense to check this with @slashrsm as well, even though he's not an active maintainer of the core module at the moment.

For reference, my concerns/comments are comments #17, #19, #22.

The last submitted patch, 25: 2835767-25-test_only_FAIL.patch, failed testing. View results

The last submitted patch, 25: 2835767-25.patch, failed testing. View results

The last submitted patch, 26: 2835767-26-test_only_FAIL.patch, failed testing. View results

Wim Leers’s picture

FYI: for the media_entity contrib module to restore this in 8.4.x contrib, it can add this to its .module file:

/**
 * Implements hook_rest_resource_alter().
 *
 * Restores what media_rest_resource_alter() removes.
 *
 * @todo Remove this after https://www.drupal.org/node/2895573 lands in core.
 */
function media_entity_rest_resource_alter(&$definitions) {
  // Add in the default plugin configuration and the resource type.
  foreach (\Drupal::entityTypeManager()->getDefinitions() as $entity_type_id => $entity_type) {
    if (!in_array($entity_type_id, ['media', 'media_type'], TRUE)) {
      continue;
    }

    $definitions[$entity_type_id] = [
      'id' => 'entity:' . $entity_type_id,
      'entity_type' => $entity_type_id,
      'serialization_class' => $entity_type->getClass(),
      'label' => $entity_type->getLabel(),
    ];

    $default_uris = [
      'canonical' => "/entity/$entity_type_id/" . '{' . $entity_type_id . '}',
      'create' => "/entity/$entity_type_id",
    ];

    foreach ($default_uris as $link_relation => $default_uri) {
      // Check if there are link templates defined for the entity type and
      // use the path from the route instead of the default.
      if ($link_template = $entity_type->getLinkTemplate($link_relation)) {
        $definitions[$entity_type_id]['uri_paths'][$link_relation] = $link_template;
      }
      else {
        $definitions[$entity_type_id]['uri_paths'][$link_relation] = $default_uri;
      }
    }

    $definitions[$entity_type_id] += [
      'class' => \Drupal\rest\Plugin\rest\resource\EntityResource::class,
      'provider' => 'rest',
    ];
  }
}

and add a src/MediaEntityServiceProvider.php file with these contents:

<?php

namespace Drupal\media_entity;

use Drupal\Core\DependencyInjection\ContainerBuilder;
use Drupal\Core\DependencyInjection\ServiceModifierInterface;

/**
 * @todo Remove this after https://www.drupal.org/node/2895573 lands in core.
 */
class MediaEntityServiceProvider implements ServiceModifierInterface {

  /**
   * {@inheritdoc}
   */
  public function alter(ContainerBuilder $container) {
    $container->removeDefinition('serializer.normalizer.media');
  }

}
Wim Leers’s picture

WRT it not being clear that this issue is so important by looking at #2825215: Media initiative: Essentials - first round of improvements in core:

  1. this issue was created in December 2016, more than 7 months ago
  2. https://www.drupal.org/node/2825215/revisions/view/10453076/10453135 -> on April 24, this was added to the "Should have" list, with a note to check with me (probably because I'm one of the API-First Initiative coordinators and the only remaining REST module maintainer)

I was not pinged around that time, but I stated the importance of this issue very clearly in #3.

xjm’s picture

larowlan’s picture

I think comment #19 highlights my 'we're competing with ourself' observation about Content moderation vs workbench moderation, there are only so many contributors and we've essentially duplicated efforts. However, that is more of an observation than anything useful for this discussion.

I think both @Berdir and @Wim Leers make excellent points, which makes this very hard to resolve.

If we don't provide media normalizers, then we risk breaking install profiles that already rely on their presence via default_content (admittedly I'm biased there).

But also if we introduce something that we're not 100% happy with, then we're stuck supporting it longer term - i.e. the technical debt.

If we rush something in now, it might mean that default_content and install profiles are happy, but it might not be the right solution either.

I think a compromise would be to collaborate on a contrib module that added back support that default content needed, but because its in contrib - we have an environment for more rapid evolution. Install profiles can then add that dependency. Realistically, they're going to need lots of other media family modules from contrib anyway. We already use entity module for a proving ground for api improvements to entity API, this would be no different.

The alternative is that this module is in core, but is hidden too. But I would argue that would make it more difficult to iterate on.