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.
Related Issues
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
Comment | File | Size | Author |
---|---|---|---|
#51 | 1942178-config-schema-user-51.patch | 874 bytes | ohthehugemanatee |
#44 | interdiff.txt | 2.58 KB | ohthehugemanatee |
#44 | 1942178-config-schema-user-43.patch | 806 bytes | ohthehugemanatee |
#39 | 1942178-config-schema-user-39.patch | 3.46 KB | ohthehugemanatee |
#34 | 1942178-config-schema-user-34.patch | 4.06 KB | ricardoamaro |
Comments
Comment #0.0
vijaycs85Updated issue summary.
Comment #1
vijaycs85Issuing 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
active-folder/user.settings.yml
May be we need to change
admin_role: ''
toadmin_role: null
, if other changes are expected...Comment #2
vijaycs85#1: 1942178-user-config-fix-1.patch queued for re-testing.
Comment #4
vijaycs85Re-rolling...
Comment #5
YesCT CreditAttribution: YesCT commentedI 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
Comment #6
YesCT CreditAttribution: YesCT commenteda.
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:
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?
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.
Comment #8
vijaycs85#6: 1942178-user-config-fix-6.patch queued for re-testing.
Comment #10
vijaycs85Re-rolling...
Comment #11
mtiftSo it seems like we should be leaving the quotes off the single words
Comment #12
vijaycs85To keep the label consistence, we add single quotes for single words as well. one of exceptional case.
Comment #13
mtiftGot 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
Comment #14
mortona2k CreditAttribution: mortona2k commentedLooks good.
Comment #15
alexpottPatch no longer applies.
Comment #16
mortona2k CreditAttribution: mortona2k commentedRerolled.
Comment #17
mortona2k CreditAttribution: mortona2k commentedNM, there are still fuzz issues.
[edit] I took a second look, seems to work with the latest HEAD now.
Comment #18
YesCT CreditAttribution: YesCT commented@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:
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
Comment #19
mortona2k CreditAttribution: mortona2k commentedThis 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/
Comment #19.0
mortona2k CreditAttribution: mortona2k commentedUpdated issue summary.
Comment #20
mortona2k CreditAttribution: mortona2k commentedThis one also adds single quotes to integers (to match config output).
Comment #21
vijaycs85Looks good to me.
Comment #22.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #23
lokapujyaRe-roll.
Comment #25
lokapujya23: 1942178-23.patch queued for re-testing.
Comment #27
lokapujya23: 1942178-23.patch queued for re-testing.
Comment #28
vijaycs85More updates...
Comment #30
vijaycs8528: 1942178-config-schema-user-28.patch queued for re-testing.
Comment #31
lokapujyaWhat did you do to find those new updates?
Comment #33
ricardoamaro CreditAttribution: ricardoamaro commentedProblem:
This patch is very old and does not apply anymore.
Let me change it manually.
Comment #34
ricardoamaro CreditAttribution: ricardoamaro commentedNew patch for review
Comment #35
ricardoamaro CreditAttribution: ricardoamaro commentedComment #37
ricardoamaro CreditAttribution: ricardoamaro commentedComment #38
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedClaiming this issue.
Comment #39
ohthehugemanatee CreditAttribution: ohthehugemanatee commented* Updated the patch for current HEAD
* Updated the user_admin_people View to match the schema, so it should pass tests.
Comment #40
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedComment #41
floretan CreditAttribution: floretan commentedHad a look at the code and also reproduced the export, everything is conform.
Comment #42
alexpottAll 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.
This change is not necessary
This change is incorrect '0' is the unselected value.
Not necessary
Not necessary
Comment #43
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedThanks 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.
Comment #44
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedAgh 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.
Comment #45
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedactually, 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.
Comment #46
vijaycs85latest 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.
Comment #47
alexpottActually 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.
Comment #48
alexpottn.b. we do not include default UUIDs in default config
Comment #49
ohthehugemanatee CreditAttribution: ohthehugemanatee commented@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).
Comment #50
alexpott@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
added at the bottom.
Comment #51
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedRe-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!
Comment #53
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedSo, 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?
Comment #54
xjmNot RTBC if tests are failing. :)
Comment #55
alansaviolobo CreditAttribution: alansaviolobo commentedComment #57
quietone CreditAttribution: quietone commentedYes, this works. Followed instructions by alexpott in #42.
Comment #58
vijaycs85It still applies and looks good to go.
Comment #59
alexpottThis 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!
Comment #62
iMiksuCleaning up drupalcampfi tags.