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.
Comment | File | Size | Author |
---|---|---|---|
#69 | 2835767-69.patch | 24.51 KB | Wim Leers |
#66 | interdiff-48-66.txt | 1.19 KB | Anonymous (not verified) |
#66 | 2835767-66.patch | 25.94 KB | Anonymous (not verified) |
#49 | interdiff-46-48.txt | 8.54 KB | Anonymous (not verified) |
#49 | 2835767-48.patch | 25.9 KB | Anonymous (not verified) |
Comments
Comment #2
Gábor HojtsySo #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.
Comment #3
Wim LeersWell, Drupal 8 wants to be "API-first", so adding new entity/field stuff without normalization support would violate that.
Comment #4
Gábor Hojtsy@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)?
Comment #5
Wim LeersThe media module. Each module is responsible for providing its own normalizers if it provides
FieldItem
s 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.)Comment #6
Gábor HojtsyThanks 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.
Comment #7
dawehner@Gábor Hojtsy
For me this is not a blocker, unless its installed by default and used, but certainly something to think about.
Comment #8
dawehnerComment #10
dawehnerWe discussed that today on the API FIRST call and thought about disabling it somehow maybe by default.
Comment #11
Wim Leers+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.)
Comment #12
xjmI'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?
Comment #13
xjmLet'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.
Comment #14
xjmFor #12 and #13.
Comment #15
dawehnerShould we have some level of test coverage for that?
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 :)
Comment #16
Gábor HojtsyAgreed it is fine to disable this :)
Comment #17
BerdirSo 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.-
Comment #18
Wim Leers#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
andfile_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.
Comment #19
BerdirIt 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.
Comment #20
Wim Leers#19:
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:
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
Comment #21
Wim Leers@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.
Comment #22
BerdirI 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.
Comment #24
seanBThe 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?
Comment #25
Wim Leers#23
Good point!
I'd say
.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:
file_field
andimage_field
to Media is a big leap forwardThere'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.
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.
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
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:
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.
Comment #26
Wim LeersOops.
Comment #27
Wim LeersAnd this is the interdiff for #26. Sorry for all the noise.
Both #26 and #27 (this comment) should've been part of #25.
Comment #28
Wim LeersI would in fact volunteer to write that Media REST API BC code!
Comment #29
seanBAlso 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.
Comment #30
BerdirNote 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.
Comment #34
Wim LeersFYI: for the
media_entity
contrib module to restore this in 8.4.x contrib, it can add this to its.module
file:and add a
src/MediaEntityServiceProvider.php
file with these contents:Comment #35
Wim LeersWRT it not being clear that this issue is so important by looking at #2825215: Media initiative: Roadmap:
I was not pinged around that time, but I stated the importance of this issue very clearly in #3.
Comment #36
xjmComment #37
larowlanI 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.
Comment #38
xjmThe 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:
media_entity
. Adding this change disrupts that, further complicating the upgrade path for default content and for media_entity REST consumers.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!
Comment #39
Wim Leers#37:
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.
Comment #40
xjmBasically, yes, with the added clarification of "unless we come to a different consensus on this issue within the next week or two".
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.
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:
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.
Comment #41
xjmI 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.
Comment #42
dawehnerI think we have a clash of understanding what REST module is.
The original goal as stated on https://groups.drupal.org/node/262063
Goal: 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.
Comment #43
xjmThanks @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.)
Comment #44
Wim LeersThis 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
to proper tests, just like #2824572: Write EntityResourceTestBase subclasses for every other entity type. did for many entity types. Working on that now…
Comment #45
Wim LeersHere's the REST + HAL test coverage for
MediaType
.Next:
Media
.Comment #46
Wim LeersWell 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:
Media
specifies aadd-page
link relation in its annotation, but that link relation type doesn't exist — added that link relation type.Media
specifies aadmin-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 aboutMediaType
— removed it.Media::setOwnerId()
doesn't return theMedia
entity that's being modified — fixedMediaAccessControlHandler
does not provide a helpful reason for when access is denied for theview
operation — addedComment #47
xjmGreat 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?
Comment #48
Wim Leers#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
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!)
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commented#46: Wow, cool assault in such a short time! A small make-up after that.
Comment #50
Wim Leers@vaplas+++++++++++++++
Comment #51
phenaproximaRe-tagging.
Comment #52
xjmAlright, 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.
Comment #53
Wim LeersThere 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.
Comment #54
dawehnerWell, this is because we simply took it over :)
Comment #55
Wim LeersAll 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?
Comment #56
dawehneryeah I think its ready
Comment #58
Anonymous (not verified) CreditAttribution: Anonymous commentedRandom fail #2898721: Random segfault currently in FileFieldWidgetTest::testMultiValuedWidget().
Comment #60
xjmThis 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.
Comment #61
xjmSome specific notes on splitting this up:
This can go in its own issue.
Both of these seem like they could have their own issue describing the change; otherwise a reviewer has to do archaeology on them.
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.
Comment #62
xjmRegarding #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.
Comment #63
xjmOr, basically, what I said in #47 but phrased as a request rather than a question. :)
Comment #64
Wim LeersI have no idea what this is referring to.
If you're referring to #52, I answered that in #53. You're absolutely right though that the issue summary should've been updated!
(Emphasis mine.)
I could not disagree more. The title is . Let's break that down:
Media
andMediaType
entity typesIOW: this is clearly a test-only issue.
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:
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.)
Same for this. (i.e. this problem was discovered only thanks to the integration test coverage this patch is adding.)
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.
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.
Comment #65
xjmPlease create the separate issues.
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commented#48 needs to be rerolled after
Additional change:
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.
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commentedDone:
Now we can postpone this issue like [PP-4], or close the created issues as a duplicate (I will not be upset)
Comment #68
Wim LeersThank you, vaplas! 👏👏👏
#2905720: Media specifies a add-page link relation, but core.link_relation_types does not have this + #2905722: Media specifies an "admin-form" link relation, but should not. + #2905748: MediaAccessControlHandler does not provide a helpful reason for when access is denied for the view operation all landed. That only leaves #2905738: Media::setOwnerId() doesn't return the Media entity, which I just RTBC'd.
Comment #69
Wim Leers#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
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.
Comment #70
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhy 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?Comment #71
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAlso, 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?Comment #72
Wim Leers#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).
Comment #73
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOh, 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.
Comment #76
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.5.x and cherry picked to 8.4.x.
Comment #77
BerdirDidn'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?
Comment #78
Wim LeersGreat, thanks to #2883813: Move File/Image media type into Standard once Media is stable, the weird
field_media_file_1
thing goes away, see #2883813-26: Move File/Image media type into Standard once Media is stable.