Updated: Comment #19

Problem/Motivation

Saved config files in files/config/active do not match default config files in core user module directory.

Proposed resolution

Use single quotes on config values that have more than one word. Do *not* use them for single word values. This is consistent with the output generated in files/config/active.

Remaining tasks

Review last patch.

This is a sub-issue of #1938580: [META] Make active config save format match the default yml file (order and quotes).

Original report by @vijaycs85

Files need to be fixed in this
user.role.anonymous.yml
user.role.authenticated.yml

Files fixed already
user.flood.yml
user.mail.yml
user.settings.yml

Files: 
CommentFileSizeAuthor
#51 1942178-config-schema-user-51.patch874 bytesohthehugemanatee
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,004 pass(es). View
#44 interdiff.txt2.58 KBohthehugemanatee
#44 1942178-config-schema-user-43.patch806 bytesohthehugemanatee
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,615 pass(es). View
#39 1942178-config-schema-user-39.patch3.46 KBohthehugemanatee
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,692 pass(es). View
#34 1942178-config-schema-user-34.patch4.06 KBricardoamaro
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,794 pass(es), 21 fail(s), and 0 exception(s). View
#28 1942178-config-schema-user-28.patch9.44 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 59,497 pass(es), 6 fail(s), and 0 exception(s). View
#28 1942178-diff-23-28.txt9.04 KBvijaycs85
#16 1942178-user-config-fix-16.patch2.25 KBmortona2k
PASSED: [[SimpleTest]]: [MySQL] 58,577 pass(es). View
#13 1942178-user-config-fix-13.patch2.23 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 57,698 pass(es). View
#13 interdiff.txt1.2 KBmtift
#11 1942178-user-config-fix-11.patch1.11 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 57,736 pass(es). View
#11 interdiff.txt1.12 KBmtift
#10 1942178-user-config-fix-10.patch2.23 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,033 pass(es). View
#6 1942178-user-config-fix-6.patch3.11 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 54,553 pass(es), 17 fail(s), and 5 exception(s). View
#6 interdiff-4-6.txt2.37 KBYesCT
#4 1942178-user-config-fix-4.patch1.15 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 54,462 pass(es). View
#1 1942178-user-config-fix-1.patch740 bytesvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1942178-user-config-fix-1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

vijaycs85’s picture

Issue summary: View changes

Updated issue summary.

vijaycs85’s picture

Status: Active » Needs review
FileSize
740 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1942178-user-config-fix-1.patch. Unable to apply patch. See the log in the details link for more information. View

Issuing patch for user.role.anonymous.yml and user.role.authenticated.yml.

P.S: I can see user.settings.admin_role and user.settings.register are changing after installation without update user settings pages manually. But I hope that is working as design or getting changed depending on profile we use to install

module/user.settings.yml

admin_role: ''
register: visitors

active-folder/user.settings.yml

admin_role: administrator
register: visitors_admin_approval

May be we need to change admin_role: '' to admin_role: null, if other changes are expected...

vijaycs85’s picture

#1: 1942178-user-config-fix-1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice

The last submitted patch, 1942178-user-config-fix-1.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
PASSED: [[SimpleTest]]: [MySQL] 54,462 pass(es). View

Re-rolling...

YesCT’s picture

Assigned: Unassigned » YesCT
+++ b/core/profiles/standard/config/user.role.administrator.ymlundefined
@@ -1,4 +1,4 @@
-label: Administrator
-weight: 2
+label: 'Administrator'
+weight: '2'

I dont think single words get quotes in config.

Let's be sure to actually save the settings and compare the saved config with the default config.

patch coming.
Also, status is in the saved config.
See #1964254-10: Configuration schemas missing langcode and uuid at places

YesCT’s picture

FileSize
2.37 KB
3.11 KB
FAILED: [[SimpleTest]]: [MySQL] 54,553 pass(es), 17 fail(s), and 5 exception(s). View

a.
minimal profile does have
admin_role: ''

so leaving that.

--
b.
but both minimal and standard have
register: visitors_admin_approval

so that should be changed.

