Expand ResponsiveImageMapping with methods

If this is something that's still needed, maybe we should check with [attiks] on what's needed to make these changes?

Added getters and setters for the pictureMapping entity.

Hi @jeroen12345

Some initial comments on your patch

1. There is no need for a getId() or getUuid() methods - id() and uuid() is provided by the Entity class.

    /**
     * @param array $mappings
     */
    public function setMappings($mappings)
    {
        $this->mappings = $mappings;
    }

2. These methods need to be declared in the appropriate interface first (i.e. core/modules/picture/lib/Drupal/picture/PictureMappingInterface.php)

3. The Infodoc comments will be placed in the interface, comment standards require a short description, followed by parameter declarations followed by return values. i.e...

/**
 * Return a list of the mappings.
 *
 * @return array
 *   An array of [mapping] types.
 */
  public function getMappings();

Note, I've not looked at the behaviour of the module so don't know what should be returned in this case.

5. Coding standards - the opening brace should be on the same line as the function declaration. i.e. ...

public function getMappings {
 // code...
}

6. Setters need to return the $this object to allow chaining of functions. There is also a specific convention around the setting of values (see some of the other issues in this meta issue for details).

    /**
     *  {@inheritdoc}
     */
    public function setMappings($mappings) {
        $this->set('mappings', $mappings);
        return $this;
    }

Here is an updated patch following on from ceng's comments

