Updated: Comment #N

Problem/Motivation

The picture module saves configuration files with keys with a dot in them. For example:

id: mapping_one
uuid: 2638a632-9d17-4aa3-b204-4f81e8976db8
label: 'Mapping One'
mappings:
  custom.user.small:
    1x: thumbnail
    2x: ''
  custom.user.medium:
    1x: medium
    2x: ''
  custom.user.large:
    1x: large
    2x: ''
breakpointGroup: atestset
status: true
langcode: en

The use of the breakpoint id, for example custom.user.small does not work out for config because a dot in a config key has a special meaning - the next level in the array. So if you did \Drupal::config('picture.mappings.someentity.yml')-get('mappings.custom.user.small.1x') it would not return a value of thumbnail. This would expect the format to be something like:

id: mapping_one
uuid: 2638a632-9d17-4aa3-b204-4f81e8976db8
label: 'Mapping One'
mappings:
  custom:
    user:
      small:
        1x: thumbnail
        2x: ''
      medium:
        1x: medium
        2x: ''
      large:
        1x: large
        2x: ''
breakpointGroup: atestset
status: true
langcode: en

Additionally dots inside config key names completely break config schemas.

Proposed resolution

We have two possible solutions:

  • Make the picture mapping config entity produce configuration as described in the second YAML structure above
  • Translate the breakpoint id to not contain dots

Remaining tasks

  • Agree solution
  • Create patch
  • Test
  • Review

User interface changes

None

API changes

Probable config schema change and maybe changes to the picture mapping config entity.

Files: 
CommentFileSizeAuthor
#18 2132551.17.patch773 bytesalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,337 pass(es). View
#14 2132551.14.patch3.38 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,269 pass(es). View
#9 2132551.9.patch3.38 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,003 pass(es). View
#9 8-9.interdiff.txt2.92 KBalexpott
#8 picture.mappings.user_profile.png142.77 KBvijaycs85
#8 2132551-diff-6-8.txt1 KBvijaycs85
#8 2132551-picute-mapping-8.patch3.53 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,990 pass(es). View
#6 picture-config-2132551-6.patch3.6 KBkfritsche
FAILED: [[SimpleTest]]: [MySQL] 58,052 pass(es), 220 fail(s), and 10 exception(s). View

Comments

YesCT’s picture

adding tags.

YesCT’s picture

heyrocker’s picture

I vote for "Make the picture mapping config entity produce configuration as described in the second YAML structure above". This seems to be more in line with how everyone else is doing similar things.

catch’s picture

Priority: Major » Critical

This seems like a release blocker.

alexpott’s picture

Issue tags: +beta blocker

It also blocks beta because it is a configuration data change

kfritsche’s picture

Status: Active » Needs review
FileSize
3.6 KB
FAILED: [[SimpleTest]]: [MySQL] 58,052 pass(es), 220 fail(s), and 10 exception(s). View

The problem I see when changing the schema here is, that this "custom.user.small" is the id for the breakpoint. This is needed in example in the formatter for getBreakpointById.
Changing the breakpoint id, somehow feels not right as splitting with (dot) is something very common and do exceptions only for this, seems just wrong.
But as we need the ids, the best I could think of was changing it afterwards again to the current format, but save it splitted for the config.
So in conclusion its a starting point, but somehow I'm not sure if it is the right way to do it.

Status: Needs review » Needs work

The last submitted patch, 6: picture-config-2132551-6.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,990 pass(es). View
1 KB
142.77 KB

Just did a manual test and seems the mappings are getting saved as per the proposed solution #1 (in issue summary). However there are some minor change in schema which is fixed and validated with config inspector (ref: picture.mappings.user_profile.png).

alexpott’s picture