--
c.
$ grep -R status_cancelled * | grep yml | grep -v ".swp"
core/modules/user/config/schema/user.schema.yml: status_cancelled:
core/modules/user/config/user.settings.yml: status_cancelled: '0'
sites/default/files/config_p8IrHKOjHxQP-h2IQnOMp-95ActxXLpbSQfsvUE6hKI/active/user.settings.yml: status_cancelled: '0'
[~/foo/drupal]
01:28 AM [YesCT] (drush-iq-make-user-module-active-config-1942178-#4)
562 $ grep -R status_canceled * | grep yml | grep -v ".swp"
core/modules/user/config/schema/user.schema.yml: status_canceled:
core/modules/user/config/user.mail.yml:status_canceled:
sites/default/files/config_p8IrHKOjHxQP-h2IQnOMp-95ActxXLpbSQfsvUE6hKI/active/user.mail.yml:status_canceled:
sites/default/files/config_p8IrHKOjHxQP-h2IQnOMp-95ActxXLpbSQfsvUE6hKI/active/user.settings.yml: status_canceled: '0'

we should fix canceled vs cancelled
wc says we should use canceled

left the ones in t()'s and comments as that was out of scope I thought, and could use a follow-up.

this change to the update:

/**
 * Moves account settings from variable to config.
 *
 * @ingroup config_upgrade
 */
function user_update_8004() 

might be out of scope.. if so, a separate issue can be opened, but it's the only place
user_mail_status_cancelled_notify is. Does this mean we dont have a test for upgrades? Probably.
core/modules/user/user.install: 'user_mail_status_cancelled_notify' => 'notify.status_cancelled',

--
d.
why is the active config saving all newlines with \r too?

$ diff ./core/modules/user/config/user.mail.yml sites/default/files/config_DocumzykiE6AOl6bPoPGjrDg5FVGEk0g_85eliFXKX0/active/user.mail.yml 
2c2
<   body: "[user:name],\n\nA request to cancel your account has been made at [site:name].\n\nYou may now cancel your account on [site:url-brief] by clicking this link or copying and pasting it into your browser:\n\n[user:cancel-url]\n\nNOTE: The cancellation of your account is not reversible.\n\nThis link expires in one day and nothing will happen if it is not used.\n\n--  [site:name] team"
---
>   body: "[user:name],\r\n\r\nA request to cancel your account has been made at [site:name].\r\n\r\nYou may now cancel your account on [site:url-brief] by clicking this link or copying and pasting it into your browser:\r\n\r\n[user:cancel-url]\r\n\r\nNOTE: The cancellation of your account is not reversible.\r\n\r\nThis link expires in one day and nothing will happen if it is not used.\r\n\r\n--  [site:name] team"

I did not make the active saved config match the default config for user.mail.yml
but that might need a issue opened if we dont want the config saved like that.

Status: Needs review » Needs work
Issue tags: -Configuration system, -Config novice

The last submitted patch, 1942178-user-config-fix-6.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

#6: 1942178-user-config-fix-6.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice

The last submitted patch, 1942178-user-config-fix-6.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,033 pass(es). View

Re-rolling...

mtift’s picture

FileSize
1.12 KB
1.11 KB
PASSED: [[SimpleTest]]: [MySQL] 57,736 pass(es). View

So it seems like we should be leaving the quotes off the single words

vijaycs85’s picture

To keep the label consistence, we add single quotes for single words as well. one of exceptional case.

mtift’s picture

FileSize
1.2 KB
2.23 KB
PASSED: [[SimpleTest]]: [MySQL] 57,698 pass(es). View

Got it -- I see it says "Use single quotes for label values even if they are one word for consistency" on https://drupal.org/node/1905070. Any idea why there are not matching single quotes in the active config directory yml file?

I've added the patch #10 back as #13 to (hopefully!) avoid confusion

mortona2k’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

mortona2k’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,577 pass(es). View

Rerolled.

mortona2k’s picture

NM, there are still fuzz issues.

[edit] I took a second look, seems to work with the latest HEAD now.

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs review » Needs work
Issue tags: -Needs reroll

@vijaycs85 and I double checked, and that link to the standard about single quotes around single words...
is to the schema recommendations. This issue is about a config (not a schema).

further in that doc it says:

Note: The regular configuration data .yml file style dictates you only use single quotes when more than one word is used because the .yml serialization will do that as a standard practice, so this standard makes diff-ing simpler for changing configuration. However the schema recommendations above differ from that, because schema files are always hand-written and using quotes around label values all the time is better for consistency.

this is confusing.. even for us who worked on schemas. :) But... it is that way to try and make it easier for contrib module maintainers.

