Picture module uses config keys with a dot [#2132551]

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.

Comment	File	Size	Author
#18	2132551.17.patch	773 bytes	alexpott
#18
#14	2132551.14.patch	3.38 KB	Gábor Hojtsy
#14
#9	2132551.9.patch	3.38 KB	alexpott
#9
#9	8-9.interdiff.txt	2.92 KB	alexpott
#8	picture.mappings.user_profile.png	142.77 KB	vijaycs85
#8	2132551-diff-6-8.txt	1 KB	vijaycs85
#8	2132551-picute-mapping-8.patch	3.53 KB	vijaycs85
#8
#6	picture-config-2132551-6.patch	3.6 KB	kfritsche
#6

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

YesCT CreditAttribution: YesCT commented 10 November 2013 at 11:01

Issue tags:

+Configuration system, +Configuration schema

adding tags.

Comment #2

YesCT CreditAttribution: YesCT commented 10 November 2013 at 11:03

Comment #3

gdd

he/him

English

Portland, OR

CreditAttribution: gdd commented 10 November 2013 at 18:59

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.

Comment #4

catch

he/him

English

CreditAttribution: catch commented 14 November 2013 at 10:34

Priority:

Major

» Critical

This seems like a release blocker.

Comment #5

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 14 November 2013 at 15:28

Issue tags:

+beta blocker

It also blocks beta because it is a configuration data change

Comment #6

kfritsche

German

🇩🇪🇪🇺

CreditAttribution: kfritsche commented 25 November 2013 at 12:52

Status:

Active

» Needs review

File	Size
picture-config-2132551-6.patch	3.6 KB

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.

Comment #7

25 November 2013 at 17:58

Status:

Needs review

» Needs work

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

Comment #8

vijaycs85

London, UK

CreditAttribution: vijaycs85 commented 27 November 2013 at 19:35

Status:

Needs work

» Needs review

File	Size
2132551-picute-mapping-8.patch	3.53 KB

2132551-diff-6-8.txt	1 KB
picture.mappings.user_profile.png	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).

Comment #9

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 28 November 2013 at 13:38

File	Size
8-9.interdiff.txt	2.92 KB
2132551.9.patch	3.38 KB

+++ 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?

Comment #10

vijaycs85

London, UK

CreditAttribution: vijaycs85 commented 28 November 2013 at 13:52

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

Comment #11

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 2 December 2013 at 14:06

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

Comment #12

slashrsm CreditAttribution: slashrsm commented 5 December 2013 at 23:53

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.

Comment #13

catch

he/him

English

CreditAttribution: catch commented 6 December 2013 at 10:37

Status:

Reviewed & tested by the community

» Needs work

+++ 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?

+++ 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.

Comment #14

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 6 December 2013 at 15:41

Status:

Needs work

» Reviewed & tested by the community

File	Size
2132551.14.patch	3.38 KB

@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.

Comment #15

catch

he/him

English

CreditAttribution: catch commented 6 December 2013 at 15:50

Status:

Reviewed & tested by the community

» Fixed

Committed/pushed to 8.x, thanks!

Comment #16

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 7 December 2013 at 00:58

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.

Comment #17

vijaycs85

London, UK

CreditAttribution: vijaycs85 commented 7 December 2013 at 01:15

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!

Comment #18

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 7 December 2013 at 01:26

Status:

Needs work

» Needs review

File	Size
2132551.17.patch	773 bytes

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

Comment #19

7 December 2013 at 02:19

Status:

Needs review

» Needs work

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

Comment #20

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 7 December 2013 at 03:07

Status:

Needs work

» Needs review

18: 2132551.17.patch queued for re-testing.

Comment #21

7 December 2013 at 04:00

Status:

Needs review

» Needs work

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

Comment #22

7 December 2013 at 04:00

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

Comment #23

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 7 December 2013 at 04:29

Status:

Needs work

» Needs review

18: 2132551.17.patch queued for re-testing.

Comment #24

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 9 December 2013 at 13:23

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.