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

CommentFileSizeAuthor
#55 2897009-55.patch1.9 KBLendude
#45 drupal-2897009-45.patch14.13 KBcburschka
#41 interdiff-v1_37-41.txt681 bytesAnonymous (not verified)
#41 2897009-41.patch14.2 KBAnonymous (not verified)
#37 v2_interdiff-25-37.txt688 bytesAnonymous (not verified)
#37 v1_interdiff-25-37.txt934 bytesAnonymous (not verified)
#37 v2_2897009-37.patch11.68 KBAnonymous (not verified)
#37 v1_2897009-37.patch12.39 KBAnonymous (not verified)
#35 v2_2897009-35.patch688 bytesAnonymous (not verified)
#35 v1_2897009-35.patch934 bytesAnonymous (not verified)
#33 interdiff-25-33.txt1000 bytesAnonymous (not verified)
#33 2897009-33.patch12.66 KBAnonymous (not verified)
#32 interdiff-25-32.txt963 bytesAnonymous (not verified)
#32 2897009-32.patch11.69 KBAnonymous (not verified)
#27 interdiff-22-25.txt418 bytesharsha012
#25 2897009-24.patch11.69 KBharsha012
#22 interdiff-13-22.txt5.44 KBseanB
#22 2897009-22.patch11.64 KBseanB
#13 interdiff-2897009-9-13.txt3.22 KBphenaproxima
#13 2897009-13.patch12.73 KBphenaproxima
#9 interdiff-7-9.txt4.35 KBseanB
#9 2897009-9.patch12.22 KBseanB
#7 interdiff-3-7.txt12.65 KBseanB
#7 2897009-7.patch14.04 KBseanB
#3 2897009-3.patch1.68 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.68 KB
18:24:27 <WimLeers> seanB: marcoscano|afk phenaproxima some low-hanging fruit? https://www.drupal.org/node/2897009
18:24:29 <drupalbot> https://www.drupal.org/node/2897009 => Media(Interface) is missing setName(), getName(), perhaps more? #2897009: MediaInterface is missing setName() and getName() => 0 comments, 1 IRC mention
18:25:16 <phenaproxima> WimLeers: Yes, it absolutely is.
18:25:26 <phenaproxima> WimLeers: I probably do not have time today to address that.
18:25:37 <phenaproxima> WimLeers: But I will absolutely RTBC if you have a quick patch
18:27:13 <WimLeers> phenaproxima: ok, was just wondering if I was forgetting something
Wim Leers’s picture

Issue tags: +Media Initiative

Forgot the tag!

phenaproxima’s picture

Issue tags: +Needs followup, +Needs tests

I raised a question on IRC about this --

<phenaproxima> WimLeers: The one question I have about getName()/setName() is that most media sources define a default_name metadata. Should getName() account for that?
<WimLeers> phenaproxima: Well, default naming woudl result in that default name being saved, which means getName() will still work fine?
<phenaproxima> WimLeers: And if it's not yet saved...?
<WimLeers> phenaproxima: how often are you handling unsaved Media items and calling getName() on them?
<phenaproxima> WimLeers: It happens in Lightning for sure.
<WimLeers> phenaproxima: We can fix that later IMHO
what matters most ATM is API/interface stability
<phenaproxima> WimLeers: If you open a follow-up for that, I will RTBC.

The patch looks good to me, but it needs tests.

xjm’s picture

Thanks @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.

seanB’s picture

A 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).

Status: Needs review » Needs work

The last submitted patch, 7: 2897009-7.patch, failed testing. View results

seanB’s picture

Status: Needs work » Needs review
FileSize
12.22 KB
4.35 KB

New patch adding the default name support to getName() to address #5. Also fixed the tests I broke..

Wim Leers’s picture

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