so... we can check this by resaving the config settings and diffing the default file with the one generated in the active config

mortona2k’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,586 pass(es). View

This one adds single quotes to the mentioned:
Files need to be fixed
user.role.anonymous.yml
user.role.authenticated.yml

And also
entity.view_mode.user.full.yml

And removes them from a single word here:
views.view.user_admin_people.yml

I checked by running diff on the files in the user module config vs my files/config/active and looking at the changes.

Run from files/config/active: diff . ../../../../../core/modules/user/config/

mortona2k’s picture

Issue summary: View changes

Updated issue summary.

mortona2k’s picture

FileSize
1.68 KB
FAILED: [[SimpleTest]]: [MySQL] 58,685 pass(es), 1 fail(s), and 0 exception(s). View

This one also adds single quotes to integers (to match config output).

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

The last submitted patch, 1942178-user-config-fix-20.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

lokapujya’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.81 KB
PASSED: [[SimpleTest]]: [MySQL] 59,461 pass(es). View

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 23: 1942178-23.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review

23: 1942178-23.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 23: 1942178-23.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review

23: 1942178-23.patch queued for re-testing.

vijaycs85’s picture

FileSize
9.04 KB
9.44 KB
FAILED: [[SimpleTest]]: [MySQL] 59,497 pass(es), 6 fail(s), and 0 exception(s). View

More updates...

Status: Needs review » Needs work

The last submitted patch, 28: 1942178-config-schema-user-28.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
lokapujya’s picture

What did you do to find those new updates?

Status: Needs review » Needs work

The last submitted patch, 28: 1942178-config-schema-user-28.patch, failed testing.

ricardoamaro’s picture

Problem:

Applying Patch: https://drupal.org/files/issues/1942178-config-schema-user-28.patch
error: patch failed: core/modules/user/config/entity.view_mode.user.full.yml:1
error: core/modules/user/config/entity.view_mode.user.full.yml: patch does not apply
error: patch failed: core/modules/user/config/user.role.anonymous.yml:1
error: core/modules/user/config/user.role.anonymous.yml: patch does not apply
error: patch failed: core/modules/user/config/user.role.authenticated.yml:1
error: core/modules/user/config/user.role.authenticated.yml: patch does not apply
error: patch failed: core/modules/user/config/views.view.user_admin_people.yml:47
error: core/modules/user/config/views.view.user_admin_people.yml: patch does not apply

This patch is very old and does not apply anymore.
Let me change it manually.

ricardoamaro’s picture

FileSize
4.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,794 pass(es), 21 fail(s), and 0 exception(s). View

New patch for review

ricardoamaro’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 34: 1942178-config-schema-user-34.patch, failed testing.

ricardoamaro’s picture

Status: Needs work » Needs review
ohthehugemanatee’s picture

Assigned: Unassigned » ohthehugemanatee

Claiming this issue.

ohthehugemanatee’s picture

Assigned: ohthehugemanatee » Unassigned
FileSize
3.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,692 pass(es). View

* Updated the patch for current HEAD
* Updated the user_admin_people View to match the schema, so it should pass tests.

ohthehugemanatee’s picture

Issue tags: +drupalcampfi
floretan’s picture

Status: Needs review » Reviewed & tested by the community