FileSize
2.92 KB
3.38 KB
PASSED: [[SimpleTest]]: [MySQL] 59,003 pass(es). View
+++ b/core/modules/picture/lib/Drupal/picture/Entity/PictureMapping.php
@@ -95,6 +95,23 @@ public function save() {
+    foreach ($loaded_mappings as $breakpoint_id => $mapping) {
+      $map = &$this->mappings;
+      foreach (explode('.', $breakpoint_id) as $id) {
+        if (!is_array($map)) {
+          $map = array();
+        }
+        elseif (!isset($map[$id])) {
+          $map[$id] = array();
+        }
+        $map = &$map[$id];
+      }
+      $map = $mapping;
+    }

This use of references seems overly complex and hard to understand.

How about something like the patch attached?

vijaycs85’s picture

+1 to #9, if we are sure they key is fixed (i.e. 3 part).

alexpott’s picture

#10 well if this changes the schema is wrong too :) - it has to be fixed.

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

I agree that #9 looks much nicer. I tested (created, edited mapping) and it works as expected. Looks good to me.

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/picture/config/schema/picture.schema.yml
    @@ -18,10 +18,16 @@ picture.mappings.*:
    +            - type: sequence
    +              label: 'Source'
    +              sequence:
    +                - type: sequence
    +                  label: 'Machine name'
    +                  sequence:
    +                    - type: string
    +                      label: 'Image style'
    

    I'm finding the number of 'sequence' in the schema a bit confusing, do we really have this many layers named the same thing?

  2. +++ b/core/modules/picture/lib/Drupal/picture/Entity/PictureMapping.php
    @@ -129,10 +139,14 @@ protected function loadAllMappings() {
    +        // Get the components of the breakpoint id to match the format of the
    

    ID.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.38 KB
PASSED: [[SimpleTest]]: [MySQL] 59,269 pass(es). View

@catch: re the sequence concern, the issue summary states that this change is made:

BEFORE:

mappings:
  custom.user.small:
    1x: thumbnail
    2x: ''

AFTER:

mappings:
  custom:
    user:
      small:
        1x: thumbnail
        2x: ''

So now it is a three level nesting storage. They are not "names the same thing", sequence is their type, not their name :) They have different names (labels) on different levels.

I've updated the patch for (2) to change "id" to "ID". I think this is back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

alexpott’s picture

Status: Fixed » Needs work

Unfortunately the schema is still not correct. We missing a whole level.

mappings:
  module:
    toolbar:
      narrow:
        1x: thumbnail
      standard:
        1x: medium
      wide:
        1x: large

A current example YAML file

    mappings:
      type: sequence
      label: 'Mappings'
      sequence:
        - type: sequence
          label: 'Source type'
          sequence:
            - type: sequence
              label: 'Source'
              sequence:
                - type: sequence
                  label: 'Machine name'
                  sequence:
                    - type: string
                      label: 'Image style'

The current schema.

    mappings:
      type: sequence
      label: 'Mappings'
      sequence:
        - type: sequence
          label: 'Source type'
          sequence:
            - type: sequence
              label: 'Source'
              sequence:
                - type: sequence
                  label: 'Machine name'
                  sequence:
                    - type: sequence
                      label: 'Multiplier'
                      sequence:
                        - type: string
                          label: 'Image style'

The correct schema.

vijaycs85’s picture

I guess not. Please refer inspector screenshot in #8. We don't get the keys as they are dynamic and we skip them in schema.

So in example, module, narrow and 1x are OUT!

alexpott’s picture

Status: Needs work » Needs review
FileSize
773 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,337 pass(es). View

Here a patch to fix the schema - wow do we need #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct to land.

Whilst looking at this issue I was wondering if maybe we made the config file look like this it'd be simpler.

mappings:
  -
    breakpoint: module.toolbar.narrow
    multipliers:
      1x: thumbnail
  -
    breakpoint: module.toolbar.standard
    multipliers:
      1x: medium
  -
    breakpoint: module.toolbar.wide
    multipliers:
      1x: large

Status: Needs review » Needs work

The last submitted patch, 18: 2132551.17.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

18: 2132551.17.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 18: 2132551.17.patch, failed testing.

The last submitted patch, 18: 2132551.17.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

18: 2132551.17.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@alexpott: looking at this data, I don't get your patch:

mappings:
  module:
    toolbar:
      narrow:
        1x: thumbnail
      standard:
        1x: medium
      wide:
        1x: large

Multipliers are 1x, 2x, etc. But the values under them are NOT sequences, they are single string values for each multiplier in this file. So your proposed schema change to have a sequence of strings be the structure under the multipliers does not click for me with the data structure.

alexpott’s picture

Status: Needs work » Fixed

#24 you are correct. Doh!

S

orry for the noise.

Status: Fixed » Closed (fixed)

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

Eli-T’s picture

Component: picture.module » responsive_image.module