#5's point is addressed already, so removing Needs follow-up. This still needs tests for #5 though.

  1. +++ b/core/modules/media/src/Entity/Media.php
    @@ -88,6 +88,29 @@ class Media extends EditorialContentEntityBase implements MediaInterface {
    +    $this->set('name', $name);
    +    return $this;
    

    Nit: Can be simplified to return $this->set('name', $name);

    It's fine if you want to keep it like this though.

  2. +++ b/core/modules/media/tests/src/Functional/MediaRevisionTest.php
    @@ -65,7 +65,7 @@ public function testRevisions() {
    -    $assert->pageTextContains($media->label());
    +    $assert->pageTextContains($media->getName());
    
    +++ b/core/modules/media/tests/src/Functional/MediaUiFunctionalTest.php
    @@ -67,7 +67,7 @@ public function testMediaWithOnlyOneMediaType() {
    -    $this->assertEquals($media->label(), $media_name);
    +    $this->assertEquals($media->getName(), $media_name);
    

    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.

seanB’s picture

#10

Test for #5 is added in #9?

+++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
@@ -24,9 +24,10 @@ public function testDefaultName() {

-    $this->assertEquals('media:' . $media->bundle() . ':' . $media->uuid(), $media_source->getMetadata($media, 'default_name'), 'Value of the default name metadata attribute does not look correct.');
+    $this->assertEquals('media:' . $media->getType() . ':' . $media->uuid(), $media_source->getMetadata($media, 'default_name'), 'Value of the default name metadata attribute does not look correct.');
+    $this->assertEquals('media:' . $media->getType() . ':' . $media->uuid(), $media->getName(), 'Default name was not used correctly by getName().');
...
+    $this->assertEquals('media:' . $media->getType() . ':' . $media->uuid(), $media->getName(), 'Default name was not saved correctly.');

#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 use getName() in stead of label(). Tests for the default name are only in MediaSourceTest::testDefaultName().

Wim Leers’s picture

Issue tags: -Needs tests

D'oh, I missed that!

Then the only thing that remains is test coverage to ensure that label() behaves the same as getName(). That's easy: repeat the assertions you added in testDefaultName()for label(). To make it pass, you'll want to add

public function label() {
  return $this->getName();
}

Then this is RTBC.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
12.73 KB
3.22 KB

This 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.

bojanz’s picture

   /**
    * {@inheritdoc}
    */
+  public function getType() {
+    return $this->bundle();
+  }

What'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.

Status: Needs review » Needs work

The last submitted patch, 13: 2897009-13.patch, failed testing. View results

Wim Leers’s picture

What #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 called NodeTypes. And since Media's "bundles" are called MediaTypes, it makes sense.

Either way, I don't feel very strongly about it.

seanB’s picture

The 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.

phenaproxima’s picture

Personally, 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.

bojanz’s picture

Yeah, 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).

Wim Leers’s picture

Sounds reasonable, let's drop MediaInterface::getType()!

dawehner’s picture

+++ b/core/modules/media/src/MediaInterface.php
@@ -14,6 +14,33 @@
+   * @return \Drupal\media\MediaInterface

Aren't we using @return $this for that?

seanB’s picture

Status: Needs work » Needs review
FileSize
11.64 KB
5.44 KB

Removed getType() as per #20 and fixed #21. Also fixed the tests.

+++ b/core/modules/media/src/Entity/Media.php
@@ -88,6 +88,42 @@ class Media extends EditorialContentEntityBase implements MediaInterface {
+  /**
+   * {@inheritdoc}
+   */
+  public function label() {
+    return $this->getName();
+  }

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?

Status: Needs review » Needs work

The last submitted patch, 22: 2897009-22.patch, failed testing. View results

Wim Leers’s picture

#21 good catch — I copied that from NodeInterface::setTitle() which apparently still does it wrong.

#22:

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?

It failed with this:

1) Drupal\Tests\media\Functional\MediaUiFunctionalTest::testMediaWithMultipleMediaTypes
Behat\Mink\Exception\ResponseTextException: The text "media:vgc1w7ie:7c7d9076-91ca-49c5-aeb8-1a2e51c642a6" was not found anywhere in the text of the current page.

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.

harsha012’s picture

Wim Leers’s picture

#25: can you please provide an interdiff, so we can see what you changed? https://www.drupal.org/documentation/git/interdiff

harsha012’s picture

FileSize
418 bytes

interdiff

Status: Needs review » Needs work

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

Wim Leers’s picture

Status: Needs work » Needs review

Interesting, 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.

Status: Needs review » Needs work

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

Wim Leers’s picture

😩👎

I can't reproduce this then. Can somebody else?

Anonymous’s picture

Status: Needs work » Needs review
FileSize
11.69 KB
963 bytes

I 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:

media:z7h6vjgv:8caaae50-bb6b-46d1-807f-48d4ef297dae | Drupal @import ... Skip to main content v092KrzQ Fri, 07/28/2017 - 08:06 Primary tabs View(active tab) Edit Delete {"path":{"baseUrl":"\/","scriptPath":null,"pathPrefix":"","currentPath":"media\/1","currentPathIsAdmin":false,"isFront":false,"currentLanguage":"en"},"pluralDelimiter":"\u0003","user":{"uid":"2","permissionsHash":"3307e6b2249536199ba3712875491105dba16837394464dc0ac7fce809e2483b"}}

After patch:

Drupal @import ... Skip to main content qfTV19DD Fri, 07/28/2017 - 07:29 Primary tabs View(active tab) Edit Delete {"path":{"baseUrl":"\/","scriptPath":null,"pathPrefix":"","currentPath":"media\/1","currentPathIsAdmin":false,"isFront":false,"currentLanguage":"en"},"pluralDelimiter":"\u0003","user":{"uid":"2","permissionsHash":"121737348a151d2e43518b7c926b6a6a0e56c94823ad4c017b516bcb35d28e78"}}

And if replace pageTextContains with responseContains, the test also pass, and response looks like:

<!DOCTYPE html>
<html lang="en" dir="ltr">
...
    <title>Drupal</title>
...
            <div class="field field--name-thumbnail field--type-image field--label-hidden field__item">  <img src="/sites/simpletest/19951676/files/styles/thumbnail/public/media-icons/generic/generic.png?itok=VzIGsWZn" width="100" height="100" alt="Thumbnail" title="media:mmgcjus6:f76fdda0-53ce-40c8-a27d-794a3858513a" class="image-style-thumbnail" />
...
  </body>
</html>
Anonymous’s picture

Or like this (I do not think that I found the right solution, just to test).

Anonymous’s picture

Step to reproduce: run on Windows with php 7 and drupal 8.4.x.

Anonymous’s picture

Well, 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.

-        // Try to set a default name for this media item if no label is
-        // provided.
-        if (!$translation->label()) {
-          $translation->set('name', $media_source->getMetadata($translation, $media_source->getPluginDefinition()['default_name_metadata_attribute']));

It seems, we need to pay attention to this code in any case, because now the lable() returns a non-empty value always.

Status: Needs review » Needs work

The last submitted patch, 35: v2_2897009-35.patch, failed testing. View results

Anonymous’s picture

Opps, these were interdiffs, sorry.

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.

seanB’s picture

#37 Nice work!
I think v1_2897009-37.patch is better, we should not call setName() in getName() 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.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

First-class.

Anonymous’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
14.2 KB
681 bytes

Thanks 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.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

That looks good. RTBC on the assumption that it'll pass Drupal CI.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: 2897009-41.patch, failed testing. View results

phenaproxima’s picture

Issue tags: +Needs reroll
cburschka’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
14.13 KB

Rebased

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +rc target, +API change
webchick’s picture

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

Nice 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.

webchick’s picture

Title: Media(Interface) is missing setName(), getName(), perhaps more? » MediaInterface is missing setName() and getName()

  • webchick committed 48f9801 on 8.5.x
    Issue #2897009 by vaplas, seanB, phenaproxima, harsha012, Wim Leers,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.4.0 release notes

Ok, 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!

  • webchick committed 6b853f1 on 8.4.x
    Issue #2897009 by vaplas, seanB, phenaproxima, harsha012, Wim Leers,...
xjm’s picture

@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!

webchick’s picture

Change record drafted at https://www.drupal.org/node/2904992

Lendude’s picture

Uhh part of #2853359: Runtime debug statement in Views now prints out object got into this patch in #41 and got committed.

Lendude’s picture

This reverts the bit that shouldn't have made it in

webchick’s picture

Status: Fixed » Needs review

Uh. How did I manage to do that? :\

Sending to testbot...

Anonymous’s picture

Oh, 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.

  • webchick committed c8daa8d on 8.5.x
    Issue #2897009 follow-up by Lendude: Remove stray code accidentally...
webchick’s picture

Status: Needs review » Fixed

No 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!

  • webchick committed 3cec1d9 on 8.4.x
    Issue #2897009 follow-up by Lendude: Remove stray code accidentally...
Lendude’s picture

And you know, you're not really core contributor until you've broken HEAD

Haha! So true!

Thanks for the quick action @webchick.

Anonymous’s picture

Thanks @webchick and @Lendude, you have a kind heart!

Status: Fixed » Closed (fixed)

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

colan’s picture