Child issue of #2860796: Plan for contributed modules with Media Entity API in core

CommentFileSizeAuthor
#12 interdiff-2869157-8-12.txt1.44 KBchr.fritsch
#12 2869157-12.patch45.62 KBchr.fritsch
#8 interdiff-7-8.txt23.33 KBparanojik
#8 media_entity_twitter-port-to-media-core-2869157-8.patch46.87 KBparanojik
#7 media_entity_twitter-port-to-media-core-2869157-7.patch54.15 KBkt2ssh
#5 media_entity_twitter-port-to-media-core-2869157-4.patch36.12 KBkt2ssh

Comments

naveenvalecha created an issue. See original summary.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Parent issue: » #2860796: Plan for contributed modules with Media Entity API in core

Added parent issue.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Title: [PP-2] Port to the proposed Media core module API » [PP-1] Twitter Port to the proposed Media core module API
kt2ssh’s picture

kt2ssh commented
Status: Active » Patch (to be ported)
kt2ssh’s picture

kt2ssh commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new media_entity_twitter-port-to-media-core-2869157-4.patch36.12 KB

Status: Needs review » Needs work

The last submitted patch, 5: media_entity_twitter-port-to-media-core-2869157-4.patch, failed testing. View results

kt2ssh’s picture

kt2ssh commented
Status: Needs work » Needs review
StatusFileSize
new media_entity_twitter-port-to-media-core-2869157-7.patch54.15 KB
paranojik’s picture

paranojik commented
StatusFileSize
new media_entity_twitter-port-to-media-core-2869157-8.patch46.87 KB
new interdiff-7-8.txt23.33 KB

This is an iteration on the patch from #7. I simplified the buildConfigurationForm() method - the source_field part, did a little cleanup and fixed the tests accordingly.

Status: Needs review » Needs work

The last submitted patch, 8: media_entity_twitter-port-to-media-core-2869157-8.patch, failed testing. View results

sylvainm’s picture

sylvainm commented
Status: Needs work » Reviewed & tested by the community

I tested it on a drupal 8.4 (rc2) and it works, thx :-)

I think this is a good base to start a 2.x branch of the module

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Reviewed & tested by the community » Needs work

Just two minor observations, if we can.

  1. +++ b/media_entity_twitter.install
@@ -10,6 +10,39 @@
+  $destination = \Drupal::config('media.settings')->get('icon_base_uri');

    Maybe we should take the same approach as media.install and prevent overriding if a file exists. 

  2. +++ b/src/Plugin/media/Source/Twitter.php
@@ -343,8 +356,7 @@ class Twitter extends MediaTypeBase {
+   *   The URL of the media (i.e., photo, video, etc.) associated with the tweet.

    Nit: column over 80chars.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Title: [PP-1] Twitter Port to the proposed Media core module API » Twitter Port to the proposed Media core module API
Status: Needs work » Needs review
StatusFileSize
new 2869157-12.patch45.62 KB
new interdiff-2869157-8-12.txt1.44 KB

I fixed the nits from #11 and tested the patch extensively. Everything worked quite good.

+1 RTBC

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Needs review » Reviewed & tested by the community

Looks good for me, great work everyone!

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented

Committed this patch to https://github.com/drupal-media/media_entity_twitter/tree/8.x-2.x. But seems that it will not synced back to d.o. So we have to ping a committer

slashrsm’s picture

slashrsm commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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