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.

CommentFileSizeAuthor
#203 drupal-account_settings-1496534-203.patch781 bytesnadavoid
#197 194-196-interdiff.txt8.48 KBalexpott
#197 1496534_196.user_settings.patch46.96 KBalexpott
#195 194-195-interdiff.txt8.56 KBalexpott
#195 1496534_195.user_settings.patch46.41 KBalexpott
#194 1496534_194.user_settings.patch47.06 KBalexpott
#191 1496534_182_account_cmi.patch44.41 KBalexpott
#188 1496534_186_account_cmi.patch44.36 KBaspilicious
#186 drupal-change_to_config_for_tz-1571632-54.patch21.87 KBaspilicious
#182 1496534_182_account_cmi.patch44.41 KBalexpott
#182 180-182-interdiff.txt482 bytesalexpott
#180 1496534_180_account_cmi.patch44.41 KBalexpott
#178 176-178-interdiff.txt20.38 KBalexpott
#178 1496534_178_account_cmi.patch44.39 KBalexpott
#176 1496534_176_account_cmi.patch47.24 KBalexpott
#175 171-175-interdiff.txt4.47 KBalexpott
#175 1496534_175_account_cmi.patch47.27 KBalexpott
#171 169-171-interdiff.txt647 bytesalexpott
#171 1496534_171_account_cmi.patch42.65 KBalexpott
#169 1496534_169_account_cmi.patch42.51 KBcosmicdreams
#167 1496534_167_account_cmi.patch38.28 KBcosmicdreams
#165 1496534_165_account_cmi.patch38.27 KBcosmicdreams
#162 1496534_160_account_cmi.patch59.66 KBcosmicdreams
#159 1496534_159_user_registration_cmi.patch61.35 KBaspilicious
#156 1496534_156_user_registration_cmi.patch59.49 KBdagmar
#155 1496534_153_user_registration_cmi.patch61.38 KBaspilicious
#153 1496534_152_user_registration_cmi.patch61.39 KBaspilicious
#151 1496534_150_user_registration_cmi.patch61.39 KBaspilicious
#150 1496534_149_account_cmi.patch59.42 KBcosmicdreams
#147 1496534_147_user_registration_cmi.patch60.22 KBaspilicious
#145 1496534_144_user_registration_cmi.patch60.22 KBaspilicious
#144 1496534_144_user_registration_cmi.patch41.25 KBdagmar
#142 1496534_141_user_registration_cmi.patch42.65 KBaspilicious
#139 1496534_139_user_registration_cmi.patch42.56 KBaspilicious
#138 1496534_138_user_registration_cmi.patch42.67 KBaspilicious
#135 1496534_132_user_registration_cmi.patch39.94 KBcosmicdreams
#126 1496534_125_account_cmi.patch60.05 KBwizonesolutions
#121 1496534_121_account_cmi.patch61.03 KBcosmicdreams
#116 1496534_116_account_cmi.patch61.29 KBalexpott
#117 1496534_117_account_settings_cmi.patch61.03 KBalexpott
#113 1496534_113_account_settings_cmi.patch61.11 KBalexpott
#112 1496534_112_account_settings_cmi.patch61.11 KBcosmicdreams
#108 1496534_108_account_cmi.patch70.53 KBalexpott
#111 1496534_110_account_cmi.patch61.29 KBalexpott
#104 1496534_104_account_cmi.patch61.18 KBalexpott
#102 1496534_102_account_cmi.patch22.08 KBcosmicdreams
#95 1496534_95_account_cmi.patch52.68 KBalexpott
#90 1496534_89_account_cmi.patch53.28 KBcosmicdreams
#84 1496534_82_account_cmi.patch55.82 KBcosmicdreams
#80 1496534_80_account_cmi.patch55.69 KBcosmicdreams
#75 1496534_73_account_cmi.patch55.24 KBcosmicdreams
#72 1496534_71_account_cmi.patch55.24 KBcosmicdreams
#69 1496534_69_account_cmi.patch54.36 KBcosmicdreams
#61 1496534_61_account_cmi.patch55.25 KBcosmicdreams
#58 1496534_64_account_cmi.patch55.09 KBcosmicdreams
#55 1496534_55_account_cmi.patch50.99 KBcosmicdreams
#44 1496534_44_account_cmi.patch49.44 KBcosmicdreams
#41 1496534-41.patch48.54 KBAnonymous (not verified)
#38 1496534_37_account_cmi.patch49.12 KBcosmicdreams
#34 1496534_34_account_cmi.patch55.94 KBcosmicdreams
#32 1496534_30_account_cmi.patch59.19 KBcosmicdreams
#31 1496534_29_cmi_user_account.patch10.76 KBcosmicdreams
#29 1496534_29_cmi_user_account.patch10.76 KBcosmicdreams
#24 1496534_24_user_account.patch10.88 KBcosmicdreams
#21 user_config-1496534-21.patch10.81 KBacrollet
#16 user_config-1496534-16.patch11.28 KBacrollet
#15 user_config-1496534-15.patch11.45 KBkim.pepper
#9 user_config-1496534-9.patch10.98 KBkim.pepper
#5 user_config-1496534-5.patch12.09 KBkim.pepper
#3 user-config-1496534-3.patch2.33 KBkim.pepper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jcfiala’s picture

