Support for DAT

1. +++ b/config/schema/bynder.schema.yml
   @@ -63,6 +63,15 @@ field.formatter.settings.bynder:
   +    dat_field:
   +      type: string
   +      label: 'DAT attribute field'
   +    responsive_sizes:
   +      type: string
   +      label: 'Responsive sizes field'
   +    responsive_images:
   +      type: string
   +      label: 'Responsive images field'
   

   none of these should have a field suffix in name or label. the one above is the name of a drupal field, these are not.

   For the names, maybe dat_query, dat_query_responsive. not sure if we want to prefix responsive_sizes with dat too or not. I guess just responsive_sizes for that is fine.

2. +++ b/src/Plugin/Field/FieldFormatter/BynderFormatter.php
   @@ -76,6 +79,46 @@ class BynderFormatter extends BynderFormatterBase implements ContainerFactoryPlu
   +      '#description' => $this->t('Write here the attributes that should be applied to the images. See  <a href="@dat-help">here</a> for explanation on possible values. Should start right after the "?", e.g. "io=transform:fill,width:100,height:200" If the following Responsive image fields are filled, this field is ignored.', array(
   +        '@dat-help' => 'https://support.bynder.com/hc/en-us/articles/360018559260-Dynamic-Asset-Transformations-DAT-',
   +      )),
   

   a URL placeholder should use:, I think it is also more common to use _ instead of -, if you look at examples in core?

3. +++ b/src/Plugin/Field/FieldFormatter/BynderFormatter.php
   @@ -76,6 +79,46 @@ class BynderFormatter extends BynderFormatterBase implements ContainerFactoryPlu
   +      '#title' => $this->t('Sizes for responsive images'),
   +      '#description' => $this->t('This is the string that will be fed in the "sizes" attribute of the img object.'),
   

   Maybe start the description with an explanation that if this and the next field is filled out, then an responsive image is generated?

4. +++ b/src/Plugin/Field/FieldFormatter/BynderFormatter.php
   @@ -76,6 +79,46 @@ class BynderFormatter extends BynderFormatterBase implements ContainerFactoryPlu
   +      '#title' => $this->t('The set of responsive images'),
   

   Maybe append (srcset) to the title?

