Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The MediaInterface
is incomplete, because there are no getter/setter methods on the interface for all base fields.
Specifically, setName()
and getName()
.
Marked as major because we're about to ship 8.4.x, with Media as stable, at which point we won't be able to add this easily anymore.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#55 | 2897009-55.patch | 1.9 KB | Lendude |
#45 | drupal-2897009-45.patch | 14.13 KB | cburschka |
#41 | interdiff-v1_37-41.txt | 681 bytes | Anonymous (not verified) |
#41 | 2897009-41.patch | 14.2 KB | Anonymous (not verified) |
#37 | v2_interdiff-25-37.txt | 688 bytes | Anonymous (not verified) |
Comments
Comment #2
Wim LeersNoticed at #2835767-45: Media + REST: comprehensive test coverage for Media + MediaType entity types.
Comment #3
Wim LeersComment #4
Wim LeersForgot the tag!
Comment #5
phenaproximaI raised a question on IRC about this --
The patch looks good to me, but it needs tests.
Comment #6
xjmThanks @Wim Leers.
These methods can go in up until the commit freeze before 8.4.0-beta1, or in a minor since there's a base class corresponding to the interface.
Comment #7
seanBA getter for the media type was missing as well. So added that as well. Also changed the tests to use these new methods instead of
$media->bundle()
/$media->label()
/$media->set('name', $value)
.Comment #9
seanBNew patch adding the default name support to
getName()
to address #5. Also fixed the tests I broke..Comment #10
Wim Leers#5's point is addressed already, so removing
. This still needs tests for #5 though.Nit: Can be simplified to
return $this->set('name', $name);
It's fine if you want to keep it like this though.
Why this change?
If it's because of the default name metadata attribute — that should have already been working in theory. But we probably missed that when we first added the
Media
entity type to core.Comment #11
seanB#10
Test for #5 is added in #9?
#10.1: True, I can change this if needed.
#10.2: We didn't have
getName()
before and want to make sure it works right? So that's why I made all the media tests usegetName()
in stead oflabel()
. Tests for the default name are only inMediaSourceTest::testDefaultName()
.Comment #12
Wim LeersD'oh, I missed that!
Then the only thing that remains is test coverage to ensure that
label()
behaves the same asgetName()
. That's easy: repeat the assertions you added intestDefaultName()
forlabel()
. To make it pass, you'll want to addThen this is RTBC.
Comment #13
phenaproximaThis oughta do it.
I changed the logic in getName() a bit, for two reasons. First, readability. Second, we shouldn't call getSource() -- which could potentially be expensive, what with plugin instantiation and all, if we don't need to.
Comment #14
bojanz CreditAttribution: bojanz at Centarro commentedWhat's the point of adding a getType() when it just duplicates ->bundle()?
I can understand getName(), but bundles are as special as it gets.
it would be like defining getMediaId() as a proxy for id(), something that no entity type does.
Comment #16
Wim LeersWhat #14 writes is exactly what I thought of too. But then I figured why:
\Drupal\node\NodeInterface::getType()
does the same.Why? Because
Node
's "bundles" are calledNodeType
s. And sinceMedia
's "bundles" are calledMediaType
s, it makes sense.Either way, I don't feel very strongly about it.
Comment #17
seanBThe NodeInterface and Node entity have a
getType()
wrapper for the type. Since we apply the same concept of Node / NodeType to Media / MediaType (and also explain it like this), it makes sense to have a getType() method for developers on media as well. The whole bundle vs type thing is already a little confusing, this makes it a little easier (although you probably run into the bundle thing anyway).I would be ok with removing it though. Just think people will expect it to be there.
Comment #18
phenaproximaPersonally, I agree with @bojanz. I don't think $node->getType() should exist either, but it obviously can't be iced until Drupal 9.
That said, I don't feel strongly about it either.
Comment #19
bojanz CreditAttribution: bojanz at Centarro commentedYeah, nodes have getType() for random historical reasons, they have always been our least standard entity type.
But adding getType() here is setting a pattern that we can't really explain (since we're not doing it for the ID).
Especially since it's a bit misnamed (it returns the type ID, not the type entity).
Comment #20
Wim LeersSounds reasonable, let's drop
MediaInterface::getType()
!Comment #21
dawehnerAren't we using
@return $this
for that?Comment #22
seanBRemoved
getType()
as per #20 and fixed #21. Also fixed the tests.For some reason this broke the tests? Don't have any time to look into it further for now, but maybe somebody has an idea?
Comment #24
Wim Leers#21 good catch — I copied that from
NodeInterface::setTitle()
which apparently still does it wrong.#22:
It failed with this:
I bet that because of the presence of the colon, it's being interpreted as a URI, and since its protocol (
media
) is not trusted, it gets stripped. Which would explain the failing test.Comment #25
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedfixed the test
Comment #26
Wim Leers#25: can you please provide an interdiff, so we can see what you changed? https://www.drupal.org/documentation/git/interdiff
Comment #27
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedinterdiff
Comment #29
Wim LeersInteresting, I can't reproduce this failure locally… I do get a similarly structured name:
media:rme72juu:8c8b8daf-b994-4db5-9430-dade1801ecf5
. But I'm running on PHP 7. Perhaps that's it? I also tried in PHP 5.6, still works. And 5.5, still works. So … IDK what's going on here.Retesting #25 in PHP 5.5+5.6+7.
Comment #31
Wim Leers😩👎
I can't reproduce this then. Can somebody else?
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedI can reproduce this (permanent failure with #25 patch, and pass test without it). But I still do not have enough knowledge to fix it :(
All what I can see so far is that the title is empty.
Before patch:
After patch:
And if replace
pageTextContains
withresponseContains
, the test also pass, and response looks like:Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedOr like this (I do not think that I found the right solution, just to test).
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedStep to reproduce: run on Windows with php 7 and drupal 8.4.x.
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedWell, two new variants (i think this is real catch). In both variants, behavior is corrected, when
getName
is returned non-empty value, but the name is left empty.It seems, we need to pay attention to this code in any case, because now the
lable()
returns a non-empty value always.Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedOpps, these were interdiffs, sorry.
Comment #39
seanB#37 Nice work!
I think v1_2897009-37.patch is better, we should not call
setName()
ingetName()
like done in the v2 patch.So I'm all for RTBCing v1 in #37. But I worked on this too much to do it. I'll ask around if someone is up for it.
Comment #40
phenaproximaFirst-class.
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for the review! Choosing the v1, we can slightly shorten the code. Tell me if we do not need to do this, and I'll reload # 37 again.
Comment #42
phenaproximaThat looks good. RTBC on the assumption that it'll pass Drupal CI.
Comment #44
phenaproximaComment #45
cburschkaRebased
Comment #46
Wim LeersComment #47
webchickNice catch. I see the "rc target" tag, though @xjm pointed out in #6 that this would either have to go in before beta1 (which has already passed) or else be postponed to the next minor. I'm a bit torn, personally... this seems like a clear oversight in the API that we fortunately caught before 8.4.0's release. Hrm.
For now, committed and pushed to 8.5.x, since there should be no contention about that. Moving back to 8.4.x and RTBC for consideration by release managers.
Comment #48
webchickComment #50
webchickOk, spoke with @xjm on Slack. Since Media is still in pre-release state, and hasn't actually been shipped in a stable release before, looks like we can indeed cherry-pick to 8.4.x as well. Whew! Awesome!
Backported to 8.4.x as well. Thanks!
Comment #52
xjm@webchick and I discussed and agreed this doesn't really need to be mentioned in the release notes; a tiny change record would be sufficient. webchick is working on one.
Also, as a reminder, only committers should add the "rc target" tag, as documented here: https://www.drupal.org/core/d8-allowed-changes#rc I previously commented on the issue's API implications back in #6. webchick and I decided it was worthwhile to do before the RC (but during it would be more iffy). Thanks!
Comment #53
webchickChange record drafted at https://www.drupal.org/node/2904992
Comment #54
LendudeUhh part of #2853359: Runtime debug statement in Views now prints out object got into this patch in #41 and got committed.
Comment #55
LendudeThis reverts the bit that shouldn't have made it in
Comment #56
webchickUh. How did I manage to do that? :\
Sending to testbot...
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedOh, this is totally my fault! Sorry. I did not try to fake the interdiff in #41, but...
Thank you @Lendude, #55 is perfect. And sorry again for unjustified trust.
Comment #59
webchickNo worries at all, @valpas, accidents happen. :) And you know, you're not really core contributor until you've broken HEAD, so! ;)
Committed and pushed #55 to 8.5.x and cherry-picked to 8.4.x. Whew! Thanks a lot for catching this so fast!
Comment #61
LendudeHaha! So true!
Thanks for the quick action @webchick.
Comment #62
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks @webchick and @Lendude, you have a kind heart!
Comment #64
colan