Assigned: Unassigned » jcfiala

I'm interested in taking a swing at this.

kim.pepper’s picture

Hi @jcfiala

If you haven't made any progress on this yet, I'm keen to make a start to some of it.

Kim

kim.pepper’s picture

Status: Active » Needs review
FileSize
2.33 KB

This 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
  • node
  • system
  • user

Status: Needs review » Needs work

The last submitted patch, user-config-1496534-3.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
12.09 KB

I'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

Status: Needs review » Needs work

The last submitted patch, user_config-1496534-5.patch, failed testing.

swentel’s picture

+++ b/core/modules/comment/comment.moduleundefined
@@ -1747,13 +1747,15 @@ function comment_form($form, &$form_state, $comment) {
+    $account_settings_config = config('user.account_settings');
+    $anonymous_name = $account_settings_config->get('anonymous');

Could be written as:

$anonymous_name = config('user.account_settings')->get('anonymous');

I'd do this everywhere in case we only need one single variable.

-29 days to next Drupal core point release.

swentel’s picture

The 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:

  config('user.account_settings')->set('anonymous', $name)->save();

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
10.98 KB

Thanks for the review. I have shortened all the calls to get config, and corrected the test to set the config correctly.

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

The last submitted patch, user_config-1496534-9.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review

#9: user_config-1496534-9.patch queued for re-testing.

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

The last submitted patch, user_config-1496534-9.patch, failed testing.

kim.pepper’s picture

I'm not sure how to progress from here. The UpgradePath tests that are failing are due to directory not existing??

Kim

kim.pepper’s picture

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

@@ -18,6 +18,7 @@ function config_get_config_directory() {
 
   if ($test_prefix = drupal_valid_test_ua()) {
     $path = conf_path() . '/files/simpletest/config_' . $test_prefix;
+    file_prepare_directory($path, FILE_CREATE_DIRECTORY);
   }
kim.pepper’s picture

Status: Needs work » Needs review
FileSize
11.45 KB

Added the above code to this patch to get it to pass tests (works locally).

acrollet’s picture

Assigned: jcfiala » Unassigned
FileSize
11.28 KB

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

marcingy’s picture

Status: Needs review » Needs work

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

xjm’s picture

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

#16: user_config-1496534-16.patch queued for re-testing.

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

The last submitted patch, user_config-1496534-16.patch, failed testing.

swentel’s picture

+++ b/core/includes/config.incundefined
@@ -18,6 +18,7 @@ function config_get_config_directory() {
+    file_prepare_directory($path, FILE_CREATE_DIRECTORY);

This needs to go since that has been committed elsewhere.

acrollet’s picture

Status: Needs work » Needs review
FileSize
10.81 KB

re-rolled without addition in config.inc.

marcingy’s picture

Status: Needs review » Needs work

This needs to load the default config

function user_update_8002() {
  update_variables_to_config('user.account_settings');
}
cosmicdreams’s picture

Load 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

cosmicdreams’s picture

Ah I think I figured it out. here ya go:

cosmicdreams’s picture

Status: Needs work » Needs review

Go testbot

marcingy’s picture

Status: Needs review » Needs work

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

marcingy’s picture

Status: Needs work » Needs review

Cross post :(

gdd’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/user.admin.incundefined
@@ -639,10 +639,20 @@ function user_admin_settings() {
+  $form['#submit'][] = 'user_admin_settings_submit';

Is this line necessary? This should happen automatically through the form api.

+++ b/core/modules/user/user.admin.incundefined
@@ -639,10 +639,20 @@ function user_admin_settings() {
+  $config = config('user.account_settings');
+  $config->set('anonymous', $form_state['values']['anonymous']);
+  $config->save();

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.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
10.76 KB

Ok, 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:

  • site_name
  • site_default_country
  • site_mail
  • maintenance_mode
  • site_slogan
  • site_frontpage

Any others?

Status: Needs review » Needs work

The last submitted patch, 1496534_29_cmi_user_account.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
10.76 KB

Here's the patch that includes all of those variables.

cosmicdreams’s picture

FileSize
59.19 KB

grr, that doesn't sound like the right patch. Trying again:

Status: Needs review » Needs work

The last submitted patch, 1496534_30_account_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
55.94 KB

Here's an attempt to fix the blockers

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

The last submitted patch, 1496534_34_account_cmi.patch, failed testing.

cosmicdreams’s picture

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

#31: 1496534_29_cmi_user_account.patch queued for re-testing.

kim.pepper’s picture

Status: Needs review » Needs work

file_put_contents(sites/default/files/simpletest/391257/config/user.account_settings.xml): failed to open stream: No such file or directory

Looks like the same issue as #17 ?

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
49.12 KB

I'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.

cosmicdreams’s picture

#37 do you mean #16?

Status: Needs review » Needs work

The last submitted patch, 1496534_37_account_cmi.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
48.54 KB

config()->set() needs to be followed by ->save(). implemented in attached patch.

Status: Needs review » Needs work

The last submitted patch, 1496534-41.patch, failed testing.

Anonymous’s picture

woops, forgot the new config file.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
49.44 KB

includes the new config file

Status: Needs review » Needs work

The last submitted patch, 1496534_44_account_cmi.patch, failed testing.

marcingy’s picture

The issue looks like the update hook needs to load system config as well as user config.

cosmicdreams’s picture

@marcingy the system update hook or the user update hook?

marcingy’s picture

My 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).

cosmicdreams’s picture

Yea, it does sound like that, but the fix was committed on April 20th. I dont know why I'm still having this issue.

marcingy’s picture

The change in the issue above means the directory is no longer created by simple test.

cosmicdreams’s picture

Does that mean I need to include code to create it myself?

marcingy’s picture

I think we need to fix the original issue and get that part of the fix in on its own.

sun’s picture

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

+++ b/core/modules/node/node.install
@@ -523,7 +523,7 @@ function _update_7000_node_get_types() {
 function node_update_8000() {
   $front_page = variable_get('site_frontpage');
   if (!isset($front_page)) {
-    variable_set('site_frontpage', 'node');
+    config('system.site')->set('site_frontpage', 'node')->save();

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.

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1434,8 +1434,9 @@ class DrupalWebTestCase extends DrupalTestCase {
+    config('system.site')->set('site_mail', 'simpletest@example.com')
+      ->set('date_default_timezone', date_default_timezone_get())
+      ->save();

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.

+++ b/core/modules/system/config/system.site.xml
@@ -0,0 +1,8 @@
+    <site_frontpage>node</site_frontpage>

The default front page is "user", not "node".

+++ b/core/modules/system/system.admin.inc
@@ -1468,19 +1468,19 @@ function system_site_information_settings() {
+    '#default_value' => config('system.site')->get('site_slogan'),
...
+    '#default_value' => config('system.site')->get('site_mail'),

@@ -1491,7 +1491,7 @@ function system_site_information_settings() {
   $form['front_page']['site_frontpage'] = array(
...
+    '#default_value' => (config('system.site')->get('site_frontpage') != 'user' ? drupal_get_path_alias(config('system.site')->get('site_frontpage')) : ''),

Let's define the config object as a $config variable for the entire form to make this prettier.

cosmicdreams’s picture

awesome feedback. Will get to work on this patch tonight.

cosmicdreams’s picture

FileSize
50.99 KB

Here'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.

cosmicdreams’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1496534_55_account_cmi.patch, failed testing.

cosmicdreams’s picture

FileSize
55.09 KB

Searched 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)

cosmicdreams’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1496534_64_account_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
55.25 KB

OK, 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.

Status: Needs review » Needs work

The last submitted patch, 1496534_61_account_cmi.patch, failed testing.

cosmicdreams’s picture

Cool! 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.

gdd’s picture

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

marcingy’s picture

I'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.

cosmicdreams’s picture

Awesome! 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.

marcingy’s picture

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

cosmicdreams’s picture

Excellent news!

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
54.36 KB

This patch should fix one of the tests: the Theme API test.

Status: Needs review » Needs work

The last submitted patch, 1496534_69_account_cmi.patch, failed testing.

cosmicdreams’s picture

I must have uploaded the wrong patch. The only thing I changed was a test. This shouldn't have caused more failures.

cosmicdreams’s picture

FileSize
55.24 KB

Second try

cosmicdreams’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1496534_71_account_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
55.24 KB

Third try

Status: Needs review » Needs work

The last submitted patch, 1496534_73_account_cmi.patch, failed testing.

cosmicdreams’s picture

OK, 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.

cosmicdreams’s picture

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

#75: 1496534_73_account_cmi.patch queued for re-testing.

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

The last submitted patch, 1496534_73_account_cmi.patch, failed testing.

cosmicdreams’s picture

FileSize
55.69 KB

reroll

cosmicdreams’s picture

Status: Needs work » Needs review
cosmicdreams’s picture

Looks 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!

Status: Needs review » Needs work

The last submitted patch, 1496534_80_account_cmi.patch, failed testing.

cosmicdreams’s picture

FileSize
55.82 KB

I'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.

cosmicdreams’s picture

Status: Needs work » Needs review

go testbot

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

The last submitted patch, 1496534_82_account_cmi.patch, failed testing.

cosmicdreams’s picture

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

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

The last submitted patch, 1496534_82_account_cmi.patch, failed testing.

cosmicdreams’s picture

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

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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
53.28 KB

Doe. here's the patch

Status: Needs review » Needs work

The last submitted patch, 1496534_89_account_cmi.patch, failed testing.

cosmicdreams’s picture

I'm considering if #1496542: Convert site information to config system should be marked as a duplicate.

alexpott’s picture

This 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

  $title = '</title><script type="text/javascript">alert("Title XSS!");</script> & < > " \' ';
 

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.

cosmicdreams’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
52.68 KB

One of the issues with the current patch and the upgrade path tests are these lines from the setUp() method UpgradePathTestCase:

-    $this->variable_set('site_mail', 'simpletest@example.com');
+    config('system.site')->set('site_mail', 'simpletest@example.com')->save();

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.

Status: Needs review » Needs work

The last submitted patch, 1496534_95_account_cmi.patch, failed testing.

alexpott’s picture

I get passes in the upgrade tests apart from the "Language Upgrade Test" with #95 after applying #1541958: Split setUp() into specific sub-methods

marcingy’s picture

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

#95: 1496534_95_account_cmi.patch queued for re-testing.

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

The last submitted patch, 1496534_95_account_cmi.patch, failed testing.

cosmicdreams’s picture

Just 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:

  • Change every get and set of each config item to use the new config system
  • Review if user_admin_settings_submit is the proper way to persist these settings into the new config system
  • If not, decide how system_settings_form_submit in system.module is going to persist settings from multiple forms

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.

gdd’s picture

Unfortunately 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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
22.08 KB

WIP

Status: Needs review » Needs work

The last submitted patch, 1496534_102_account_cmi.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
61.18 KB

Hmmm... this patch is getting humungous... more work in progress...

For some reason in tests the following is not working:

  $config->set('user_register', USER_REGISTER_ADMINISTRATORS_ONLY)->save();
  // Do test 1
  $config->set('user_register', USER_REGISTER_VISITORS)->save();
  // Do test 2

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 :)

cosmicdreams’s picture

alex, 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.

Status: Needs review » Needs work

The last submitted patch, 1496534_104_account_cmi.patch, failed testing.

alexpott’s picture

cosmicdreams 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 :)

alexpott’s picture

Status: Needs review » Needs work
FileSize
70.53 KB

Patch to fix the 3 exceptions...

EDIT: This patch includes debug code

alexpott’s picture

Status: Needs work » Needs review
cosmicdreams’s picture

cool, I'll just assume this passes and review your patch right away.

alexpott’s picture

Borked patch... here's a new one

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
61.11 KB

Here's a first pass on reviewing #111

alexpott’s picture

Status: Needs review » Needs work
FileSize
61.11 KB

Cleanup some whitespace issues I introduced in #111

alexpott’s picture

Status: Needs work » Needs review
cosmicdreams’s picture

Reviewed 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:

<?php
function testRegistrationWithEmailVerification() {
    // Require e-mail verification.
    $config = config('user.account_settings');
    $config->set('user_email_verification', TRUE)->save();

    // Set registration to administrator only.
    $config->set('user_register', USER_REGISTER_ADMINISTRATORS_ONLY)->save();
    $this->drupalGet('user/register');
    $this->assertResponse(403, t('Registration page is inaccessible when only administrators can create accounts.'));

could be

<?php
function testRegistrationWithEmailVerification() {
    // Require e-mail verification.
    $config = config('user.account_settings');
    $config->set('user_email_verification', TRUE);

    // Set registration to administrator only.
    $config->set('user_register', USER_REGISTER_ADMINISTRATORS_ONLY);
    $config->save();
    $this->drupalGet('user/register');
    $this->assertResponse(403, t('Registration page is inaccessible when only administrators can create accounts.'));
alexpott’s picture

Updated patch attached as per #115 and fixed a couple of spaces at end of line issues

Ignore this patch

alexpott’s picture

Had some issues rebasing the patch to latest changes to 8.x

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

The last submitted patch, 1496534_117_account_settings_cmi.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Config novice
aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/user.testundefined
@@ -21,16 +21,19 @@ class UserRegistrationTestCase extends WebTestBase {
+ ¶

trailing whitespace

16 days to next Drupal core point release.

cosmicdreams’s picture

I think I've tracked down that whitespace and fixed it with this patch.

cosmicdreams’s picture

Status: Needs work » Needs review
alexpott’s picture

Yep 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

cosmicdreams’s picture

Is there anything more that needs to be done on this patch other than having a solid review?

alexpott’s picture

Wait 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 :)

wizonesolutions’s picture

Reroll against HEAD, though now out of date I guess.

cosmicdreams’s picture

I don't feel that we need to wait unless you're talking about "Wait a day" because the other patch is going in today.

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

The last submitted patch, 1496534_125_account_cmi.patch, failed testing.

cosmicdreams’s picture

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

#121: 1496534_121_account_cmi.patch queued for re-testing.

gdd’s picture

Status: Needs review » Needs work

- 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

sun’s picture

Issue tags: +API change

All config system conversions are API changes, so tagging as such.

cosmicdreams’s picture

Looking at the config that is used by this module, I think the resultant yml file should be:

anonymous: 'Anonymous'
admin_role: 2
register: 0
email_verification: 'TRUE'
picture:
  default: ''
  dimensions: '85x85'
  file_size: '30'
  guidelines: ''
  path: 'pictures'
  pictures: 1
mail_status:
  activated_notify: TRUE
  blocked_notify: FALSE
  cancelled_notify: FALSE
dagmar’s picture

I'm now sure if this is correct:

admin_role: 2

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

aspilicious’s picture

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

anonymous_text: 'Anonymous'
admin_role: authenticated
can_register: 0
picture:
  default: ''
  dimensions: '85x85'
  enabled: 1
  file_size: '30'
  guidelines: ''
  path: 'pictures'
mail:
   status:
     activated_notify: TRUE
     blocked_notify: FALSE
     cancelled_notify: FALSE
   verification: 'TRUE'
cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
39.94 KB

This patch:

  • renames variable_get('user_pictures') to config('user.settings')->get('picture.enabled')
  • grouped picture and mail config
  • rerolled the patch
  • tracked down even more places where these variables are used and changed them to use the new config system

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.

cosmicdreams’s picture

Ha, 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.

Status: Needs review » Needs work

The last submitted patch, 1496534_132_user_registration_cmi.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
42.67 KB

More improvements

aspilicious’s picture

Even more improvements

Status: Needs review » Needs work

The last submitted patch, 1496534_139_user_registration_cmi.patch, failed testing.

dagmar’s picture

+++ b/core/modules/user/config/user.settings.ymlundefined
@@ -0,0 +1,18 @@
+admin_role: authenticated

Hm. This means that every autenticated user of the site will be an administrator. Not really secure.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
42.65 KB

This one should fix a bunch of the failures

Status: Needs review » Needs work

The last submitted patch, 1496534_141_user_registration_cmi.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
41.25 KB

Lets try removing the line

admin_role: authenticated 
aspilicious’s picture

More fixes!

Status: Needs review » Needs work

The last submitted patch, 1496534_144_user_registration_cmi.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
60.22 KB

Argh typo...

cosmicdreams’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1496534_147_user_registration_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
59.42 KB

Here's a try to incorporate the feedback I posted above:

This patch will also fail, because of line 3305 of user.module.

aspilicious’s picture

I'm not sure it's that bad to seperate status. Needs more discussion.
Fixed more stuff...

cosmicdreams’s picture

Cool, 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.

aspilicious’s picture

Oh 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

aspilicious’s picture

oh wait... damnit... the $op is status_cancelled and not cancelled.. damnit!

aspilicious’s picture

Final try and now lets wait an hour for the bot lol

dagmar’s picture

Same patch that #155 with changes suggested in #144. It should fix 7 more warnings.

cosmicdreams’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentPreviewTest.phpundefined
@@ -35,8 +35,10 @@ class CommentPreviewTest extends CommentTestBase {
+    $config = config('user.settings');
+    $config->set('signatures', 1);
+    $config->set('picture.enabled', 1);

I 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();

+++ b/core/modules/openid/openid.moduleundefined
@@ -86,7 +86,7 @@ function openid_help($path, $arg) {
+    if (config('user.settings')->get('mail_verification')) {

@@ -218,7 +218,7 @@ function openid_form_user_register_form_alter(&$form, &$form_state) {
+    if (!config('user.settings')->get('mail_verification')) {

@@ -708,7 +708,7 @@ function openid_authentication($response) {
+    if (!config('user.settings')->get('mail_verification') || $account->login) {

incomplete conversion

+++ b/core/modules/system/system.api.phpundefined
@@ -3781,7 +3781,7 @@ function hook_tokens($type, $tokens, array $data = array(), array $options = arr
+          $name = ($node->uid == 0) ? config('user.settings')->get('anonymous') : $node->name;

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.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPictureTest.phpundefined
@@ -80,7 +84,7 @@ class UserPictureTest extends WebTestBase {
+      $style = $config->get('user_picture_style');

Missed this one.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPictureTest.phpundefined
@@ -202,10 +212,12 @@ class UserPictureTest extends WebTestBase {
+      $config->set('picture.dimensions', $test_dim);
+      $config->set('picture.file_size', 0);

Many examples of not using the fluid syntax

+++ b/core/modules/user/user.admin.incundefined
@@ -312,7 +313,7 @@ function user_admin_settings() {
+    '#default_value' => $config->get('mail_verification'),

another missed conversion.

+++ b/core/modules/user/user.admin.incundefined
@@ -524,7 +525,7 @@ function user_admin_settings() {
+    '#default_value' => $config->get('mail.status.activated_notify'),

@@ -559,7 +560,7 @@ function user_admin_settings() {
+    '#default_value' => $config->get('mail.status.blocked_notify'),

@@ -615,7 +616,7 @@ function user_admin_settings() {
+    '#default_value' => $config->get('mail.status.canceled_notify'),

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.

+++ b/core/modules/user/user.admin.incundefined
@@ -639,7 +640,35 @@ function user_admin_settings() {
+    ->set('mail.status.activated_notify', $form_state['values']['user_mail_status_activated_notify'])
+    ->set('mail.status.blocked_notify', $form_state['values']['user_mail_status_blocked_notify'])
+    ->set('mail.status.canceled_notify', $form_state['values']['user_mail_status_canceled_notify']);

+++ b/core/modules/user/user.installundefined
@@ -442,5 +442,30 @@ function user_update_8002() {
+    'user_mail_status_activated_notify' => 'mail.status.activated_notify',
+    'user_mail_status_blocked_notify' => 'mail.status.blocked_notify',
+    'user_mail_status_cancelled_notify' => 'mail.status.cancelled_notify',

We can still group "mail" stuff, but have only one level of items, not two.

What do you think?

+++ b/core/modules/user/user.moduleundefined
@@ -787,7 +789,7 @@ function user_account_form(&$form, &$form_state) {
+  elseif (!$config->get('mail_verification') || $admin) {

Missed conversion

+++ b/core/modules/user/user.moduleundefined
@@ -3299,8 +3302,7 @@ function user_preferred_language($account, $default = NULL) {
+  $notify = config('user.settings')->get('mail.' . $op . '_notify');

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.

+++ b/core/modules/user/user.moduleundefined
@@ -3568,8 +3571,9 @@ function user_register_validate($form, &$form_state) {
+  if (!$config->get('mail_verification') || $admin) {

Missed conversion.

+++ b/core/modules/user/user.moduleundefined
@@ -3608,7 +3612,7 @@ function user_register_submit($form, &$form_state) {
+  elseif (!$admin && !$config->get('mail_verification') && $account->status) {

missed conversion.

+++ b/core/modules/user/user.pages.incundefined
@@ -419,6 +419,7 @@ function user_cancel_confirm_form_submit($form, &$form_state) {
+  $anonymous_name = config('user.settings')->get('anonymous');

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.

+++ b/core/modules/user/user.pages.incundefined
@@ -472,7 +473,7 @@ function user_cancel_confirm($account, $timestamp = 0, $hashed_pass = '') {
+        'user_cancel_notify' => isset($account->data['user_cancel_notify']) ? $account->data['user_cancel_notify'] : config('user.settings')->get('mail_status.canceled_notify'),

Missed conversion

cosmicdreams’s picture

I can make the changes in 157 if that will help progress.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
61.35 KB

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

dagmar’s picture

Ok, 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.

Status: Needs review » Needs work

The last submitted patch, 1496534_159_user_registration_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
59.66 KB

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

sun’s picture

Status: Needs review » Needs work

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

cosmicdreams’s picture

Assigned: Unassigned » cosmicdreams

sun: will do.

aspillicious: I'll mark this on me while I do this then relinquish after I'm done.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
38.27 KB

Pulled out picture config from the CMI patch
found a typo on line 3084 in user.module where we weren't calling the right variable.

Status: Needs review » Needs work

The last submitted patch, 1496534_165_account_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
38.28 KB

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

Status: Needs review » Needs work

The last submitted patch, 1496534_167_account_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
42.51 KB

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

Status: Needs review » Needs work

The last submitted patch, 1496534_169_account_cmi.patch, failed testing.

alexpott’s picture

The patch attached fixes some of the test failures above.

This change in #169 from variable to CMI was not equivalent.

+++ b/core/modules/user/user.moduleundefined
@@ -3299,8 +3300,7 @@ function user_preferred_language($account, $default = NULL) {
 function _user_mail_notify($op, $account, $language = NULL) {
   // By default, we always notify except for canceled and blocked.
-  $default_notify = ($op != 'status_canceled' && $op != 'status_blocked');
-  $notify = variable_get('user_mail_' . $op . '_notify', $default_notify);
+  $notify = config('user.settings')->get('mail.' . $op . '_notify');
   if ($notify) {
     $params['account'] = $account;
alexpott’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1496534_171_account_cmi.patch, failed testing.

alexpott’s picture

The single test failure is because contact_form_user_admin_settings_alter() adds the variable contact_default_status to the user_admin_settings form which is no longer being saved once the form is converted to CMI.

alexpott’s picture

Status: Needs work » Needs review
FileSize
47.27 KB
4.47 KB

The 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

alexpott’s picture

Fix missing line at end of file.

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/contact/contact.module
@@ -250,10 +250,16 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+  array_unshift($form['#submit'], 'contact_form_user_admin_settings_submit');

No need for array_unshift(), just use the regular $form['#submit'][].

+++ b/core/modules/contact/contact.module
@@ -250,10 +250,16 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+function contact_form_user_admin_settings_submit($form, &$form_state) {

Missing phpDoc.

+++ b/core/modules/contact/contact.module
@@ -250,10 +250,16 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+  config('contact.settings')->set('user_contact_default_enabled', $form_state['values']['user_contact_default_enabled'])->save();

Let's use the regular style/syntax for chained methods, please :)

+++ b/core/modules/user/config/user.settings.yml
@@ -0,0 +1,9 @@
+admin_role: 0

Should be an empty string.

+++ b/core/modules/user/config/user.settings.yml
@@ -0,0 +1,9 @@
+register: visitors
...
+  verification: '1'

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.

+++ b/core/modules/user/config/user.settings.yml
@@ -0,0 +1,9 @@
+mail:
+  status_activated_notify: '1'
+  status_blocked_notify: '0'
+  status_cancelled_notify: '0'

I'd suggest to replace "mail" with "notify", and thus, simplify the keys and their usage throughout User module.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.php
@@ -113,11 +115,13 @@ class UserRegistrationTest extends WebTestBase {
-    // Don't require e-mail verification.
...
-    // Allow registration by site visitors without administrator approval.
...
+    // Don't require e-mail verification.
+    // Then, allow registration by site visitors without administrator approval.

@@ -139,10 +143,12 @@ class UserRegistrationTest extends WebTestBase {
     // Allow registration by site visitors without administrator approval.
...
-    // Don't require e-mail verification.
...
+    // Then, don't require e-mail verification.

Let's update and merge these comments a bit more smartly... it's a single operation now :)

+++ b/core/modules/user/user.admin.inc
@@ -257,9 +257,10 @@ function user_admin_account_validate($form, &$form_state) {
+ * @see system_config_form()
...
+function user_admin_settings($form, &$form_state) {

This should point to the submit handler instead of system_config_form().

+++ b/core/modules/user/user.admin.inc
@@ -639,7 +640,23 @@ function user_admin_settings() {
+/**
+ * Save account settings settings.
+ */
+function user_admin_settings_submit($form, $form_state) {

1) The phpDoc summary should be:

"Form submission handler for user_admin_settings()."

2) &$form_state should always be taken by reference.

alexpott’s picture

Status: Needs work » Needs review
FileSize
44.39 KB
20.38 KB

Patch attached:

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.

diff -u b/core/modules/comment/comment.module b/core/modules/comment/comment.module
--- b/core/modules/comment/comment.module
+++ b/core/modules/comment/comment.module
@@ -2212,7 +2212,7 @@
         $destination = array('destination' => "node/$node->nid#comment-form");
       }

-      if (config('user.settings')->get('register')) {
+      if (config('user.settings')->get('register') != USER_REGISTER_ADMINISTRATORS_ONLY) {
aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/config/user.settings.ymlundefined
@@ -0,0 +1,9 @@
+notify:
+  activated: '1'
+  blocked: '0'
+  cancelled: '0'

Needs to be "status_activated", "status_blocked", ... it looks like this isn't tested?

28 days to next Drupal core point release.

alexpott’s picture

Status: Needs work » Needs review
FileSize
44.41 KB

Good point. The effects of changing the values are tested ... but the values are always set before testing.

Interdiff:

diff -u b/core/modules/user/config/user.settings.yml b/core/modules/user/config/user.settings.yml
--- b/core/modules/user/config/user.settings.yml
+++ b/core/modules/user/config/user.settings.yml
@@ -5,5 +5,5 @@
 notify:
-  activated: '1'
-  blocked: '0'
-  cancelled: '0'
+    status_activated: '1'
+    status_blocked: '0'
+    status_cancelled: '0'
 mail_verification: '1'
aspilicious’s picture

quoting sun

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.

alexpott’s picture

re. #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.

register:
    mode: visitor
    mail_verification: '0'

Or 5 USER_REGISTER_* constants :)

const USER_REGISTER_ADMINISTRATORS_ONLY = 'admin_only';
const USER_REGISTER_VISITORS = 'visitors';
const USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL = 'visitors_admin_approval';
const USER_REGISTER_VISITORS_MAIL_VERIFICATION = 'visitors_mail_verify';
const USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL_MAIL_VERIFICATION = 'visitors_admin_approval_mail_verify';

The patch attached fixes the key order of the settings YAML file.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Still applies and the patch looks good.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/openid/openid.module
@@ -724,7 +724,7 @@ function openid_authentication($response) {
-  elseif (variable_get('user_register', USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL)) {
+  elseif (config('user.settings')->get('register') != USER_REGISTER_ADMINISTRATORS_ONLY) {

I'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. :)

+++ b/core/modules/user/user.module
@@ -3299,9 +3300,8 @@ function user_preferred_language($account, $default = NULL) {
 function _user_mail_notify($op, $account, $language = NULL) {
   // By default, we always notify except for canceled and blocked.
-  $default_notify = ($op != 'status_canceled' && $op != 'status_blocked');
-  $notify = variable_get('user_mail_' . $op . '_notify', $default_notify);
-  if ($notify) {
+  $notify = config('user.settings')->get('notify.' . $op);
+  if ($notify || ($op != 'status_canceled' && $op != 'status_blocked')) {

Hm. The new condition looks wrong to me. Unless I'm mistaken, the entire second OR condition can and should be removed.

ZenDoodles’s picture

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

aspilicious’s picture

Lets see if it blows up...

aspilicious’s picture

Status: Needs work » Needs review
aspilicious’s picture

sigh another try

Status: Needs review » Needs work

The last submitted patch, 1496534_186_account_cmi.patch, failed testing.

alexpott’s picture

Status: Needs review » Needs work

It definitely does blow up if the second condition from the OR is removed.

+++ b/core/modules/user/user.module
@@ -3299,9 +3300,8 @@ function user_preferred_language($account, $default = NULL) {
 function _user_mail_notify($op, $account, $language = NULL) {
   // By default, we always notify except for canceled and blocked.
-  $default_notify = ($op != 'status_canceled' && $op != 'status_blocked');
-  $notify = variable_get('user_mail_' . $op . '_notify', $default_notify);
-  if ($notify) {
+  $notify = config('user.settings')->get('notify.' . $op);
+  if ($notify || ($op != 'status_canceled' && $op != 'status_blocked')) {

_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:

notify:
    cancel_confirm: '1'
    password_reset: '1'
    status_activated: '1'
    status_blocked: '0'
    status_cancelled: '0'
    register_admin_created: '1'
    register_no_approval_required: '1'
    register_pending_approval: '1'

But this seems out-of-scope to me.

alexpott’s picture

Status: Needs work » Needs review
FileSize
44.41 KB

Re-uploading patch from #182 as I think now that @sun's second point is answered this one should be good to go.

sun’s picture

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

alexpott’s picture

Assigned: cosmicdreams » alexpott

Currently re-rolling...

alexpott’s picture

Status: Needs work » Needs review
FileSize
47.06 KB

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

alexpott’s picture

after talking to @sun we're renaming mail_verification to verify_mail

Status: Needs review » Needs work

The last submitted patch, 1496534_195.user_settings.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
46.96 KB
8.48 KB

Patch 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 to verify_mail

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Reviewd this and it all looks good to me...

webchick’s picture

Title: Convert account settings to configuration system » Needs change notification: Convert account settings to configuration system
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Wow, 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:

+notify:
+    cancel_confirm: '1'
+    password_reset: '1'
+    status_activated: '1'
+    status_blocked: '0'
+    status_cancelled: '0'
+    register_admin_created: '1'
+    register_no_approval_required: '1'
+    register_pending_approval: '1'

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.

sun’s picture

Title: Needs change notification: Convert account settings to configuration system » Convert account settings to configuration system
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs issue summary update, -Needs change record

Thanks! :)

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.

Status: Fixed » Closed (fixed)

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

Lars Toomre’s picture

Status: Closed (fixed) » Needs work

Looks 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().

nadavoid’s picture

Addressing the 2 issues raised in #202.

The attached patch converts variable_set('user_mail_status_canceled_notify', TRUE); to config('user.settings')->set('notify.status_canceled', TRUE)->save();.

It appears that testMassUserCancelByAdmin() has already been updated, the instance of variable_set() replaced.

nadavoid’s picture

Status: Needs work » Needs review
andyceo’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed #203 to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.