5. +++ b/src/Plugin/Field/FieldFormatter/BynderFormatter.php
   @@ -76,6 +79,46 @@ class BynderFormatter extends BynderFormatterBase implements ContainerFactoryPlu
   +      '#description' => $this->t('Write here the info about the different images to be used in a responsive setting. It should have the form: "io=transform:fill,width:100,height:200 100w, io=transform:fill,width:200,height:400 200w, ...", with the "io=..." following <a href="@dat-help">these</a> guidelines, and the second argument being the width of the image. Make sure to separate each setting with ", " for parsing.', array(
   

   I'd leave out the " for parsing" and also the "Write here the info about". Just "The different images..", other than that, good description.

6. +++ b/src/Plugin/Field/FieldFormatter/BynderFormatter.php
   @@ -103,6 +146,10 @@ class BynderFormatter extends BynderFormatterBase implements ContainerFactoryPlu
    
   +    if (!empty($settings['dat_field'])) {
   +      $summary[] = $this->t('DAT attribute field: @field', ['@field' => $settings['dat_field']]);
   +    }
   

   don't forget to update the field stuff everywhere then. In the UI, maybe use "DAT configuration" or so instead of field or attribute. Because neither is accurate? (it is "Alt attribute field" because it is the name of the field that is used for the alt HTML attribute. no attribute involved here beside image src) We could also not display the derivative: DAT part and display this instead. Saves one bogus line in those often very verbose summaries.

   And for responsive, I think it's way too much info to display all the responsive image stuff, so we could just add a line that says "With responsive image configuration" or so if both are not empty.

7. +++ b/src/Plugin/Field/FieldFormatter/BynderFormatter.php
   @@ -162,4 +210,38 @@ class BynderFormatter extends BynderFormatterBase implements ContainerFactoryPlu
    
   +  /**
   +   * Generates the thumbnail_uri of the image.
   +   * @param array $settings
   +   *   Settings array.
   +   * @param $thumbnails
   +   *   Array with the uri for various image types.
   +   *
   +   * @return string
   +   *   uri of the image.
   +   */
   +  function generateThumbnailUri($settings, $thumbnails) {
   +    if ($settings['thumbnail'] == 'DAT' && isset($thumbnails['transformBaseUrl']) && !empty($settings['dat_field'])) {
   +      return $thumbnails['transformBaseUrl'] . '?' . $settings['dat_field'];
   +    }
   

   some coding standard issues here. empty line between first line and first @param. $thumbnails is missing the type (also array).

   Actually unsure if we really need to pass in settings. We could access it through $this->getSetting('thumbnail') here.

8. +++ b/src/Plugin/Field/FieldFormatter/BynderFormatter.php
   @@ -162,4 +210,38 @@ class BynderFormatter extends BynderFormatterBase implements ContainerFactoryPlu
   +  /**
   +   * Sets the img attributes in the case of responsive image
   +   */
   +  function setResponsiveAttributes(&$image, $settings, $thumbnails) {
   

   missing param and return here and . at the end of the sentence. but read next comment first and lets finalize the API before documenting it.

9. +++ b/src/Plugin/Field/FieldFormatter/BynderFormatter.php
   @@ -162,4 +210,38 @@ class BynderFormatter extends BynderFormatterBase implements ContainerFactoryPlu
   +      $new_srcset = [];
   +      foreach (explode(', ', $settings['responsive_images']) as $srcset) {
   +        $new_srcset[] = $thumbnails['transformBaseUrl'] . '?' . $srcset;
   +      }
   +      $image['#attributes']['srcset'] = implode(', ', $new_srcset);
   +    }
   +    if (isset($settings['responsive_sizes'])) {
   +      $image['#attributes']['sizes'] = $settings['responsive_sizes'];
   +    }
   

   Both settings need to be non-empty or we shouldn't add it.

   like in the other method, transformBaseUrl also needs to be set. So a single if with 3 checks.

   I'm not 100% convinced yet that the two additional methods are really worth it. We need to add quite a bit of lines for documentation and only use both of them just once. Agreee that it does make it easier to read. in the viewElements() method though. but it's a strange mix of setting some things in a method and then directly below, we set #title directly.

   I'd actually suggest to just keep both inline, but happy to discuss.

   Either way, the parsing is a bit fragile, if someone doesn't add a ", " for example, then it wouldn't work.

   I idea for using , already in the configuration was to skip parsing it completely and just do a str_replace('io=', $base_url . '?io=' of the whole string once. That does make the assumption that each configuration must begin with "io=", but that might be OK?

10. +++ b/src/Plugin/media/Source/Bynder.php
    @@ -182,6 +182,7 @@ class Bynder extends MediaSourceBase {
           'modified' => $this->t('Data modified'),
           'propertyOptions' => $this->t('Meta-property option IDs'),
    +      'DAT attributes' => $this->t('DAT attributes'),
         ];
     
         return $fields;
    @@ -207,6 +208,7 @@ class Bynder extends MediaSourceBase {
    
    @@ -207,6 +208,7 @@ class Bynder extends MediaSourceBase {
           'dateCreated',
           'dateModified',
           'propertyOptions',
    +      'datAttributes',
         ];
    

    huh, what's in those, don't see this being used? really necessary or some testing leftover?

Thanks, another round of nitpicks.

1. +++ b/src/Plugin/Field/FieldFormatter/BynderFormatter.php
   @@ -76,6 +79,46 @@ class BynderFormatter extends BynderFormatterBase implements ContainerFactoryPlu
   +      '#title' => $this->t('DAT query field'),
   +      '#description' => $this->t('Attributes that should be applied to the images. See  <a href=":dat_help">here</a> for explanation on possible values. Should start right after the "?", e.g. "io=transform:fill,width:100,height:200" If the following Responsive image fields are filled, this field is ignored.', array(
   +        ':dat_help' => 'https://support.bynder.com/hc/en-us/articles/360018559260-Dynamic-Asset-Transformations-DAT-',
   +      )),
   

   The last part about this being ignored is not correct, it's not. It's still added as the regular image on the img, which means it is the fallback image, we should write that. Also seems to be missing a . as that should be a new sentence? I'd suggest to duplicate the #states definition for 'required' as well, then it will at least be validated in the browser to be required (not 100% safe, in a frontend form we'd need server validation too, but doesn't matter here).

   Maybe put the support URL into a variable above so we can reuse it. If it changes in the future, we need to update it in one place only.

2. +++ b/src/Plugin/Field/FieldFormatter/BynderFormatter.php
   @@ -103,6 +146,18 @@ class BynderFormatter extends BynderFormatterBase implements ContainerFactoryPlu
   +    if ($settings['thumbnail'] == 'DAT') {
   +      if (!empty($settings['dat_query_responsive']) && !empty($settings['responsive_sizes'])) {
   +        $summary[] = $this->t('DAT configuration: Responsive');
   +      }
   +      elseif (!empty($settings['dat_query'])) {
   +        $summary[] = $this->t('DAT configuration: @field', ['@field' => $settings['dat_query']]);
   +      }
   +      else {
   +        $summary[] = $this->t('DAT configuration: Incomplete');
   +      }
   +    }
   

   What I meant is putting the "Derivative: @style" into the else of thumbnail == DAT, so we don't show Derivative: DAT *and* DAT configuration, only one or the other. And then first in the summary, above title/alt.

   And since dat_query is then required, although not strictly validated (#states is just browser validation, you could work around it, but it's good enough for this backend setting), we don't need the incomplete case.

   And, one last @field to replace, should be @dat or @dat_query instead.

   What I'd suggest for the responsive case is either "Responsive DAT configuration: @dat" or "DAT configuration: @dat, responsive" (@field is then the fallback but gives a hint maybe at what it is).

Tested and approved, committing.