Had a look at the code and also reproduced the export, everything is conform.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/user/config/install/user.role.authenticated.yml
    index 6ab676d..99bf69e 100644
    --- a/core/modules/user/config/install/views.view.user_admin_people.yml
    
    --- a/core/modules/user/config/install/views.view.user_admin_people.yml
    +++ b/core/modules/user/config/install/views.view.user_admin_people.yml
    

    All of the changes to this file are incorrect. The way to test this is to enable views and view_ui. Then go and add the admin people view and resave it. Then export your configuration and compare the resulting YAML file to the one in the default.

  2. +++ b/core/modules/user/config/install/views.view.user_admin_people.yml
    @@ -299,7 +299,7 @@ display:
    -          not: '0'
    +          not: 'false'
    

    This change is not necessary

  3. +++ b/core/modules/user/config/install/views.view.user_admin_people.yml
    @@ -614,12 +614,12 @@ display:
    -            user_bulk_form: '0'
    -            name: '0'
    -            status: '0'
    -            rid: '0'
    -            created: '0'
    -            access: '0'
    +            user_bulk_form: 'false'
    +            name: 'false'
    +            status: 'false'
    +            rid: 'false'
    +            created: 'false'
    +            access: 'false'
    

    This change is incorrect '0' is the unselected value.

  4. +++ b/core/modules/user/config/install/views.view.user_admin_people.yml
    @@ -696,8 +696,8 @@ display:
    -              anonymous: '0'
    -              administrator: '0'
    +              anonymous: 'false'
    +              administrator: 'false'
    
    @@ -737,8 +737,8 @@ display:
    -              anonymous: '0'
    -              administrator: '0'
    +              anonymous: 'false'
    +              administrator: 'false'
    
    @@ -778,8 +778,8 @@ display:
    -              anonymous: '0'
    -              administrator: '0'
    +              anonymous: 'false'
    +              administrator: 'false'
    
    @@ -819,8 +819,8 @@ display:
    -              anonymous: '0'
    -              administrator: '0'
    +              anonymous: 'false'
    +              administrator: 'false'
    

    Not necessary

  5. +++ b/core/modules/user/config/install/views.view.user_admin_people.yml
    @@ -941,7 +941,7 @@ display:
    -        context: ''
    +        context: 'false'
    

    Not necessary

ohthehugemanatee’s picture

Status: Needs work » Needs review
FileSize
806 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,691 pass(es). View
2.58 KB

Thanks for the fast review, @alexpott . I tried making the changes manually b/c I thought it would be faster and teach me more about the new standards... my bad. Patch recreated with the automated view export, against current tip of 8.x branch... turns out that the View is actually identical to what's already in 8.x. Just the changes to the schema file.

ohthehugemanatee’s picture

FileSize
806 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,615 pass(es). View
2.58 KB

Agh attached the wrong version of that patchfile and interdiff. It's fine except that it includes an extra whitespace and UUID in the View. Here's the proper version.

ohthehugemanatee’s picture

Status: Needs review » Reviewed & tested by the community

actually, given that this is so simple, the funtional elements are already reviewed, and the busted parts are removed from the patch, I'll just send this right back to you as reviewed. Hope that's in order.

vijaycs85’s picture

latest patch in #44 looks good. As mentioned by @ohthehugemanatee, it is just two label changes in schema file (which exactly what we intended to change as part of this issue as per issue summary). So +1 for RTBC and thanks to all worked on this one.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Actually the views.view.user_admin_people.yml is not quite correct :) it is missing a dependency in one of it's plugins. If you export the view and compare it you'll see.

alexpott’s picture

n.b. we do not include default UUIDs in default config

ohthehugemanatee’s picture

Status: Needs work » Reviewed & tested by the community

@alexpott I think you might have been looking at the wrong version of the patch, which I uploaded by accident in comment 43.

Please check the one in comment 44, which doesn't modify the View at all. The version in 8.x perfectly matched my export (apart from the UUID of course).

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@ohthehugemanatee - yep I exported all the config and compared to default... so you're right the views are looking good but the roles also need

status: true
dependencies: {  }

added at the bottom.

ohthehugemanatee’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
874 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,004 pass(es). View

Re-exported the roles, removed the permissions from them by hand because they aren't in the original... I guess they get added by a different module. Thanks for your patience with me!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: 1942178-config-schema-user-51.patch, failed testing.

ohthehugemanatee’s picture

Status: Needs work » Reviewed & tested by the community

So, the test failed because dependencies isn't defined as an array type in the schema.

1) should that be explicitly defined in our schema, or is that a universal element?
2) does that fit into this ticket anyway?

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Not RTBC if tests are failing. :)

alansaviolobo’s picture

Issue tags: -Config novice +config, +Novice

Status: Needs work » Needs review
quietone’s picture

Yes, this works. Followed instructions by alexpott in #42.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

It still applies and looks good to go.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 0116e11 and pushed to 8.0.x. Thanks!

  • alexpott committed 0116e11 on 8.0.x
    Issue #1942178 by ohthehugemanatee, vijaycs85, mtift, mortona2k, YesCT,...

Status: Fixed » Closed (fixed)

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

iMiksu’s picture

Issue tags: -drupalcampfi

Cleaning up drupalcampfi tags.