Comments

marcoscano created an issue. See original summary.

marcoscano’s picture

Assigned: marcoscano » Unassigned
Status: Active » Needs review
StatusFileSize
new25.43 KB

First attempt.

The simpletests this module was using were deliberately removed, once they depended on a trait that does not exist anymore in media core. Because of this, this issue should not get merged before #2880144: Port simple tests to browser-based tests.

marcoscano’s picture

StatusFileSize
new25.43 KB
new1.34 KB

Spotted some minor CS issues.

marcoscano’s picture

Title: Create a port of this module compatible with media in core API » Create a port of media_entity_instagram compatible with media in core API
StatusFileSize
new25.54 KB
new617 bytes

And now I realized that one of the attributes defined by MediaSourceBase::getMetadata() was missing in our plugin implementation.

marcoscano’s picture

aspilicious’s picture

This should be added in a new branch.
The old branch can be deprecated when 8.5/8.6 gets in.

marcoscano’s picture

Yes, all ports to the new API should land in a new branch. There isn't one open for this project yet though, so I just created the patch against the current -dev, we should be able to bump it to 2.x as soon as it is created.

marcoscano’s picture

StatusFileSize
new24.98 KB
new7.24 KB

Re-rolling and minor tweaks.

chr.fritsch’s picture

Status: Needs review » Needs work

Nice work Marcos,

It works pretty good. Just one thing i discovered is that the thumbnail is not generated.

And maybe we should ship some default configuration, like we do in core now.

bkosborne’s picture

  1. +++ b/media_entity_instagram.info.yml
    @@ -4,4 +4,5 @@ type: module
    +  - drupal:media
    +  - drupal:system (>= 8.4.0)
    

    Why not just drupal:media (>= 8.4.0)

  2. +++ b/media_entity_instagram.install
    @@ -9,7 +9,13 @@
    +  foreach ($files as $file) {
    +    file_unmanaged_copy($file->uri, $destination, FILE_EXISTS_ERROR);
    +  }
    

    See media.install for their example. There is another if statement in there that ensures we don't overwrite an icon that may already exist.

  3. +++ b/src/Plugin/media/Source/Instagram.php
    @@ -15,20 +16,15 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
    + * @MediaSource(
      *   id = "instagram",
      *   label = @Translation("Instagram"),
    - *   description = @Translation("Provides business logic and metadata for Instagram.")
    + *   description = @Translation("Provides business logic and metadata for Instagram."),
    + *   allowed_field_types = {"string", "string_long", "link"},
    + *   default_thumbnail_filename = "instagram.png"
      * )
    

    does link make sense here?

marcoscano’s picture

Status: Needs work » Needs review
StatusFileSize
new29.87 KB
new6.54 KB

This addresses almost all feedback from #9 and #10.

Re: #10.3:
We had link earlier, so in the port I just kept what we had in the previous version.

@chr.fritsch Re: the thumbnail generation, I see that the local icon is being used. Do you mean to fetch a remote thumbnail instead? Does this work with the 1.x branch?

marcoscano’s picture

StatusFileSize
new29.87 KB
new601 bytes

Thisi should fix the remote thumbnail generation.

marcoscano’s picture

StatusFileSize
new25.2 KB
new4.69 KB

After discussion in the sprint at DrupalCon, it seems to be best to have no default configuration at all shipped with the contrib providers.

chr.fritsch’s picture

Status: Needs review » Fixed

Thank you everyone

Status: Fixed » Closed (fixed)

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