1. +++ b/core/modules/picture/lib/Drupal/picture/Entity/PictureMapping.php
   @@ -93,8 +93,8 @@ public function __construct(array $values, $entity_type) {
   +    if ($this->getBreakpointGroup() && is_object($this->getBreakpointGroup())) {
   +      $this->setBreakpointGroup($this->getBreakpointGroup()->id());
   
   @@ -116,9 +116,9 @@ public function createDuplicate() {
   +    if ($this->getBreakpointGroup()) {
   +      $breakpoint_group = entity_load('breakpoint_group', $this->getBreakpointGroup());
   

   So this code calls getBreakpointGroup() multiple times, ... why not just use if ($breakpoint_group = $this->getBreakpointGroup()) {

2. +++ b/core/modules/picture/lib/Drupal/picture/Entity/PictureMapping.php
   @@ -165,4 +166,67 @@ public function hasMappings() {
   +   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::getExportProperties();
   

   Let's use {@inheritdoc}

3. +++ b/core/modules/picture/lib/Drupal/picture/PictureMappingInterface.php
   @@ -19,4 +19,40 @@
    
   +  /**
   +   * @param string $label
   +   *   The label the picture mapping will use.
   +   */
   +  public function setLabel($label);
   ...
   +  /**
   +   * @param array $mappings
   +   *   The mappings the picture mapping will be set with.
   +   */
   +  public function setMappings($mappings);
   ...
   +  /**
   +   * @param BreakpointGroup $breakpointGroup
   +   *   An array of breakpoints.
   +   */
   +  public function setBreakpointGroup($breakpointGroup);
   

   These methods need to to document the return value as well as have a online description.

4. +++ b/core/modules/picture/lib/Drupal/picture/PictureMappingInterface.php
   @@ -19,4 +19,40 @@
   +
   +  /**
   +   * @return string
   +   *   The picture mapping's label.
   +   */
   +  public function getLabel();
   +
   ...
   +  /**
   +   * @return array
   +   *   The picture mappings's.
   +   */
   +  public function getMappings();
   ...
   +
   +  /**
   +   * @return BreakpointGroup
   +   *   The picture mappings breakpoint group.
   +   */
   +  public function getBreakpointGroup();
   

   These methods need a one-line description.

5. +++ b/core/modules/picture/lib/Drupal/picture/Plugin/Field/FieldFormatter/PictureFormatter.php
   @@ -127,14 +127,14 @@ public function viewElements(FieldItemListInterface $items) {
   +          if (isset($picture_mapping->getBreakpointGroup()->breakpoints[$breakpoint_name])) {
   +            $breakpoint = $picture_mapping->getBreakpointGroup()->breakpoints[$breakpoint_name];
   

   We could save a call to getBreakpointGroup as well.

6. +++ b/core/modules/picture/lib/Drupal/picture/Tests/PictureFieldDisplayTest.php
   @@ -88,9 +88,11 @@ public function setUp() {
   +    $mappings['custom.user.small']['1x'] = 'thumbnail';
   

   Just a general question ... is there a reason why we have small and thumbnail (I know this is out of scope).

I've made changes from previous comments so here is a new slightly modified patch.

Looks good! I've got a few comments, mostly about the documentation standards:

1. +++ b/core/modules/picture/lib/Drupal/picture/PictureMappingInterface.php
   @@ -19,4 +19,61 @@
   +   * @return \Drupal\picture\Entity\PictureMappingInterface
   +   *  The modified picture mapping.
   

   If a method returns the object it was called on, we document this with @return self. This needs to be changed multiple times.

2. +++ b/core/modules/picture/lib/Drupal/picture/PictureMappingInterface.php
   @@ -19,4 +19,61 @@
   +  public function getLabel();
   

   We use EntityInterface::label() for this.

3. +++ b/core/modules/picture/lib/Drupal/picture/PictureMappingInterface.php
   @@ -19,4 +19,61 @@
   +   * @param array $mappings
   +   *   The mappings the picture mapping will be set with.
   

   The array's keys and values need to be documented as well.

4. +++ b/core/modules/picture/lib/Drupal/picture/PictureMappingInterface.php
   @@ -19,4 +19,61 @@
   +   *   The picture mappings's.
   

   Just mappings.

5. +++ b/core/modules/picture/lib/Drupal/picture/PictureMappingInterface.php
   @@ -19,4 +19,61 @@
   +   *   An array of breakpoints.
   

   The parameter type indicates a class, but its description talks about an array. Which one is it? If it's a class, the type must be namespaced.

6. +++ b/core/modules/picture/lib/Drupal/picture/PictureMappingInterface.php
   @@ -19,4 +19,61 @@
   +   * @return BreakpointGroup
   

   The class name of the return value needs to be namespaced in the docblock. This needs to be changed multiple times.

Thanks for your comments @Xano. I have been working on this recently so hope to have a patch soon.

Unassigning and tagging so folks can work on the doc updates mentioned in #8.

@mike.davis if you are still working on this please feel free to reassign.

Thanks @Cottser, I am still working on this - been a bit delayed with the holidays and working through issues that have come up after an update :).

Typical, spent ages scratching my head about a silly merge issue. OK I have finally managed to get an updated patch for this now :)

Just a note that it looks like this patch still applies if somebody more knowledgeable than me wants to review it.

Hiding older patches.

I updated some documentation and type hints. Someone with more knowledge of the internals than me must document the mapping array structure for \Drupal\picture\PictureMappingInterface::setMappings(), \Drupal\picture\PictureMappingInterface::getMappings(), and probably for \Drupal\picture\Entity\PictureMapping::mappings as well.

FYI: picture is renamed to responsive_image so this needs a reroll once #2124377-74: Rename "Picture" module to "Responsive Image" module is committed

Note there is a problem in that getMappings() and setBreakpointGroup() are inconsistent in what they return.

getMappings() can return an array in the format

array(
  'theme.bartik.mobile' => array(
    '1.x' => 'thumbnail',
  ),
  'theme.bartik.narrow' => array(
    '1.x' => 'medium',
  ),
  'theme.bartik.wide' => array(
    '1.x' => 'large',
  ),
);

or

array(
  'theme' => array(
    'bartik' => array(
      'mobile' => array(
        '1x' => 'thumbnail',
      ),
      'narrow' => array(
        '1x' => 'medium',
      ),
      'wide' => array(
        '1x' => 'large',
      ),
    ),
  ),    
);

Similarly, getBreakpointGroup can either be a string specifying the breakpoint group, or an object of class \Drupal\breakpoint\Entity\BreakpointGroup.

These problems weren't caused by this issue, although I believe the added @return value in the docblock for \Drupal\breakpoint\Entity\BreakpointGroup::getBreakpointGroup() is wrong whilst the method is inconsistent in return type. However, the issue will be easier to fix once this patch is implemented.

Thanks @Eli-T for talking about #18 and open an issue to address this #2219329: ResponsiveImageMapping::mappings and ResponsiveImageMapping::breakpointGroup properties have inconsistent return structure and type respectively - it's super awkward atm.

Reroll the patch #15 as picture module has been changed to responsive _image module

Looks good to me, once this is committed we can address the remarks from #18

+++ b/core/modules/responsive_image/lib/Drupal/responsive_image/Entity/ResponsiveImageMapping.php
@@ -168,4 +170,52 @@ public function hasMappings() {
+  public function setMappings(array $mappings) {
...
+  public function getMappings() {
...
+  public function setBreakpointGroup($breakpoint_group) {
...
+  public function getBreakpointGroup() {

If we are going to add setters and getters can we make the mappings and breakpointGroup properties protected? This will necessitate changing all the code that accesses the public properties but this is a good thing.

Also needs changes following #2219925: Rename ConfigEntityInterface::getExportProperties() to toArray()

Protected $mappings and $breakpointGroup, and renamed getExportProperties() to toArray().

Patch in #33 looks good to go. Address the issues raised in #30. Tests pass.

Fix unit test fail in ResponsiveImageMappingEntityTest::testCalculateDependencies().

Note there are leftover references to Picture module in the unit tests that should have been picked up under #2124377: Rename "Picture" module to "Responsive Image" module but I don't intend fixing those under this issue.

There isn't a picture module more, so we can't put in a patch that refers to the picture module. The tests need to be modified to refer to the responsive image module.

#2225677: Fix naming in ResponsiveImageMappingEntityTest following module rename from Picture to Responsive Image has been raised to fix the naming issues in the unit tests. These do not prevent the tests from being run and weren't caused by this issue.

Fair enough!

… and fix in #36 means it passes unit tests, too!

Committed ed5d055 and pushed to 8.x. Thanks!

Can existing change notices be checked.

Sorry for arriving here after the commit, but...

+++ b/core/modules/responsive_image/lib/Drupal/responsive_image/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
@@ -129,16 +129,16 @@ public function viewElements(FieldItemListInterface $items) {
-          $breakpoint = $responsive_image_mapping->breakpointGroup->getBreakpointById($breakpoint_name);
-          if ($breakpoint) {
+          $breakpointGroup = $responsive_image_mapping->getBreakpointGroup()->getBreakpointById($breakpoint_name);
+          if ($breakpointGroup) {
             // Determine the enabled multipliers.

Assuming that getBreakpointById() returns a single breakpoint, as the docs in BreakpointGroupInterface say, why is the variable called $breakpointGroup? And if it is a group, shouldn't the variable be called $breakpoint_group?

Patch to address #44.

No interdiff as previous patch was committed.

Patch applies and works as expected.

46: expand-responsive_image-mapping-with-methods-2030653-46.patch queued for re-testing.

RTBC, as the tests failed because of a segmentation fault on the server.

Committed 2fe3785 and pushed to 8.x. Thanks!