Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The account settings at admin/config/people/accounts need to be converted to use the new configuration system.
Tasks
- Create a default config file and add it to the user module.
- Convert the admin UI in the user_admin_settings of user.admin.inc to read to/write from the appropriate config.
- Convert any code that currently accesses the variables used to use the config system.
- It is possible that some of these settings are first set in the installer, which may or may not make life more complicated
- Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.
This is possibly a more involved task than the other novice ones because of the number of variables and the fact that you might need to touch the installer. However if you're feeling brave feel free to give it a start. Any amount of progress would be helpful.
Comment | File | Size | Author |
---|---|---|---|
#203 | drupal-account_settings-1496534-203.patch | 781 bytes | nadavoid |
#197 | 194-196-interdiff.txt | 8.48 KB | alexpott |
#197 | 1496534_196.user_settings.patch | 46.96 KB | alexpott |
#195 | 194-195-interdiff.txt | 8.56 KB | alexpott |
#195 | 1496534_195.user_settings.patch | 46.41 KB | alexpott |
Comments
Comment #1
jcfiala CreditAttribution: jcfiala commentedI'm interested in taking a swing at this.
Comment #2
kim.pepperHi @jcfiala
If you haven't made any progress on this yet, I'm keen to make a start to some of it.
Kim
Comment #3
kim.pepperThis first patch adds only the anonymous variable to the configuration system, and handles the account settings form. The anonymous variable is used an a number of other modules including:
Comment #5
kim.pepperI'm not sure why the last patch failed. There seems to be an exception with the testbot writing to the new config file.
Anyway, I have gone through the remain modules that used to call variable_get('anonymous') and replace it with new config system.
Kim
Comment #7
swentel CreditAttribution: swentel commentedCould be written as:
I'd do this everywhere in case we only need one single variable.
-29 days to next Drupal core point release.
Comment #8
swentel CreditAttribution: swentel commentedThe fail in entity user callback tests is because of the test itself. user.test around line 2287, remove the variable_set and change it to:
The upgrade fail tests are due to the simpletest directory not available yet. There is a fix in either #1496458: Convert maintenance mode settings to configuration system or #1496510: Convert search settings to configuration system that needs to be reviewed and get in first.
Comment #9
kim.pepperThanks for the review. I have shortened all the calls to get config, and corrected the test to set the config correctly.
Comment #11
kim.pepper#9: user_config-1496534-9.patch queued for re-testing.
Comment #13
kim.pepperI'm not sure how to progress from here. The UpgradePath tests that are failing are due to directory not existing??
Kim
Comment #14
kim.pepperOK. Looking at a patch for one of those issues that you posted, I can now see I need this hunk to land before my code will pass.
Comment #15
kim.pepperAdded the above code to this patch to get it to pass tests (works locally).
Comment #16
acrollet CreditAttribution: acrollet commentedI tested this patch, and because the update function called variable_get() without a default, the live setting for anonymous was being set to an empty string. I changed the update function to use the update_variables_to_config() helper function.
Comment #17
marcingy CreditAttribution: marcingy commented#1517138: SimpleTest does not create configuration folder causing test failures has now been committed to get round the folder issue this likely needs a re-roll.
Comment #18
xjm#16: user_config-1496534-16.patch queued for re-testing.
Comment #20
swentel CreditAttribution: swentel commentedThis needs to go since that has been committed elsewhere.
Comment #21
acrollet CreditAttribution: acrollet commentedre-rolled without addition in config.inc.
Comment #22
marcingy CreditAttribution: marcingy commentedThis needs to load the default config
Comment #23
cosmicdreams CreditAttribution: cosmicdreams commentedLoad from the settings.php? This function loads the default config from the variables table into the xml file.
I'm not sure what you mean
Comment #24
cosmicdreams CreditAttribution: cosmicdreams commentedAh I think I figured it out. here ya go:
Comment #25
cosmicdreams CreditAttribution: cosmicdreams commentedGo testbot
Comment #26
marcingy CreditAttribution: marcingy commentedSorry I should have explained more that function loads an xml to perform processing but there will be no active configuration at this point in time so you need to load the file into the active store. config_install_default_config needs to be called in the update function first.
Comment #27
marcingy CreditAttribution: marcingy commentedCross post :(
Comment #28
gddIs this line necessary? This should happen automatically through the form api.
We should chain these calls.
Ultimately this patch looks good, although there is a LOT more information in this config form than is included here, and I'm not sure I see a lot of value in committing this incomplete. Lets take this as a starting point and keep it moving forward?
-27 days to next Drupal core point release.
Comment #29
cosmicdreams CreditAttribution: cosmicdreams commentedOk, first here's the patch with changes requested in #28 (attached)
Second, let's try to itemize the settings to include that would make this an acceptable patch:
Are this settings (once converted to cmi) sufficient:
Any others?
Comment #31
cosmicdreams CreditAttribution: cosmicdreams commentedHere's the patch that includes all of those variables.
Comment #32
cosmicdreams CreditAttribution: cosmicdreams commentedgrr, that doesn't sound like the right patch. Trying again:
Comment #34
cosmicdreams CreditAttribution: cosmicdreams commentedHere's an attempt to fix the blockers
Comment #36
cosmicdreams CreditAttribution: cosmicdreams commented#31: 1496534_29_cmi_user_account.patch queued for re-testing.
Comment #37
kim.pepperLooks like the same issue as #17 ?
Comment #38
cosmicdreams CreditAttribution: cosmicdreams commentedI've removed the changes to maintenance mode and added a install step to the upgrade so I can perform a loading of system settings. Hopefully this improves things. But it will unlikely solve all of the issues as there are some unresolved issues with the user modules.
Comment #39
cosmicdreams CreditAttribution: cosmicdreams commented#37 do you mean #16?
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedconfig()->set() needs to be followed by ->save(). implemented in attached patch.
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commentedwoops, forgot the new config file.
Comment #44
cosmicdreams CreditAttribution: cosmicdreams commentedincludes the new config file
Comment #46
marcingy CreditAttribution: marcingy commentedThe issue looks like the update hook needs to load system config as well as user config.
Comment #47
cosmicdreams CreditAttribution: cosmicdreams commented@marcingy the system update hook or the user update hook?
Comment #48
marcingy CreditAttribution: marcingy commentedMy bad I missing the second update hook when quickly eyeballing last night. The upgrade issue looks related to this change http://drupal.org/node/1517138#comment-5898532 as it seems like we don't have a config folder however head works at the moment I believe we have configs being loaded now in other place (might be wrong though).
Comment #49
cosmicdreams CreditAttribution: cosmicdreams commentedYea, it does sound like that, but the fix was committed on April 20th. I dont know why I'm still having this issue.
Comment #50
marcingy CreditAttribution: marcingy commentedThe change in the issue above means the directory is no longer created by simple test.
Comment #51
cosmicdreams CreditAttribution: cosmicdreams commentedDoes that mean I need to include code to create it myself?
Comment #52
marcingy CreditAttribution: marcingy commentedI think we need to fix the original issue and get that part of the fix in on its own.
Comment #53
sunThe upgrade test failures happen, because the current upgrade path tests override the default DrupalWebTestCase::setUp() and ::prepareEnvironment() methods, of which the latter creates the config directory for the test run.
We should introduce a temporary helper function that wraps variable_get() and is used wherever variable_get() is getting called for a variable that has been converted to the config system already, so we can identify these more easily later on.
Whenever leveraging chained method calls, the code should follow our coding standard for chained method calls; i.e., as with db_select(), all method calls should be on a separate line (including the first).
If config save only involves setting one key and fits into 80 chars, then a single line containing config()->set()->save() is fine.
The default front page is "user", not "node".
Let's define the config object as a $config variable for the entire form to make this prettier.
Comment #54
cosmicdreams CreditAttribution: cosmicdreams commentedawesome feedback. Will get to work on this patch tonight.
Comment #55
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a patch that should contain all of the improvements outlined above.
I also found a line where I was setting config and not saving it.
Comment #56
cosmicdreams CreditAttribution: cosmicdreams commentedComment #58
cosmicdreams CreditAttribution: cosmicdreams commentedSearched through the code and found 5 or 6 instances where the frontpage settings were handled by variable_get, variable_set.
Fixed those and added some more debugging information. (which I will take out if this goes green)
Comment #59
cosmicdreams CreditAttribution: cosmicdreams commentedComment #61
cosmicdreams CreditAttribution: cosmicdreams commentedOK, this patch might fix a lot of remaining fails but, it opens the door for converting site_403, site_404 and default_nodes_main to the config system. These new three variables are also on the account settings page and need to be converted at the same time as these others so that the code can be clean.
Let's see how the test come out.
Comment #63
cosmicdreams CreditAttribution: cosmicdreams commentedCool! only 5 more fails. But I don't really know what's going on in the upgrade test fails. I'm going to need another set of eyes on this to help me understand what is wrong.
Comment #64
gddIt seems likely that the upgrade tests are related to #1500312: Create $config_directory_name during a D7 => D8 upgrade. Marcingy was working on it so hopefully it will get in soon.
Comment #65
marcingy CreditAttribution: marcingy commentedI'll try and take a look at the upgrade failures this evening - it is blocked on #1541958: Split setUp() into specific sub-methods. The issue at the moment is that the upgrade.test does not create a config folder unlike web test case and my fix in http://drupal.org/node/1517138#comment-5944154 needs to be worked on as part of the other issue.
Comment #66
cosmicdreams CreditAttribution: cosmicdreams commentedAwesome! We still could do more work to cleanup two of the failures. The "HTML in page titles" and "Theme API" tests seem like solvable problems. I'll have time to look into those perhaps tomorrow, perhaps tonight.
Hopefully they are simple enough that a more experienced dev could see the issue after about 5 minutes.
Comment #67
marcingy CreditAttribution: marcingy commentedFrom a quick test once http://drupal.org/node/1541958#comment-5944186 gets committed (I'll do a review this evening hopefully) this patch will be unblocked as a config dir in upgrade paths is created.
Comment #68
cosmicdreams CreditAttribution: cosmicdreams commentedExcellent news!
Comment #69
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch should fix one of the tests: the Theme API test.
Comment #71
cosmicdreams CreditAttribution: cosmicdreams commentedI must have uploaded the wrong patch. The only thing I changed was a test. This shouldn't have caused more failures.
Comment #72
cosmicdreams CreditAttribution: cosmicdreams commentedSecond try
Comment #73
cosmicdreams CreditAttribution: cosmicdreams commentedComment #75
cosmicdreams CreditAttribution: cosmicdreams commentedThird try
Comment #77
cosmicdreams CreditAttribution: cosmicdreams commentedOK, so with the ongoing work to fix the upgrade tests happening elsewhere, we've got 1 remaining test to fix here. I've looked into why the title is not working properly with the XXS test but I found that using filter_xss or filter_xss_admin doesn't help.
Comment #78
cosmicdreams CreditAttribution: cosmicdreams commented#75: 1496534_73_account_cmi.patch queued for re-testing.
Comment #80
cosmicdreams CreditAttribution: cosmicdreams commentedreroll
Comment #81
cosmicdreams CreditAttribution: cosmicdreams commentedComment #82
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like the database changes folks have been talking about landed. Also, the HTML page title failed test is passing locally (I didn't even fix it, something else must have).
So if this reroll might go green!
Comment #84
cosmicdreams CreditAttribution: cosmicdreams commentedI'm suspicious that the upgrade tests continue to fail. This leads me to question how soon I have the config() method is available. I'm using the config method to set default values in the install.core.inc form for account settings.
That would be the likely cause of the installation failures if config is available at that time.
Anyway, the HTML title test continues to pass in my local environment. Trying again.
Comment #85
cosmicdreams CreditAttribution: cosmicdreams commentedgo testbot
Comment #87
cosmicdreams CreditAttribution: cosmicdreams commented#84: 1496534_82_account_cmi.patch queued for re-testing.
I saw some updates to the update tests. I want to see if this improves the patch's ability to pass those tests. If it doesn't I've been able to make the patch go green locally by reverting the change it's making to the install.core.inc.
I'd rather revert those changes, confirm this patch goes green, and continue the changes we need to convert the other account settings to the new config system.
Perhaps we need to create a new issue for the config changes we need to make in the install.core.inc or other files that drive the install process.
Comment #89
cosmicdreams CreditAttribution: cosmicdreams commentedthis patch reverts all changes to the install.core.inc file. If this goes green or only has 1 fail I'll continue with spreading the config changes to the remainder of the account settings as itemized above.
Comment #90
cosmicdreams CreditAttribution: cosmicdreams commentedDoe. here's the patch
Comment #92
cosmicdreams CreditAttribution: cosmicdreams commentedI'm considering if #1496542: Convert site information to config system should be marked as a duplicate.
Comment #93
alexpottThis currently failing the HTML in page titles (PageTitleFiltering) [System] test because XML is failing to save the title when it is set in testTitleXSS() to
After applying the patch in #44 in #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML and converting system.site.xml to Yaml this test passes.
Comment #94
cosmicdreams CreditAttribution: cosmicdreams commentedInteresting... wonder why it was passing on my system. That gives me hope. Looks like the remaining tests are update related. And in my previous post I tried to get rid of the changes that the patch makes to install.core.inc in the hopes of passing those tests too. I might not have done that properly so I'll try again tonight.
Comment #95
alexpottOne of the issues with the current patch and the upgrade path tests are these lines from the setUp() method UpgradePathTestCase:
This should stay as it was as this 'variable_set' is occurring on the db that is going to be upgraded - and therefore it does not have the config table yet. Patch attached reverts this change.
Comment #97
alexpottI get passes in the upgrade tests apart from the "Language Upgrade Test" with #95 after applying #1541958: Split setUp() into specific sub-methods
Comment #98
marcingy CreditAttribution: marcingy commented#95: 1496534_95_account_cmi.patch queued for re-testing.
Comment #100
cosmicdreams CreditAttribution: cosmicdreams commentedJust wanted to post a work in progress here: This will likely have many fails.
This patch refocuses this issue on converting all of the settings in the user_admin_settings function within user.admin.inc (starting at line 260). There are MANY settings, and I've tried to compile a list of them as shown in the user.account_settings.xml file.
What remains in this issue is the following:
Previously, I found that if I change one line in system_settings_form_submit I can persist every setting into the new config system. (line 2839). The problem with this approach is that we now namespace our sets of configuration changes into separate files. Is it possible to algorithmically discover the proper namespace of each set of config changes? If so we will have nice solution on our hands, and a forced naming structure for our configuration files.
Comment #101
gddUnfortunately right now that is not possible, and problems with system_settings_form() is why we have just been using individual submit functions in the previous patches. There is an issue around this at #1324618: Implement automated config saving for simple settings forms
Comment #102
cosmicdreams CreditAttribution: cosmicdreams commentedWIP
Comment #104
alexpottHmmm... this patch is getting humungous... more work in progress...
For some reason in tests the following is not working:
The first config is picked up by the simpletest install - the second is not. If you remove the first test the second test works fine. If you output a select from the config table in the simpletest site during the the second test it returns the wrong information. However if you pause simpletest before it runs test 2 but after the config is saved for the second time the config with the simpletest prefix has been updated... why is debugging simpletest such a major PITA :)
Comment #105
cosmicdreams CreditAttribution: cosmicdreams commentedalex, did you see the patch I put in 102 that had zero fails? I think all you need to do is track down those three exceptions and we're good. Each of the three exceptions are likely the cause of configuration conversation that I missed. Just search for each of the settings in the user.account.xml file and see if they need to be set somewhere.
Comment #107
alexpottcosmicdreams patch in #104 was built from your patch but includes conversion to the config system of all uses of the key/value pairs defined in user.account_settings.xml. Once this patch has landed we can't have an variable_get/set('user_register'...
My issue with changing config only seems to happen for me :(... same 3 errors to fix :)
Comment #108
alexpottPatch to fix the 3 exceptions...
EDIT: This patch includes debug code
Comment #109
alexpottComment #110
cosmicdreams CreditAttribution: cosmicdreams commentedcool, I'll just assume this passes and review your patch right away.
Comment #111
alexpottBorked patch... here's a new one
Comment #112
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a first pass on reviewing #111
Comment #113
alexpottCleanup some whitespace issues I introduced in #111
Comment #114
alexpottComment #115
cosmicdreams CreditAttribution: cosmicdreams commentedReviewed the patch, change by change. It's looks good the only "improvement" I would make the to code is:
In user.test, some of the config()->set's could be chained or we could defer the ->save() for a little bit so that more configuration sets could occur before we write out to the file. For example:
could be
Comment #116
alexpottUpdated patch attached as per #115 and fixed a couple of spaces at end of line issues
Ignore this patch
Comment #117
alexpottHad some issues rebasing the patch to latest changes to 8.x
Comment #119
alexpott#117: 1496534_117_account_settings_cmi.patch queued for re-testing.
Comment #120
aspilicious CreditAttribution: aspilicious commentedtrailing whitespace
16 days to next Drupal core point release.
Comment #121
cosmicdreams CreditAttribution: cosmicdreams commentedI think I've tracked down that whitespace and fixed it with this patch.
Comment #122
cosmicdreams CreditAttribution: cosmicdreams commentedComment #123
alexpottYep cosmicdreams you got it... p.s. try this out http://drupal.org/project/dreditor make finding the little critters very simple... I also try to always run the following command on a patch before submitting:
grep -n "^\+.* $" patch_filename
Comment #124
cosmicdreams CreditAttribution: cosmicdreams commentedIs there anything more that needs to be done on this patch other than having a solid review?
Comment #125
alexpottWait for #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML so we can convert the config xml file to Yaml :)
Comment #126
wizonesolutionsReroll against HEAD, though now out of date I guess.
Comment #127
cosmicdreams CreditAttribution: cosmicdreams commentedI don't feel that we need to wait unless you're talking about "Wait a day" because the other patch is going in today.
Comment #129
cosmicdreams CreditAttribution: cosmicdreams commented#121: 1496534_121_account_cmi.patch queued for re-testing.
Comment #130
gdd- The patch at #126 is missing the default configuration file.
- This file needs to be converted to YAML per #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML
- The names need to match the new naming conventions laid out at http://drupal.org/node/1667896
Comment #131
sunAll config system conversions are API changes, so tagging as such.
Comment #132
cosmicdreams CreditAttribution: cosmicdreams commentedLooking at the config that is used by this module, I think the resultant yml file should be:
Comment #133
dagmarI'm now sure if this is correct:
By default this is set to 0 in the original variable_get.
Also, I'm not sure if this should be updated to a machine name ID because of this: #935062: Change role id to machine name
Comment #134
aspilicious CreditAttribution: aspilicious commented- Maybe anonymous should be 'anonymous_text'
- User roles are machine names now: http://drupal.org/node/1619504
- picture.pictures should be picture.enabled
- maybe we should group the mail settings
- register sounds like the account is registrered or not, but in fact it defines if you can register... Maybe use can_register (not sure at all)
So maybe something like this?
Comment #135
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch:
I suspect that this patch breaks, and I am suspicious of the number of tests that needed to be changed in order to use these settings. I feel there need to be more tests. But I haven't spent the time to discover why.
Let's see how this does.
Comment #136
cosmicdreams CreditAttribution: cosmicdreams commentedHa, I worked up that patch without seeing aspilicious's comment. Looks like we agreed on the renaming of the user_pictures variable.
I'll include the other changes into a following patch.
EDIT:
One exception:
The mail status config setting can't have it's "status" broken away from it's activated or blocked or cancelled.
This is because of line number 3305 of user.module.
Comment #138
aspilicious CreditAttribution: aspilicious commentedMore improvements
Comment #139
aspilicious CreditAttribution: aspilicious commentedEven more improvements
Comment #141
dagmarHm. This means that every autenticated user of the site will be an administrator. Not really secure.
Comment #142
aspilicious CreditAttribution: aspilicious commentedThis one should fix a bunch of the failures
Comment #144
dagmarLets try removing the line
Comment #145
aspilicious CreditAttribution: aspilicious commentedMore fixes!
Comment #147
aspilicious CreditAttribution: aspilicious commentedArgh typo...
Comment #148
cosmicdreams CreditAttribution: cosmicdreams commentedI did a code review of the previous patch, but the wifi keeps eating it.
There are some incomplete conversions in the code. Search for user_picture_* and ensure it has the proper picture.* syntax.
Also, did my note about why separating "status" from (blocked, activated, cancelled) is a bad idea as there is a line of code that expects the variable name to have status and the state combined?
Comment #150
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a try to incorporate the feedback I posted above:
This patch will also fail, because of line 3305 of user.module.
Comment #151
aspilicious CreditAttribution: aspilicious commentedI'm not sure it's that bad to seperate status. Needs more discussion.
Fixed more stuff...
Comment #152
cosmicdreams CreditAttribution: cosmicdreams commentedCool, looks like this incorporates the missing conversions. Doing a closer review now. I'll keep an eye towards line 3305 of user.module and how you're handling that.
Comment #153
aspilicious CreditAttribution: aspilicious commentedOh crap I see what you mean, line 3305 isn't a problem at all I just forgot to save my change:
$notify = config('user.settings')->get('mail.status.' . $op . '_notify');
should work
Comment #154
aspilicious CreditAttribution: aspilicious commentedoh wait... damnit... the $op is status_cancelled and not cancelled.. damnit!
Comment #155
aspilicious CreditAttribution: aspilicious commentedFinal try and now lets wait an hour for the bot lol
Comment #156
dagmarSame patch that #155 with changes suggested in #144. It should fix 7 more warnings.
Comment #157
cosmicdreams CreditAttribution: cosmicdreams commentedI think in these cases we want to standardize on the fluid syntax instead of the multi-line syntax. So instad of this use:
$config
->set('signatures', 1)
->set('picture.enabled', 1)
->save();
incomplete conversion
You may have incorporated my patch, because previously you had this variabled named as anonymous_name and now it's just anonymous.
I think that's how I had it. I liked your name better.
Missed this one.
Many examples of not using the fluid syntax
another missed conversion.
What I'm suggesting is to have mail.status_activated_notify instead. What way status_activated remains a statement that we can plugin in while the process iterates over status states.
We can still group "mail" stuff, but have only one level of items, not two.
What do you think?
Missed conversion
The $op coming in here contains status_cancelled, status_activated or status_blocked. That's why I'm saying we have to keep the string status and state.
Missed conversion.
missed conversion.
Ah THIS is where you have $anonymous_name. I still like that name better. just calling the variable "anonymous" is as descriptive as it could be.
Missed conversion
Comment #158
cosmicdreams CreditAttribution: cosmicdreams commentedI can make the changes in 157 if that will help progress.
Comment #159
aspilicious CreditAttribution: aspilicious commentedCan we stop posting patches next to each other. Dagmar removing that line is incorrect. There has to be a default value in a config file.
Quick patch so I can go get my diner. I ddn't convert the anonymous stuff yet.
Comment #160
dagmarOk, don't remove it, but don't use authenticated as the default value either. It is a security risk.
The original code says: variable_get('user_admin_role', 0)
So, by default user_admin_role is not defined. With this patch, you are saying that the Admin Role == Authenticated users.
I think this is the reason why Drupal\filter\Tests\FilterFormatAccessTest was failing in #151.
Comment #162
cosmicdreams CreditAttribution: cosmicdreams commentedI made another pass on this patch and fixed a number of conversions and reformatted a few more. There was one case where a set needed a save().
There was another case where I needed to rearrange comments to have more consistent (fluid) syntax.
Let's see how this goes.
Comment #163
sunThanks for working on this! :)
Also, friends, please use the "Assigned" property/field to declare ownership and prevent concurrent work! :)
As much as I'd like to see this issue resolved, I'm very tempted to postpone this on #1292470: Convert user pictures to Image Field -- that is, because changing all these user picture variables will result in a major hazard for that issue (which has been RTBC already, just needs some more upgrade path testing/confirmations). At least, that order would make much more sense to me, since we'd just simply remove all the user picture settings and changes from this patch here afterwards.... ;)
Instead of postponing, we could as well just leave out and ignore the user picture settings in this patch?
Comment #164
cosmicdreams CreditAttribution: cosmicdreams commentedsun: will do.
aspillicious: I'll mark this on me while I do this then relinquish after I'm done.
Comment #165
cosmicdreams CreditAttribution: cosmicdreams commentedPulled out picture config from the CMI patch
found a typo on line 3084 in user.module where we weren't calling the right variable.
Comment #167
cosmicdreams CreditAttribution: cosmicdreams commentedFound my mistake, didn't catch that I'm using an undefined variable now that I've pulled out the picture stuff.
Hopefully this one actually gets to tests that run.
If this passes, I'll unassign myself.
Comment #169
cosmicdreams CreditAttribution: cosmicdreams commentedFound a number of places where configuration had not been converted to use cmi. Mostly these were user_register, I'm hopeful that I've caught everything now.
Comment #171
alexpottThe patch attached fixes some of the test failures above.
This change in #169 from variable to CMI was not equivalent.
Comment #172
alexpottComment #174
alexpottThe single test failure is because
contact_form_user_admin_settings_alter()
adds the variablecontact_default_status
to the user_admin_settings form which is no longer being saved once the form is converted to CMI.Comment #175
alexpottThe patch attached fixes the issue described in #174 by converting the contact setting that is added to the user_admin_settings form to CMI. The rest of the contact system will be converted by #1588422: Convert contact categories to configuration system
Comment #176
alexpottFix missing line at end of file.
Comment #177
sunNo need for array_unshift(), just use the regular $form['#submit'][].
Missing phpDoc.
Let's use the regular style/syntax for chained methods, please :)
Should be an empty string.
1) I think it would make sense to bundle the 'register' and 'verification' keys together. Those two define the user registration mode of a site. I.e., the 'verification' setting is not about sending out a simple notification; no, it declares whether your site requires a valid e-mail address for each site member, and that to be confirmed, before they can log in to the site.
2) The value for that current 'register' is bogus - the USER_REGISTER_* constants are defined as integers currently. Ideally, we should replace the values of those constants (pretty much like we did for #1493108: Convert logging and error settings to configuration system), so the value in user.settings makes sense for humans.
I'd suggest to replace "mail" with "notify", and thus, simplify the keys and their usage throughout User module.
Let's update and merge these comments a bit more smartly... it's a single operation now :)
This should point to the submit handler instead of system_config_form().
1) The phpDoc summary should be:
"Form submission handler for user_admin_settings()."
2) &$form_state should always be taken by reference.
Comment #178
alexpottPatch attached:
update_variables_to_config()
and$config->set('register', $map[$user_register])->save()
I'm not 100% convinced about the USER_REGISTER_* constants change as there was quite a few places is the code that assumes that a value of 0 for
user.settings:register
means that users can not register eg.Comment #179
aspilicious CreditAttribution: aspilicious commentedNeeds to be "status_activated", "status_blocked", ... it looks like this isn't tested?
28 days to next Drupal core point release.
Comment #180
alexpottGood point. The effects of changing the values are tested ... but the values are always set before testing.
Interdiff:
Comment #181
aspilicious CreditAttribution: aspilicious commentedquoting sun
Comment #182
alexpottre. #181 I think @sun's point was more valid when the verification key was a sub-key of mail but now it is not a sub-key of notify I think this is no longer a problem. I think the current patch is fine. Additionally
mail_verification
should not only be for registration but also email change.In order to combine it will the
register
settings we would need sub-keys for register eg.Or 5 USER_REGISTER_* constants :)
The patch attached fixes the key order of the settings YAML file.
Comment #183
cosmicdreams CreditAttribution: cosmicdreams commentedStill applies and the patch looks good.
Comment #184
sunI'd *love* to see a separate follow-up issue to replace those wonky tristate constants with properly separated configuration keys; e.g., 'registration.enabled' and a separate 'registration.admin_approval'. That would heavily simplify the code all over the place.
Definitely out of scope for this issue though. If you'll create that issue, please link to it from here. :)
Hm. The new condition looks wrong to me. Unless I'm mistaken, the entire second OR condition can and should be removed.
Comment #185
ZenDoodles CreditAttribution: ZenDoodles commentedCreated followup issue #1712296: Replace UserInterface::REGISTER_* tristate constants with properly separated configuration keys.
Also, unless I missed something while scanning the #184 comments (tagging for issue summary too), I agree with sun re: not adding the new condition here. If I did miss a reason to do so, maybe it would also be appropriate in a follow-up issue?
Comment #186
aspilicious CreditAttribution: aspilicious commentedLets see if it blows up...
Comment #187
aspilicious CreditAttribution: aspilicious commentedComment #188
aspilicious CreditAttribution: aspilicious commentedsigh another try
Comment #190
alexpottIt definitely does blow up if the second condition from the OR is removed.
_user_mail_notify()
gets called with all sorts of things that are not controlled by notify.status_* configuration. See http://api.drupal.org/api/drupal/core%21modules%21user%21user.module/fun...In order to drop the second condition the notify would need to look like this:
But this seems out-of-scope to me.
Comment #191
alexpottRe-uploading patch from #182 as I think now that @sun's second point is answered this one should be good to go.
Comment #192
sun#190 actually looks way more appropriate to me... :-/
We're essentially missing configuration keys/values in the default config object/file. I actually hope that we did not forget to include similar keys in other converted config objects.
Comment #193
alexpottCurrently re-rolling...
Comment #194
alexpottRerolled patch and made changes suggested by #190 after chatting with @sun and deciding that this definitely was not out of scope.
Interdiff is nuts ... not point.
Comment #195
alexpottafter talking to @sun we're renaming
mail_verification
toverify_mail
Comment #197
alexpottPatch in #195 is well borked :( - a drupalconmunich tired patch - and this issue needs a few comments :)
Correct patch and interdiff attached.
After talking to @sun we're
renaming mail_verification
toverify_mail
Comment #198
aspilicious CreditAttribution: aspilicious commentedReviewd this and it all looks good to me...
Comment #199
webchickWow, great work on this folks! This looks like a good start, so committed and pushed to 8.x. Thanks!
I know the user picture stuff is at #1292470: Convert user pictures to Image Field but did not see a similar follow-up for user mail templates. Can we make sure there is one?
My only other comment would be that I don't understand why:
you took out the "mail" portion of this variable name grouping, which is the most useful word in it to help understand what's going on.
I imagine we need a change notice here for these renamed variables, so marking such.
Comment #200
sunThanks! :)
We don't do individual change notices for all the converted variables. There's a single change notice that informs about the new configuration system. I don't know whether it makes sense to provide a more detailed listing of converted variables somewhere in the upgrade docs, but if we want to do so, then we should discuss how to do that in a dedicated issue (guess it will boil down to grepping for update_variables_to_config() throughout Drupal core prior to the D8 release).
ACK on user mail templates - let's create a separate issue for those. :)
#1757566: Convert user account e-mail templates to configuration system
I purposively did not want to have "mail" in the 'notify' key, because I believe there is nothing in core that prevents anyone from overriding the default decision/assumption of using e-mails for these notifications. The configuration only declares which notifications to send, not how they ought to be sent.
Comment #202
Lars Toomre CreditAttribution: Lars Toomre commentedLooks like this patch missed one conversion of 'user_mail_status_canceled_notify' in the UserCancelTest.php Test class.
It also appears that there is a problem in the testMassUserCancelByAdmin() method since it is passing despite the variable_set().
Comment #203
nadavoid CreditAttribution: nadavoid commentedAddressing the 2 issues raised in #202.
The attached patch converts
variable_set('user_mail_status_canceled_notify', TRUE);
toconfig('user.settings')->set('notify.status_canceled', TRUE)->save();
.It appears that
testMassUserCancelByAdmin()
has already been updated, the instance ofvariable_set()
replaced.Comment #204
nadavoid CreditAttribution: nadavoid commentedComment #205
andyceo CreditAttribution: andyceo commentedLooks good to me.
Comment #206
webchickCommitted and pushed #203 to 8.x. Thanks!
Comment #207.0
(not verified) CreditAttribution: commentedUpdated issue summary.