Problem/Motivation

We decided to ship Media with its REST support as-is (i.e. using core's default normalizers), because that ensures backwards compatibility with the contrib Media module in 8.3.x and before.

To improve Media entities' normalization, we have #2895573: Decide on normalization of Media module entities to add custom normalizers to improve the DX in 8.5.0.

Proposed resolution

However, we should still ensure we don't regress Media entities' normalization unknowingly, so we still need test coverage. This issue then is therefore solely about adding test coverage, just like we are doing/did for all other entity types: #2824572: Write EntityResourceTestBase subclasses for every other entity type..

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#69 2835767-69.patch24.51 KBWim Leers
#66 interdiff-48-66.txt1.19 KBAnonymous (not verified)
#66 2835767-66.patch25.94 KBAnonymous (not verified)
#49 interdiff-46-48.txt8.54 KBAnonymous (not verified)
#49 2835767-48.patch25.9 KBAnonymous (not verified)
#46 2835767-46.patch27.5 KBWim Leers
#46 interdiff.txt19.65 KBWim Leers
#45 2835767-45.patch8.65 KBWim Leers
#27 interdiff.txt1.69 KBWim Leers
#26 2835767-26.patch7.99 KBWim Leers
#26 2835767-26-test_only_FAIL.patch4.45 KBWim Leers
#25 2835767-25.patch6.35 KBWim Leers
#25 2835767-25-test_only_FAIL.patch4.45 KBWim Leers
#25 interdiff.txt3.72 KBWim Leers
#21 2835767-21.patch2.57 KBWim Leers
#21 2835767-21-test_only_FAIL.patch1.46 KBWim Leers
#21 interdiff.txt1.3 KBWim Leers
#18 2835767-18.patch2.71 KBWim Leers
#18 interdiff.txt2.56 KBWim Leers
#11 2835767-11.patch635 bytesWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

Gábor Hojtsy’s picture

So #2825812: ImageItem should have an "derivatives" computed property, to expose all 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 entities should expose the file URL as a computed property on the 'uri' 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 via REST requests (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: Decide 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: Roadmap:
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: Roadmap:

  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.

xjm’s picture

The committers discussed this issue yesterday, both about whether it's required to mark Media stable and about the change itself.

In general, we see the concern about the sub-par DX for the API-first implementation, and about adding technical debt for API-first as we add features to core. We will usually prefer to err on the side of excluding incomplete functionality from a feature, then adding it once it's ready to be stable to reduce the maintenance burden.

In the case of this issue, there are a few counterpoints:

  1. The primary goal for core Media in 8.4.x is to provide a stable API for contrib and ensure a smooth transition from media_entity. Adding this change disrupts that, further complicating the upgrade path for default content and for media_entity REST consumers.
  2. File, Image, and Entity Reference fields generally already have similar DX.
  3. Media is mostly just an Entity Reference implementation; in itself, it's not adding new normalizer implementations.
  4. As @Berdir points out:

    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.

So, based on all that, we agreed that this issue won't block Media being marked stable for 8.4.x. We do still want Media to provide good DX for decoupled implementations, so the original scope of this issue (providing better normalizers for Media) will still be on the Media roadmap for future releases.

We can also continue to discuss the current scope. If we come up with an approach that resolves my point 4 above and come to a consensus that contrib maintainers are comfortable with, then we can still consider an approach where core turns off the REST support and media_entity in contrib re-enables it, up until the release of 8.4.0-beta1. Thereafter, though, we should provide BC for whatever is currently in core, and we might want to postpone this issue on #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field so that Media can provide a matching implementation to the final solution there.

Thanks everyone for your discussion here!

Wim Leers’s picture

#37:

Realistically, they're going to need lots of other media family modules from contrib anyway

Precisely.

#38: The conclusion is not very clear to me. It sounds like "we will just ship the broken stuff we have today and pretend it's both stable and supportable"?
Which would imply that Media is not at all "API-First", which means Media will be as awful as File/Image are today (with horribly broken normalizations, an absolute nightmare for fixing it since it requires breaking BC, and again no support for uploads). I really can't fathom why this is any better than what #37 proposes. We are again making the mistake of marking something unreliable as stable and not paying the integration cost. Put another way: to prevent a known cost for today's sites, we're forcing a hidden cost on all future new sites.

I know I'm starting to sound like a broken record, but it feels like my duty to repeat the problems with this approach succinctly.

It *is* possible to do #38, but it will be much costlier. But we'll get it done.

I rest my case.

xjm’s picture

The conclusion is not very clear to me. It sounds like "we will just ship the broken stuff we have today and pretend it's both stable and supportable"?

Basically, yes, with the added clarification of "unless we come to a different consensus on this issue within the next week or two".

Which would imply that Media is not at all "API-First", which means Media will be as awful as File/Image are today (with horribly broken normalizations, an absolute nightmare for fixing it since it requires breaking BC, and again no support for uploads).

Yes. The first goal of the media initiative is to provide a stable API and data model for contrib, and the second is feature parity with file and image. So, an API that's equivalent to core file, image, and entity reference fields generally meets these requirements. In this case, Media is not exposing new technical debt or increasing the severity of problems that exist in core. This is an existing issue since release that, while certainly major, is not made worse by the current state of Media.

However, let's please stop using emotionally loaded language to describe the entity API and the DX of its normalizations, REST support, etc. Words like "nightmare", "awful", "horribly broken", etc. are not helpful. They are unnecessarily negative and unnecessarily insulting to contributors who put hundreds of hours into vastly improving these subsystems over what was available for Drupal 7. Please see http://xjmdrupal.org/blog/someone-worked-hard-on-it. We all recognize and understand that Drupal has a long way to go to provide a delightful decoupled experience. Let's take that as a given, let's acknowledge that we all want Drupal to be a pleasure to use for all these usecases, and let's recognize that sometimes we'll need to prioritize one improvement over another.

I really can't fathom why this is any better than what #37 proposes.

Because of #30, because Media is not just a new module (again, the first priority is to minimize the disruption in the upgrade path for contrib and existing sites), because Media not worse than what we have, and because an Entity API subsystem maintainer disagrees with the current suggested approach for this issue. The framework managers have a couple additional ideas to propose, but we agreed that marking Media stable as-is in 8.4.x was preferable to rushing to a decision on how to "unsupport" REST and normalizers, and we don't even have consensus that unsupporting them is preferable over the state in HEAD.

As I mentioned, this issue will still be a part of the Media initiative's roadmap, so that in the long term, Media provides improvements over File and Image for both the Drupal UI and decoupled applications. We can and should continue the discussion here on how to best do that, and if we come to a resolution in the next 1-3 weeks we can adopt that solution for 8.4.x, but it won't block marking Media stable now.

Edit: regarding:

Put another way: to prevent a known cost for today's sites, we're forcing a hidden cost on all future new sites.

This issue remains on the roadmap to complete before the module is shown in the UI, currently as a must-have and it will remain at least a should-have for that milestone, which is the milestone for Media and new sites.

xjm’s picture

I forgot to mention it above, but the committers also agreed that we will document in the release notes and so forth that the API-first implementations are not final, regardless of the decisions here.

dawehner’s picture

I think we have a clash of understanding what REST module is.

The original goal as stated on https://groups.drupal.org/node/262063Goal: You may know modules like Services or RESTWS, we want something similar for Drupal 8 core. It should be possible to simply enable a web API that automatically exposes any entity type via a RESTful interface. That means that we can easily apply CRUD operations (Create, Read, Update and Delete) to entities remotely. The use cases are for example native mobile applications that interact with a Drupal site, third party services that can consume Drupal resources in a standard format, mass migration of legacy data, deployment of staging content ... and a lot more. The key feature is a standard-compliant, machine-readable interface that opens Drupal up for integration with any external system.
at least makes it primarily a tool which exposes everything, especially for the usecase of content deployment.

Then there is the other side, which sees REST as an interface for decoupled applications. Those requirements are different to content deployment, as for example the HTTP response size matters a bit.

At least for me the general future of the REST module vs. maybe something else targeted for decoupled applications only, is basically the same question as what @Wim Leers and @Berdir discussed already.

xjm’s picture

Thanks @dawehner. A computed base field would comply with that original intent for REST, right? Similarly to #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field. Adding upload capability for full CRUD would also comply with that original intent, although it's a separate scope/missing feature vs. the computed field. I think maybe we should postpone this on #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field, because these questions would presumably be the same as for that issue, and because I'm increasingly convinced we should just let core continue to expose the API it already provides for this entity reference, and make improvements as feature additions.

#42 also seems like an important higher-level discussion; are there any policy/plan/ideas issues discussing this elsewhere that we should redirect to? (I know we have proposed adding e.g. JSON API but I'm not very familiar with the API-first roadmap.)

Wim Leers’s picture

Title: Deal with Media and REST/normalizers: opt out Media & Media Type entity types from REST support » Media + REST: comprehensive test coverage for Media + MediaType entity types
Assigned: Unassigned » Wim Leers
Status: Needs review » Needs work
Parent issue: » #2824572: Write EntityResourceTestBase subclasses for every other entity type.

but the committers also agreed that we will document in the release notes and so forth that the API-first implementations are not final

This is super important — it sends a strong signal that the API-first implementation is bound to change. That achieves roughly the same as what I was trying to achieve with the patch: Media is stable for site builders (config), PHP developers (PHP API), but not for decoupled developers (REST API).

Given that, we need to drop 100% of this patch.

And specifically, we need to convert

+++ b/core/modules/media/tests/src/Kernel/MediaResourceTest.php
@@ -0,0 +1,47 @@
+class MediaResourceTest extends KernelTestBase {

to proper tests, just like #2824572: Write EntityResourceTestBase subclasses for every other entity type. did for many entity types. Working on that now…

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.65 KB

Here's the REST + HAL test coverage for MediaType.

Next: Media.

Wim Leers’s picture

Well that took a lot longer than I expected. I found multiple bugs that needed new issues (#2897009: MediaInterface is missing setName() and getName() + #2897026: \Drupal\media\Entity\Media::preSaveRevision() handles the revision timestamp compared to other entity types) plus some issues that I had to fix in this patch:

  1. Media specifies a add-page link relation in its annotation, but that link relation type doesn't exist — added that link relation type.
  2. Media specifies a admin-form link relation in its annotation, but that link relation type A) does not exist,
    B) does not make sense, because it's not about Media, but about MediaType — removed it.
  3. Media::setOwnerId() doesn't return the Media entity that's being modified — fixed
  4. MediaAccessControlHandler does not provide a helpful reason for when access is denied for the view operation — added
xjm’s picture

Great to see the test coverage since it helps us meet the testing gate for the module, but don't we still need the original scope of this issue ("a nice way to retrieve media in REST" that matches the eventual File and Image improvements) as a feature request? Shouldn't the tests get their own issue?

Wim Leers’s picture

#47: we already have #2895573: Decide on normalization of Media module entities for that. This issue is about dealing with Media+REST/normalization before shipping 8.4.0. Before shipping 8.4.0, it's essential that we either

  1. remove REST/normalization support and have test coverage proving it is removed (#10 through #43)
  2. test coverage proving that the current normalization works, so that we cannot change it (and hence regress) without knowing (#44 and later), and so that when we do change the normalization of Media entities, we'll be able to have solid test coverage for BC layers

No matter which we choose, the next step is #2895573: Decide on normalization of Media module entities.

(I created this in #18 and I now see I forgot to add it as a related issue — sorry!)

Anonymous’s picture

#46: Wow, cool assault in such a short time! A small make-up after that.

Wim Leers’s picture

@vaplas+++++++++++++++

phenaproxima’s picture

Issue tags: -D8Media +Media Initiative

Re-tagging.

xjm’s picture

Alright, I'll update the roadmap so that #2895573: Decide on normalization of Media module entities replaces the original scope of this issue and this issue is testing gate coverage for the initial release. We should make sure that all the relevant discussion from here is quoted and documented in that issue, though, because we did sort of replace the original scope of the OP here.

Wim Leers’s picture

There has been zero discussion on this issue about the normalization, so this issue never complied with the original scope in the first place.

All discussion here has been about BC handling of the auto-generated normalization. The chosen direction means supporting that auto-generated normalization, and we can only support it if we have test coverage.

#2895573 already clearly refers back to this issue, so rest assured, the discussion here will not be forgotten.

dawehner’s picture

There has been zero discussion on this issue about the normalization, so this issue never complied with the original scope in the first place.

Well, this is because we simply took it over :)

Wim Leers’s picture

All I meant was that #52 was saying "all relevant discussion here is quoted and documented" — but there never was any discussion about how to make the normalization work, so there's nothing to quote/document. That's all :)

So … I think this is ready?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

yeah I think its ready

Status: Reviewed & tested by the community » Needs work

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

Anonymous’s picture

Status: Needs work » Reviewed & tested by the community

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

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue title again confused us looking at the issue queue. I see that I already asked about this a month ago.

Also, it looks like the most recent test runs failed and the issue didn't get marked NW, possibly because of the issue migration.

I've brought this up a few times before on similarly titled issues, but please follow the issue scope guidelines and put added test coverage (which can be committed to any branch at any time) in separate issues from API additions, access fixes, etc. The title of this issue does not convey its purpose.

Please split generalized test coverage out into its own issues, and file dedicated issues for things that are bugs. Lumping bugfixes into an issue with this title delays the fix because it makes the issue seem less important when reading the title or summary.

xjm’s picture

Some specific notes on splitting this up:

  1. +++ b/core/core.link_relation_types.yml
    @@ -3,6 +3,9 @@
     add-form:
       uri: https://drupal.org/link-relations/add-form
       description: A form where a resource of this type can be created.
    +add-page:
    +  uri: https://drupal.org/link-relations/add-page
    +  description: A page where a resource of this type and related types can be created.
     delete-form:
       uri: https://drupal.org/link-relations/delete-form
    

    This can go in its own issue.

  2. +++ b/core/modules/media/src/Entity/Media.php
    @@ -77,7 +77,6 @@
      *     "delete-form" = "/media/{media}/delete",
      *     "edit-form" = "/media/{media}/edit",
      *     "revision" = "/media/{media}/revisions/{media_revision}/view",
    - *     "admin-form" = "/admin/structure/media/manage/{media_type}"
      *   }
      * )
      */
    @@ -124,7 +123,7 @@ public function getOwnerId() {
    
    @@ -124,7 +123,7 @@ public function getOwnerId() {
        * {@inheritdoc}
        */
       public function setOwnerId($uid) {
    -    $this->set('uid', $uid);
    +    return $this->set('uid', $uid);
    

    Both of these seem like they could have their own issue describing the change; otherwise a reviewer has to do archaeology on them.

  3. +++ b/core/modules/media/src/MediaAccessControlHandler.php
    @@ -23,9 +23,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
         $is_owner = ($account->id() && $account->id() === $entity->getOwnerId());
         switch ($operation) {
           case 'view':
    -        return AccessResult::allowedIf($account->hasPermission('view media') && $entity->isPublished())
    +        $access_result = AccessResult::allowedIf($account->hasPermission('view media') && $entity->isPublished())
               ->cachePerPermissions()
               ->addCacheableDependency($entity);
    +        if (!$access_result->isAllowed()) {
    +          $access_result->setReason("The 'view media' permission is required and the media item must be published.");
    +        }
    +        return $access_result;
    

    This seems like an important bugfix and should have its own issue, with a title describing the fix and the relevant part of the test coverage. It might be even that most of the patch is test coverage for just this bug and any other access issues; if so, the title should reflect that and it'd be categorized as a bug, complete with test-only patch exposing the new coverage.

xjm’s picture

Regarding #61.3, I misread that; looks like maybe it's just adding a missing access denied message. So that would be sensible as an independent bugfix as well.

xjm’s picture

Or, basically, what I said in #47 but phrased as a request rather than a question. :)

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

This issue title again confused us looking at the issue queue.

I have no idea what this is referring to.

I see that I already asked about this a month ago.

If you're referring to #52, I answered that in #53. You're absolutely right though that the issue summary should've been updated!

I've brought this up a few times before on similarly titled issues, but please follow the issue scope guidelines and put added test coverage (which can be committed to any branch at any time) in separate issues from API additions, access fixes, etc. The title of this issue does not convey its purpose.

(Emphasis mine.)
I could not disagree more. The title is Media + REST: comprehensive test coverage for Media + MediaType entity types. Let's break that down:

  • Media + REST: ok, so the integration of the Media + REST modules
  • comprehensive test coverage for Media + MediaType entity types: ok so this is adding comprehensive REST test coverage for the Media module's Media and MediaType entity types

IOW: this is clearly a test-only issue.

Please split generalized test coverage out into its own issues, and file dedicated issues for things that are bugs. Lumping bugfixes into an issue with this title delays the fix because it makes the issue seem less important when reading the title or summary.

Ahhh, so this is what you mean (you'll forgive me for not remembering every line of this patch after it's been RTBC for more than a month). We can't do those bugfixes in separate issues, because those (small) bugs were only discovered thanks to this test coverage.

Finally, it seems at least some of those test failures are due to random failures from a while ago. So, retesting them.


Let's look at those small non-pure-test-coverage things:

  1. +++ b/core/core.link_relation_types.yml
    @@ -3,6 +3,9 @@
    +add-page:
    +  uri: https://drupal.org/link-relations/add-page
    +  description: A page where a resource of this type and related types can be created.
    

    This can't go in its own issue because there's no sensible way to test this on its own. (i.e. this problem was discovered only thanks to the integration test coverage this patch is adding.)

  2. +++ b/core/modules/media/src/Entity/Media.php
    @@ -77,7 +77,6 @@
    - *     "admin-form" = "/admin/structure/media/manage/{media_type}"
    

    Same for this. (i.e. this problem was discovered only thanks to the integration test coverage this patch is adding.)

  3. +++ b/core/modules/media/src/Entity/Media.php
    @@ -124,7 +123,7 @@ public function getOwnerId() {
       public function setOwnerId($uid) {
    -    $this->set('uid', $uid);
    +    return $this->set('uid', $uid);
       }
    

    This is a super trivial bugfix. It's uncovered by this test coverage. Writing a specific unit test for this is not sensible — IMHO it'd be a waste of time.

  4. +++ b/core/modules/media/src/MediaAccessControlHandler.php
    @@ -23,9 +23,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
    -        return AccessResult::allowedIf($account->hasPermission('view media') && $entity->isPublished())
    +        $access_result = AccessResult::allowedIf($account->hasPermission('view media') && $entity->isPublished())
               ->cachePerPermissions()
               ->addCacheableDependency($entity);
    +        if (!$access_result->isAllowed()) {
    +          $access_result->setReason("The 'view media' permission is required and the media item must be published.");
    +        }
    +        return $access_result;
    
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php
    @@ -0,0 +1,259 @@
    +      case 'GET';
    +        return "The 'view media' permission is required and the media item must be published.";
    

    There's zero change in the logic here, this is only adding a reason message so that the test coverage can assert a sensible 403 response body.

Creating separate issues for these = massive overhead IMHO. They're problems uncovered by the test coverage here. If any of them were significant bugs, I'd agree with creating separate issues.

xjm’s picture

Please create the separate issues.

Anonymous’s picture

#48 needs to be rerolled after

Additional change:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php
@@ -95,12 +95,12 @@ protected function createEntity() {
-    $media->setOwnerId(static::$auth ? $this->account->id() : 0)
...
       ->setCreatedTime(123456789)
+      ->setOwnerId(static::$auth ? $this->account->id() : 0)
       ->setRevisionUserId(static::$auth ? $this->account->id() : 0)

Just a permutation, for a more pretty looks 🎨

I completely agree with Wim Leers, but if we need separate tests, no problem. It does not hurt. Now I will be working on these issues.

Wim Leers’s picture

Title: Media + REST: comprehensive test coverage for Media + MediaType entity types » [PP-1] Media + REST: comprehensive test coverage for Media + MediaType entity types
Status: Needs review » Postponed
Wim Leers’s picture

Title: [PP-1] Media + REST: comprehensive test coverage for Media + MediaType entity types » Media + REST: comprehensive test coverage for Media + MediaType entity types
Status: Postponed » Reviewed & tested by the community
Issue tags: -Needs framework manager review
FileSize
24.51 KB

#2905738: Media::setOwnerId() doesn't return the Media entity was just committed. Rebased this patch. Thanks to 3-way rebase, I didn't have to do anything :)

Now this patch is a pure test coverage patch (hence I can remove the Needs framework manager review tag).

Which also means it could also be committed to 8.4.x. All 4 issues in #67 were cherry-picked to 8.4.x, so that should be possible.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
+        $this->baseUrl . '/rest/relation/media/camelids/field_media_file_1' => [
+        $this->baseUrl . '/rest/relation/media/camelids/field_media_file_1' => [
+      'field_media_file_1' => [
+      'field_media_file_1' => [

Why is this suffixed with _1? Shouldn't that only happen if MediaSourceBase::getSourceFieldName() finds that 'field_media_file' is already taken? But what in the test is causing it to be already taken?

effulgentsia’s picture

+class MediaHalJsonBasicAuthTest extends MediaHalJsonAnonTest {
+class MediaHalJsonCookieTest extends MediaHalJsonAnonTest {

Also, see #2835845-45: EntityResource: Provide comprehensive test coverage for BlockContent entity. I think a base test class would make more sense than having Anon serve as a base class. Or, is there a reason we're doing it this way here?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#70: This is not something this test is choosing specifically. It's \Drupal\media\MediaSourceBase::createSourceFieldStorage() that does this. Making that more elegant is out of scope for this issue: the point is to add test coverage for existing logic, not to clean up existing logic, nor to clean up existing default config.

#71: See #2835845-46: EntityResource: Provide comprehensive test coverage for BlockContent entity + #2835845-47: EntityResource: Provide comprehensive test coverage for BlockContent entity: this test follows the pattern established by dozens of other tests (i.e. REST test coverage for entity types).

effulgentsia’s picture

But what in the test is causing it to be already taken?

Oh, I see, media.module includes default config for field.storage.media.field_media_file. That's what causes it to already be taken and the test setup to therefore create a separate field. I wonder if we should have a follow-up for adding test coverage to also cover the field created in default config, since that is likely the more common one to be accessed via REST for most sites.

Ticking credit boxes for everyone who left significant comments on this issue.

  • effulgentsia committed 3b6a05b on 8.5.x
    Issue #2835767 by Wim Leers, vaplas, xjm, dawehner, Gábor Hojtsy, Berdir...

  • effulgentsia committed 73ba3a9 on 8.4.x
    Issue #2835767 by Wim Leers, vaplas, xjm, dawehner, Gábor Hojtsy, Berdir...
effulgentsia’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Pushed to 8.5.x and cherry picked to 8.4.x.

Berdir’s picture

Didn't really read the last few comments in depth, but that sounded very related to #2883813: Move File/Image media type into Standard once Media is stable, which I guess is going to break now?

Wim Leers’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.