Part of meta issue #1802750: [Meta] Convert configurable data to ConfigEntity system

Converting the contact form settings to the config system would help the Drupal 8 Multilingual Initiative to have a focused, simple but prominent enough example implementation to work with to add internationalization support *afterwards*. Currently there are no subsystems converted that would need internationalization support naturally and I did not find ongoing issues for any of that area either, so let's start with this one to benefit D8MI's discussion on internationalization.

The implementation can take great cues from the image effects conversion that already landed and #1493108: Convert logging and error settings to configuration system. Documentation on API for conversion: http://groups.drupal.org/node/191283

Related issues

Follow-ups
#1816540: Convert both contact forms to FormControllers

CommentFileSizeAuthor
#185 1588422-contact-config-185.patch16.67 KBandypost
#185 1588422-interdiff-185.txt951 bytesandypost
#184 drupal8.contact-config.184.patch16.4 KBsun
#167 contact_delete.png18.75 KBandypost
#163 drupal8.contact-config.163.patch17.2 KBsun
#155 drupal8.contact-config.155.patch13.87 KBsun
#148 1588422-contact-config-148.patch47.54 KBandypost
#148 1588422-interdiff-144vs148.txt648 bytesandypost
#145 1588422-contact-config-144.patch47.53 KBandypost
#145 1588422-interdiff-143vs144.txt3.47 KBandypost
#143 1588422-contact-config-143.patch47.26 KBandypost
#143 1588422-interdiff-139vs143.txt616 bytesandypost
#139 1588422-contact-config-139.patch47.21 KBandypost
#139 1588422-interdiff-136vs139.txt6.65 KBandypost
#137 1588422-contact-config-137.patch46.62 KBandypost
#137 1588422-interdiff-136vs137.txt3.25 KBandypost
#136 contact-1588422-136.patch46.73 KBtim.plunkett
#136 interdiff.txt2.36 KBtim.plunkett
#133 contact-1588422-133.patch46.7 KBtim.plunkett
#133 interdiff.txt5 KBtim.plunkett
#119 1588422-contact-config-119.patch44.9 KBandypost
#119 1588422-interdiff-117vs119.txt831 bytesandypost
#117 1588422-contact-config-117.patch44.75 KBandypost
#117 1588422-interdiff-115vs117.txt658 bytesandypost
#115 1588422-contact-config-115.patch45.39 KBandypost
#115 1588422-interdiff-114vs115.txt394 bytesandypost
#114 1588422-contact-config-114.patch45.78 KBandypost
#114 1588422-interdiff-106vs114.txt2.27 KBandypost
#108 1588422-contact-config-106.patch45.89 KBandypost
#108 1588422-interdiff-105vs106.patch2.38 KBandypost
#105 1588422-contact-config-105.patch45.84 KBandypost
#104 1588422-contact-config-104.patch45.84 KBandypost
#104 1588422-interdiff-95vs104.txt3.31 KBandypost
#95 1588422-contact-config-95.patch45.83 KBandypost
#95 1588422-interdiff-94vs95.txt1.81 KBandypost
#94 1588422-contact-config-94.patch45.85 KBandypost
#94 1588422-interdiff-89vs94.txt12.53 KBandypost
#89 config.contact.89.patch42.09 KBsun
#86 config.contact.86.patch42.08 KBsun
#86 interdiff.txt1.57 KBsun
#84 config.contact.84.patch42.22 KBsun
#84 interdiff.txt3.44 KBsun
#78 config.contact.78.patch42.24 KBsun
#78 interdiff.txt1.68 KBsun
#71 drupal-config_entity_contact-1588422-71.patch42.41 KBdisasm
#71 interdiff.txt2.94 KBdisasm
#68 drupal-config_entity_contact-1588422-68.patch42.39 KBdisasm
#65 drupal-config_entity_contact-1588422-65-do-not-test.patch42.39 KBdisasm
#65 interdiff.txt895 bytesdisasm
#65 drupal-config_entity_contact-1588422-65-merged-with-1668820.patch133.84 KBdisasm
#64 drupal-config_entity_contact-1588422-64-do-not-test.patch42.69 KBdisasm
#61 drupal-config_entity_contact-1588422-62.patch82.92 KBdisasm
#61 interdiff.txt1.88 KBdisasm
#60 drupal-config_entity_contact-1588422-60.patch83.53 KBdisasm
#58 config.entity.contact.58.patch83.58 KBlarowlan
#58 1588422-interdiff.txt841 byteslarowlan
#56 config.entity.contact.55.patch82.77 KBsun
#56 config.contact.55.patch.txt41.95 KBsun
#54 drupal8.contact-categories.fallback.54.patch30.34 KBsun
#51 1588422-contact-config-51.patch31.71 KBalexpott
#51 49-51-interdiff.txt730 bytesalexpott
#49 1588422-contact-config-49.patch31.71 KBalexpott
#45 1588422-contact-config-45.patch39.76 KBdawehner
#39 1588422-contact-config-39.patch36.98 KBandypost
#39 1588422-interdiff-33vs39.txt11.46 KBandypost
#33 1588422-contact-config-33.patch35.79 KBvasi1186
#28 1588422-contact-config-28.patch35.89 KBvasi1186
#26 1588422-contact-config-26.patch34.43 KBvasi1186
#23 1588422-contact-config-23.patch34.07 KBdawehner
#13 1588422-contact-config-13.patch34.36 KBandypost
#13 1588422-contact-config-interdiff-10vs13.txt19.4 KBandypost
#10 1588422-contact-config-10.patch24.54 KBdawehner
#7 1588422-contact-config.patch22.27 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

The only thing that stops this issue is ability of new config system to enumerate sub-values. Example of this could be found at #1479466: Convert IP address blocking to config system
Once you going to get list of contact categories to put them into select...

sun’s picture

Can you clarify what you mean by "enumerate sub-values", please?

The notion of nested keys is already built-in. If "categories.foo" and "categories.bar" are both sub-keys of "categories", then $config()->get('categories') will return an associative array holding the sub-keys foo and bar.

Gábor Hojtsy’s picture

Well, the image module code at least uses these two code snippets to enumerate all items saved or load a specific item respectively:

      $configured_styles = config_get_storage_names_with_prefix('image.style');
      foreach ($configured_styles as $config_name) {
        // @todo Allow to retrieve the name without prefix only.
        $style = image_style_load(str_replace('image.style.', '', $config_name));
        $styles[$style['name']] = $style;
      }
function image_style_load($name) {
  $style = config('image.style.' . $name)->get();
  // ---

To me this looks like properly enough for what contact module needs (AND is an existing pattern used). Of course if there is going to be a better way, both image module and this patch could use it, but I don't see there is anything lacking to get started.

sun’s picture

When talking about enumerating separate configuration objects (instead of sub-keys within a single config object), then #1552396: Convert vocabularies into configuration will add a range of higher-level API functions for that.

Gábor Hojtsy’s picture

So that looks like would convert the above to config_load_all('image.style', 'image_style_load'). It does not seem to change keys or storage, just the surface API, so we could move there in this issue too later if it gets in soon. Looks like that would not make this postponed.

dawehner’s picture

For this conversion it seems to make sense to add a machine name to categories and use that internally.

dawehner’s picture

Status: Active » Needs work
FileSize
22.27 KB

Here is a general description what i think should be done

  • Convert the category config from IDs to machine readable category names
  • Convert the default category from storage in the table to a new config: contact.default_category
  • Convert the categories from the storage in the table to storage in config: contact.category.$name
  • Added some todos for: translatable

Here is just a short start, 50% of the test classes are running, though there are quite a lot of errors remaining.

dawehner’s picture

+++ b/core/modules/contact/contact.pages.incundefined
@@ -26,14 +26,14 @@ function contact_site_form($form, &$form_state) {
+  $categories_names = config_get_storage_names_with_prefix('contact.category');

Didn't saw the last comment of gabor, use his suggested function.

andypost’s picture

Mostly looks good

+++ b/core/modules/contact/contact.admin.inc
@@ -203,6 +239,7 @@ function contact_category_delete_form($form, &$form_state, array $contact) {
+  debug($contact);

some debug dsm() are left here

+++ b/core/modules/contact/contact.admin.inc
@@ -220,9 +257,8 @@ function contact_category_delete_form($form, &$form_state, array $contact) {
+  dsm($contact);

same

+++ b/core/modules/contact/contact.install
@@ -6,80 +6,6 @@
 /**
- * Implements hook_schema().
- */
-function contact_schema() {
-  $schema['contact'] = array(

Suppose we should drop useless table in hook_update_8000

+++ b/core/modules/contact/contact.pages.inc
@@ -134,7 +134,9 @@ function contact_site_form_submit($form, &$form_state) {
+  dsm($values['category']);
+  dsm($values['category_name']);
 

dsm

dawehner’s picture

Status: Needs work » Needs review
FileSize
24.54 KB

Some work to get the amount of test failures down. The only missing is that deleting a category doesn't work.

Status: Needs review » Needs work

The last submitted patch, 1588422-contact-config-10.patch, failed testing.

andypost’s picture

+++ b/core/modules/contact/config/contact.category.feedback.xml
@@ -0,0 +1,7 @@
+<?xml version="1.0"?>
+<config>
+  <name>feedback</name>
+  <category>Website feedback</category>
+  <recipients>test@example.com</recipients>
+  <weight>0</weight>
+</config>

this should be done in hook_install() to fill actual the site admin email

+++ b/core/modules/contact/contact.admin.inc
@@ -20,22 +20,21 @@ function contact_category_list() {
-  $categories = db_select('contact', 'c')
-    ->addTag('translatable')
-    ->fields('c', array('cid', 'category', 'recipients', 'selected'))
-    ->orderBy('weight')
-    ->orderBy('category')
-    ->execute()
-    ->fetchAll();
+  // @todo: Translatables.
+  $categories = config_get_storage_names_with_prefix('contact.category');

Also this require sorting, probably we need a _contact_get_categories() because same code used in tests.

+++ b/core/modules/contact/contact.admin.inc
@@ -92,6 +92,23 @@ function contact_category_edit_form($form, &$form_state, array $category = array
+    '#machine_name' => array(
+      'exists' => 'contact_category_exists',

use contact_load($name) no need to have the same function

+++ b/core/modules/contact/contact.admin.inc
@@ -164,24 +176,48 @@ function contact_category_edit_form_validate($form, &$form_state) {
+function contact_category_exists($name) {
+  $config = config('contact.category.' . $name)->get();
+  return !empty($config);
+}

not needed

+++ b/core/modules/contact/contact.install
@@ -6,80 +6,6 @@
-function contact_install() {
-  // Insert a default contact category.
-  db_insert('contact')
-    ->fields(array(
-      'category' => 'Website feedback',
-      'recipients' => variable_get('site_mail', ini_get('sendmail_from')),

site_mail should be populated so how to deal with defaults???

+++ b/core/modules/contact/contact.module
@@ -149,19 +149,15 @@ function _contact_personal_tab_access($account) {
+function contact_load($name) {
+  // @todo: What about translatable?

config should care about translation, see #1448330: [META] Discuss internationalization of configuration

+++ b/core/modules/contact/contact.pages.inc
+++ b/core/modules/contact/contact.pages.inc
@@ -26,14 +26,14 @@ function contact_site_form($form, &$form_state) {

@@ -26,14 +26,14 @@ function contact_site_form($form, &$form_state) {
   }
 
   // Get an array of the categories and the current default category.
-  $categories = db_select('contact', 'c')
-    ->addTag('translatable')
-    ->fields('c', array('cid', 'category'))
-    ->orderBy('weight')
-    ->orderBy('category')
-    ->execute()
-    ->fetchAllKeyed();
-  $default_category = db_query("SELECT cid FROM {contact} WHERE selected = 1")->fetchField();
+  // @todo: Fix translatable.
+  $categories_names = config_get_storage_names_with_prefix('contact.category');
+  $categories = array();
+  foreach ($categories_names as $config_name) {
+    $category = config($config_name)->get();
+    $categories[$category['name']] = $category['category'];
+  }

Let's move this to one function _contact_get_categories() as mentioned above

+++ b/core/modules/contact/contact.test
@@ -278,21 +282,26 @@ class ContactSitewideTestCase extends WebTestBase {
   function getCategories() {
-    $categories = db_query('SELECT cid FROM {contact}')->fetchCol();
+    $categories_names = config_get_storage_names_with_prefix('contact.category');
+    $categories = array();
+    foreach ($categories_names as $name) {
+      $categories[] = str_replace('contact.category.', '', $name);
+    }
+

3rd fetching of categories _contact_get_categories()

andypost’s picture

Status: Needs work » Needs review
FileSize
19.4 KB
34.36 KB

All variables are converted.
Introduced contact_categories() to get all categories
Fixed all tests
Added upgrade path

There's 2 @todo - copied from image module: about caching and str_replace for config prefix

Gábor Hojtsy’s picture

@andypost: re config should care about translation, see #1448330: [META] Discuss internationalization of configuration, I've opened this one specifically because it is a great example to prototype language support on *once* the initial conversion landed in core. It does not care about language now either (minus the translatable tags on queries which does not really help certain aspects of it). So let's get the initial port done and committed first :) Thanks!

andypost’s picture

@Gabor I got IRC talk with @sun about config and he planing to postpone this issue until commit of #1552396: Convert vocabularies into configuration because we have no common approach for converting table-based "content-related-staff" to config.
By the same time initial patch works and we can start experimenting with localization on this to find all troubles that we can't foreseen in theory.

In the current state I got troubles with configAPI by getting a list of objects contact_categories() and loading a config obejct contact_load()

@sun mentioned about new function config_load_all() in #1552396-8: Convert vocabularies into configuration and which accepts a loader function (as I understand) and proposal to move to a config entity storage backend see #1552396-12: Convert vocabularies into configuration

I'm not postponing the issue for further reviews, probably we could convert this to some kind of a light config-entity but it looks like overkill. OTOH we really need some kind of meta-data to convert schema-translatable bits to config flags to mark a key in config as translatable.

+++ b/core/modules/contact/contact.module
@@ -149,19 +149,14 @@ function _contact_personal_tab_access($account) {
+function contact_load($name) {
+  return config('contact.category.' . $name)->get();

First place to apply for language

+++ b/core/modules/contact/contact.module
@@ -254,6 +249,60 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+function contact_categories() {
+  // @todo Configuration must not be statically cached nor cache-system cached.
+  //$categories = &drupal_static(__FUNCTION__);
+  $categories = array();
+
+  // Select the categories we have configured.
+  $configured_categories = config_get_storage_names_with_prefix('contact.category');
+  foreach ($configured_categories as $config_name) {
+    // @todo Allow to retrieve the name without prefix only.
+    $category = contact_load(str_replace('contact.category.', '', $config_name));

Most pain-points here. There's no static cache because it's now configuration (same in image module)
Another issue with $config_name that needs special processing.
Also this function loads a whole config objects in memory and passes out. But for most of contact forms I just need a keyed array name=>category(localized)

Gábor Hojtsy’s picture

@andypost:

I got IRC talk with @sun about config and he planing to postpone this issue until commit of #1552396: Convert vocabularies into configuration because we have no common approach for converting table-based "content-related-staff" to config.
By the same time initial patch works and we can start experimenting with localization on this to find all troubles that we can't foreseen in theory.

For multilingual support, I don't think it makes sense to work against a temporary patch that is going to be reworked in storage strategy / API and is being postponed for another issue. I hoped that going through this one fast can help set up a real ground for discussing multilingual support for config, since otherwise the discussion stalled. We can postpone this and practically postpone the multilingual discussion in parallel then.

OTOH we really need some kind of meta-data to convert schema-translatable bits to config flags to mark a key in config as translatable.

Yeah, I explicitly don't want to mix in language support here. I wanted to have this done in a straight port. There is no language support for contact forms in core, except the markers for translatable in schema/queries, which we can live without for a month or two I think in the dev branch, while we figure out how do we want to do it. If we postpone the whole conversion on figuring this out, we discuss too many issues at once.

CMI has been going for 14 months, and we have 6 months until code freeze. Need to get pragmatic or accept losses.

andypost’s picture

Status: Needs review » Needs work

This patch could be re-titled to introduce a machine names for contact categories anyway.
Probably we need review of @daveraid

@dawehner any reason to disallow changing of machine names?

+++ b/core/modules/contact/contact.admin.inc
@@ -92,6 +88,23 @@ function contact_category_edit_form($form, &$form_state, array $category = array
+    '#machine_name' => array(
+      'exists' => 'contact_load',
+      'source' => array('category'),
+    ),
+    '#disabled' => !empty($category['name']),
+  );

The #disabled seems unnecessary here

Gábor Hojtsy’s picture

Title: Convert contact form settings to configuration system » Convert contact form settings to configuration system, add machine names

There you go.

dawehner’s picture

About the renaming of machine names, this is common on some of the "exportables" in core (filter, menu, fields, but not all of them) that you can't rename them. The reason is that other things might rely on this machine_name to be constant.
For example this improves the deploy situation a lot. I agree though that for contact categories, i can't easily find a use case beside the deploy one.

Gábor Hojtsy’s picture

CMI switched to yaml in the meantime, not XML anymore as in the patch. I think it would make sense to do this port as-is now and then revisit later if we want to introduce an entity API like solution for this. To me that sounds pretty convoluted at least. But having this simple conversion in would let us experiment with language support on a concerete converted subsystem, which we don't have any examples yet that would need language support.

andypost’s picture

YAML it is, all patches needs re-roll...
@Gabor do you think we should leave a todos in patch to mark translation points?

andypost’s picture

Probably category.default should be a "state" not the config as @catch mentioned in #1591412-11: Convert tracker settings to configuration system

dawehner’s picture

Status: Needs work » Needs review
FileSize
34.07 KB

I would think that the default category is configuration, it's a setting which you want to deploy from dev to live systems.

+++ b/core/modules/contact/contact.installundefined
@@ -6,84 +6,58 @@
+      if ($category->category == 'Website feedback') {
+        // Use default name for unchanged category.
+        $category->name = 'feedback';
+      }
+      else {
+        $category->name = 'category_' . $category->cid;

Wouldn't it be possible to use the category name lowercase and replace of spaces. I guess that's a similar problem to the migration
of filters in d7, there also just ids were used.

This patch just converts to yml.

andypost’s picture

use the category name lowercase and replace of spaces

we could only use transliteration module since we have no multilingual machine-name generator at all

@dawehner if you need to migrate a category & its "defaultness" so it make sense to have them in one config bundle, otherwise we get troubles with merging config values

Gábor Hojtsy’s picture

we could only use transliteration module since we have no multilingual machine-name generator at all

As with all places where we need to introduce machine names, we can provide a default implementations and let people override/extend that if they need to. I don't think this should be a show-stopped for introducing machine names.

vasi1186’s picture

Rerolled the patch to the latest version of d8, without any other changes.
From my point of view, the patch seems pretty ok, it does replace the information from the contact table in the new config system, and the tests should also all pass now.

Status: Needs review » Needs work

The last submitted patch, 1588422-contact-config-26.patch, failed testing.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
35.89 KB

Let's try again...

vasi1186’s picture

Issue tags: -D8MI, -sprint

#28: 1588422-contact-config-28.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1588422-contact-config-28.patch, failed testing.

vasi1186’s picture

Status: Needs work » Needs review

#28: 1588422-contact-config-28.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint

The last submitted patch, 1588422-contact-config-28.patch, failed testing.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
35.79 KB

Rerolled the patch to the latest 8.x version.

Gábor Hojtsy’s picture

What is still holding this up? Any complaints against the latest patch?

gdd’s picture

I suspect we are going to resolve #1599554: Tutorial/guidelines for how to convert variables into configuration in the next 24 hours, and that will require a reroll of this, so you might want to hold off until that happens.

Gábor Hojtsy’s picture

@heyrocker: Now four times your estimated time passed. There has not been any comments on #1599554: Tutorial/guidelines for how to convert variables into configuration for 23 days. Are you advocating postponing this issue on that indefinitely?

gdd’s picture

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/contact/contact.admin.incundefined
@@ -138,15 +147,10 @@ function contact_category_edit_form_validate($form, &$form_state) {
+  // When creating a new contact form make sure that the given category is unique.
+  $name = $form_state['values']['name'];
+  if ($form_state['values']['is_new'] && contact_load($name)) {
+    form_set_error('category', t('A contact form with name %name already exists.', array('%name' => $name)));

If machine name has no trim() on value so this require $name = trim($form_state['values']['name']);

+++ b/core/modules/contact/contact.admin.incundefined
@@ -164,22 +168,34 @@ function contact_category_edit_form_validate($form, &$form_state) {
 function contact_category_edit_form_submit($form, &$form_state) {
+  $default_category = config('contact.settings')->get('default_category');
   if ($form_state['values']['selected']) {
-    // Unselect all other contact categories.
-    db_update('contact')
-      ->fields(array('selected' => '0'))
-      ->execute();
+    // Update new default category.
+    config('contact.settings')
+      ->set('default_category', $form_state['values']['name'])
+      ->save();
   }
-
-  if (empty($form_state['values']['cid'])) {
-    drupal_write_record('contact', $form_state['values']);
+  elseif ($default_category == $form_state['values']['name']) {
+    // Set to empty when changed to be not default category.

same here

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.phpundefined
@@ -106,7 +106,9 @@ class ContactPersonalTest extends WebTestBase {
+      ->set('contact_threshold_limit', $flood_limit)

according #1599554: Tutorial/guidelines for how to convert variables into configuration names should be changed without contact_ prefix

andypost’s picture

Status: Needs work » Needs review
FileSize
11.46 KB
36.98 KB

Fixed #38

Gábor Hojtsy’s picture

Anything else left here to do?

andypost’s picture

Suppose it's ready, but still needs review :)

sun’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system

Fixing tag. Wasn't able to find this issue.

+++ b/core/includes/update.inc
@@ -250,18 +250,18 @@ function update_prepare_d8_language() {
-    $language_default = variable_get('language_default');
+    $language_default = variable_get('language_default', array());
...
+    variable_set('language_default', (array) $language_default);

eh? This saves an empty array, if no default language was configured. It is impossible that this is what you want.

+++ b/core/modules/contact/config/contact.category.feedback.yml
@@ -0,0 +1,4 @@
+name: 'feedback'
+category: 'Website feedback'

These should ideally be "id" and "label".

I'm currently trying very very hard to find and introduce a definite solution for configurable thingies over in #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) — hopefully, we'll be able to resolve that in the next few days.

In general, however, I guess you could already start to copy most of the basic concepts from there, especially for the administrative module implementation code, if you'd want.

+++ b/core/modules/contact/config/contact.category.feedback.yml
@@ -0,0 +1,4 @@
+recipients: 'mail@example.com'

It looks like these YAML files have been hand-coded, since string values are not quoted in YAML. Only integers, floats, and anything else that's not a string is quoted.

+++ b/core/modules/contact/config/contact.settings.yml
@@ -0,0 +1,4 @@
+default_status: '1'
+threshold_limit: '5'
+threshold_window: '3600'

1) These settings could really use some more self-descriptive names. It's not clear what "default_status" relates to.

2) The flood threshold settings form a sub-set, so we should use sub-keys; e.g.:

flood:
    - limit: '5'
    - interval: '3600'
+++ b/core/modules/contact/contact.install
@@ -6,84 +6,62 @@
+function contact_install() {
+  config('contact.category.feedback')
+    ->set('recipients', variable_get('site_mail', ini_get('sendmail_from')))
+    ->save();

1) Needs to be converted to config('system.site')->get('mail') now.

2) I'm not sure whether hook_install() isn't invoked too early during initial installation of Drupal. The site e-mail address gets only configured in the last screen of the installer; modules are installed upfront.

...which makes this detail a veeeery interesting use-case ;)
Didn't think about this yet.

+++ b/core/modules/contact/contact.install
@@ -6,84 +6,62 @@
+function contact_update_8000() {
...
+    db_drop_table('contact');

As we haven't converted any configurable thingies yet, there hasn't been a discussion on whether the former database tables should be retained during the upgrade.

Contributed modules that might integrate with Contact module will be disabled during the Drupal core major version upgrade, and only enabled and upgraded at a later point in time.

+++ b/core/modules/contact/contact.pages.inc
@@ -20,22 +20,21 @@ function contact_site_form($form, &$form_state) {
+  $limit = config('contact.settings')->get('threshold_limit');
+  $window = config('contact.settings')->get('threshold_window');

Configuration objects should only be instantiated once within a local scope/context.

Gábor Hojtsy’s picture

Two notes on the more complex questions:

- For the installation question, contact module could react to the installation being done and update its config?
- For removing/not removing the table, contrib module updates always worked with updated cores, it would just be the same here. Don't think we are doing anything new here.

Gábor Hojtsy’s picture

Issue tags: -D8MI, -sprint

I clearly do not have people in D8MI to do "our own" issues even, so moving this off our sprint.

dawehner’s picture

Status: Needs work » Needs review
FileSize
39.76 KB

eh? This saves an empty array, if no default language was configured. It is impossible that this is what you want.

I reverted these changes, because they are out of scope here.

These should ideally be "id" and "label".

Changed the naming all over the module.

1) These settings could really use some more self-descriptive names. It's not clear what "default_status" relates to.

What about "user_contact_default_status"?

2) I'm not sure whether hook_install() isn't invoked too early during initial installation of Drupal. The site e-mail address gets only configured in the last screen of the installer; modules are installed upfront.

This worked before, so i guess that's fine.

Let's keep the table for now.

Status: Needs review » Needs work

The last submitted patch, 1588422-contact-config-45.patch, failed testing.

alexpott’s picture

The patch for #1496534: Convert account settings to configuration system needs to convert the contact_default_status variable. I guess that one of these patches needs to be postponed until the other gets in.

dawehner’s picture

To be honest we should have converted first the settings and on another issue the actual categories as this patch changes so much at the moment.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +API change
FileSize
31.71 KB

Rerolled patch to apply cleanly on 8.x and removed variables in light of #48, #1496534: Convert account settings to configuration system and #1704530: Convert contact module variables to configuration system.

Couldn't create an interdiff for some reason - I think it's because #45 no longer applied cleanly.

Status: Needs review » Needs work

The last submitted patch, 1588422-contact-config-49.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
730 bytes
31.71 KB

Fixed the broken upgrade path.

alexpott’s picture

Whilst reviewing #51 I noticed the comment // Migrate and drop {contact} table in contact_update_8000() however the db_drop_table() was removed due to comments by @sun in #42.

Are we sure that this is the right thing to do? Wouldn't an 8.x version of a module that has been disabled during upgrade use CMI and not the database - and if it does isn't it doing something wrong?

Personally I think that once the schema is removed from contact.install and the table converted to config then the table should be removed because everything that is interacting with contact categories from then on should be doing it through CMI alone.

andypost’s picture

+++ b/core/modules/contact/contact.admin.incundefined
@@ -201,7 +219,7 @@ function contact_category_delete_form($form, &$form_state, array $contact) {
-    t('Are you sure you want to delete %category?', array('%category' => $contact['category'])),
+    t('Are you sure you want to delete %label?', array('%label' => $contact['label'])),

Any reason to break all translations?

EDIT related issue #183678: Select Category via URL in Contact Form

sun’s picture

Assigned: Unassigned » sun
FileSize
30.34 KB

Re-rolled against HEAD. This patch only serves as a "fallback".

Status: Needs review » Needs work

The last submitted patch, drupal8.contact-categories.fallback.54.patch, failed testing.

sun’s picture

Title: Convert contact form settings to configuration system, add machine names » Convert contact categories to configuration system
Status: Needs work » Needs review
FileSize
41.95 KB
82.77 KB

Awesomeness.

First patch also contains the changes from #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc)

Second patch only contains the changes relevant to Contact module, but won't pass without the former changes, so using .txt suffix.

Status: Needs review » Needs work

The last submitted patch, config.entity.contact.55.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
841 bytes
83.58 KB

Attached patch updates Module api test to include config in list of modules.
Interdiff from patch at #56 attached.

Dave Reid’s picture

Something I've noticed, is please do not use the entity URI as an admin page. Entity URIs should be the publicly-visible pages, which does not apply to contact categories, unless the URI is the contact form set to that category.

disasm’s picture

disasm’s picture

I removed the contact_uri function as well as the uri_callback lines from both hook_entity_info to satisfy what Dave Reid commented on.

Status: Needs review » Needs work

The last submitted patch, drupal-config_entity_contact-1588422-62.patch, failed testing.

disasm’s picture

note: the patch larowlan made was created off of sun's patch containing [1668820]. I'm going to take sun's patch without the merge, and apply larowlan's interdiff to get this patch back to being a single one after I finish the patch review I'm currently doing.

disasm’s picture

here's a reroll of 58 based on the correct patch from #56. It will fail testbot because it's missing the stuff dependant on #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc).

disasm’s picture

Status: Needs review » Needs work
Gábor Hojtsy’s picture

disasm’s picture

attached is a reroll. Seeing errors in the last failed test about undefined index config prefix, so this will probably fail testbot.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-config_entity_contact-1588422-68.patch, failed testing.

disasm’s picture

here's a patch adding 'config prefix' to hook_entity_info and fixing entity_get_multiple calls with FALSE as 2nd param.

disasm’s picture

Status: Needs work » Needs review
kbasarab’s picture

Status: Needs review » Needs work
--- /dev/null
+++ b/core/modules/contact/config/contact.category.feedback.ymlundefined

+++ b/core/modules/contact/config/contact.category.feedback.ymlundefined
@@ -0,0 +1,5 @@

@@ -0,0 +1,5 @@
+id: feedback
+label: Website feedback

I was under the impression all YML values had to be strings in quotes. Did I miss this change?

+++ b/core/modules/contact/contact.moduleundefined
@@ -147,21 +149,85 @@ function _contact_personal_tab_access($account) {
+  // @todo Make this less ugly.
+  $id = substr($name, strlen(Category::getConfigPrefix()) + 1);
+  $category = entity_load('contact_category', $id);
+
+  $category->original = clone $category;
+  foreach ($old_config->get() as $property => $value) {
+    $category->original->$property = $value;
+  }
+
+  foreach ($new_config->get() as $property => $value) {
+    $category->$property = $value;
+  }

Why not make this less Ugly now? Only thing I can think is combining foreach loops into one but not sure if that is possible or not. Its definitely not the ugliest I've seen. Could maybe do without the @todo.

tim.plunkett’s picture

aspilicious’s picture

Strings don't have to be in quotes, but integers should be. Yeah yeah...

dog
'3'
'12-7'
test

are all strings when importing the .yml file.
Look at the core config files to see how it's used.

cosmicdreams’s picture

Gábor Hojtsy’s picture

@cosmicdreams: there are just some followup cleanup happening there, the infrastructure needed for conversions is in place.

sun’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
42.24 KB

We definitely don't need to wait for #1760358: Provide a way to extract the ID from a config object name here.

Updated for config prefix declaration moved into hook_entity_info().

cosmicdreams’s picture

Wow, I've never seen list() used that way, but i suppose that since the patch went green that's legal. But I'm curious why that's better taking the third item of the array that explode produces.

I find this kind of use for list to be confusing. My knee-jerk reaction was, "What's two purpose-less commas here for?"

Robin Millette’s picture

I use list() = explode() all the time. A comment would help, something explicit like

// $id is found after the second '.' in $name
sun’s picture

The exact details for how the ID/machine-name extraction is going to work and look like in the future will be figured out in #1760358: Provide a way to extract the ID from a config object name

I will work on simplifying that, but my battle plan is to see more more actual use-cases and code in core first, before drawing conclusions on the best way to combat the problem. It is perfectly possible that we're going to change the entire design of the config import callbacks. Let's please not hold up this patch on that.

This patch looks RTBC to me — but I've authored very large parts of it, so I'm not eligible to mark it as such.

aspilicious’s picture

I would like to have an upgrade path test for this.
Especially for this part:

+    else {
+      $category->id = drupal_strtolower($category->category);
+      $category->id = preg_replace('@[^a-z0-9]@', '_', $category->id);
+    }

Don't want the upgrade path to break because someone chose a funky name as contact category.
Chinese people for example use different characters.

And I would love to have the test to go in with this patch (and not in a followup).

Dave Reid’s picture

Yeah I have a feeling we need to migrate the IDs of these categories based on something like 'category_' . {contact}.cid. This should have tests for the upgrade path included.

sun’s picture

FileSize
3.44 KB
42.22 KB

Ugh. Upgrade path tests are a PITA to write and will surely delay this issue for many weeks. :(

That said, I've changed the module update and applied the standard pattern for everything we're migrating from serial IDs to machine names: Taking over the serial ID as-is as machine name.

I hope that resolves the major concerns with regard to the upgrade path.

Furthermore:

Renamed ConfigurableBase to ConfigEntityBase.
Removed config module dependency by making config module required.
Ensure unique machine names for converted contact categories by taking over serial ID directly.

Status: Needs review » Needs work

The last submitted patch, config.contact.84.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +Configurables
FileSize
1.57 KB
42.08 KB

Fixed bogus dependency resolution for config module.

The remaining part here should be the upgrade path tests... I'm seriously unhappy that we're blocking this issue on that, although I do understand that we need some tests for that, but I actually do not see us writing individual upgrade path tests for every single Configurable that is being converted into configuration. Instead, I think we want to write a single "catch-all" upgrade path test for all possible configurables in Drupal core to verify the raw data conversions from database tables into config objects/files.

dagmar’s picture

#86: config.contact.86.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API change, +Configuration system, +Configurables

The last submitted patch, config.contact.86.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
42.09 KB

Merged in HEAD.

xjm’s picture

Issue tags: +Needs tests

Love it! Reading this patch just reinforces for me what an architectural win configurables are. I haven't reviewed the upgrade path or the test coverage yet, but all the logic looks great.

Note that I disagree with #86--I think that thorough upgrade path test coverage for configurables is essential, since no two are actually stored the same way at present. Given that CMI is radically new from the ground up, it's a huge deal to make sure there aren't unexpected oddities while switching to the new storage. Generic tests also sound like a great idea, but I think module-specific ones are also important.

Tagging for the upgrade path tests so potential contributors can find the issue.

plach’s picture

Aren't we using entity form controllers here?

andypost’s picture

I think we need a common test which should be extended with each converted variable to cmi. Probably there's should be one already.

EDIT @plach is right, this module should be the example of ConfigEntity with Form and multilingual

plach’s picture

I'm not sure whether we will be able to exploit entity forms to translate configuration (I still have to catch-up with the latest progress made by Jose) but I guess we want them anyway. Translatability is certailny not the only benefit involved.

andypost’s picture

Introduced a default entity form controller
changed a path to delete confirmation to more shorter

andypost’s picture

Fixed broken tests and copy/paste errors

plach’s picture

Looks good to me: what about calling it ContactCategoryFormController which makes more clear which is the involved entity?

Edit: Better, since we are in the Contact namespace simply CategoryFormController.

+++ b/core/modules/contact/lib/Drupal/contact/ContactFormController.php
@@ -0,0 +1,136 @@
+    // Update the default category.
+    $contact_config = config('contact.settings');
+    if ($form_state['values']['selected']) {
+      $contact_config
+        ->set('default_category', $form_state['values']['id'])
+        ->save();
+    }
+    // If it was the default category, empty out the setting.
+    elseif ($contact_config->get('default_category') == $form_state['values']['id']) {
+      $contact_config
+        ->clear('default_category')
+        ->save();

Just one question: why are we saving parts of the object in the entity-building phase (before the entity is actually built!) and not in the save submit handler?

yched’s picture

Status: Needs review » Needs work
+++ b/core/modules/contact/lib/Drupal/contact/Category.phpundefined
@@ -0,0 +1,59 @@
+  /**
+   * The category ID.
+   *
+   * @var string
+   */
+  public $id;
+
+  /**
+   * The category UUID.
+   *
+   * @var string
+   */
+  public $uuid;
+
+  /**
+   * The category label.
+   *
+   * @var string
+   */

At least some of those are defined in ConfigEntityBase, right ?
If so, they shouldn't appear here as well.

+++ b/core/modules/contact/lib/Drupal/contact/ContactFormController.phpundefined
@@ -0,0 +1,136 @@
+    drupal_set_message(t('Category %label has been saved.', array('%label' => $category->label())));
+    watchdog('contact', 'Category %label has been saved.', array('%label' => $category->label()), WATCHDOG_NOTICE, l(t('Edit'), 'admin/structure/contact/manage/' . $category->id()));
+
+    $form_state['redirect'] = 'admin/structure/contact';

Not too familiar with EntityFormController yet, but isn't it odd to have i/o and redirect stuff inside a save() method ?

7 days to next Drupal core point release.

plach’s picture

@yched:

Not too familiar with EntityFormController yet, but isn't it odd to have i/o and redirect stuff inside a save() method ?

It's the submit handler for the save action, it has to deal with redirects :)

yched’s picture

Also, that's really not specific to this issue, but my eyes bleed at entity_create('contact_category').
I wished I'd never have to write entity_load('field'), nor a hook_entity_info() entry for 'field_instance'.

I thought we had separated notions of config and content entities, but apparently this got washed away in the reshuffles since then ?
If all those entity types live in a flat space, and when 'field' becomes an entity type next to 'node', I fear the insane cross dependencies and race conditions on rebuilding entity_get_info()...

yched’s picture

@plach: ok - then I guess save() tickles me as a slightly misleading name for a submit handler, but that's not for this patch either.

plach’s picture

IMHO it feels very natural once you get used to it, however I agree this is not the right place to discuss it.

andypost’s picture

Actually most of all core entities are using pattern with delete confirm and all implementations are different, suppose we need a boolean in default controller to allow this functionality.

Contact or ContactCategory - we could rename it latter in follow-up after neverending bikeshedding... as always

Just one question: why are we saving parts of the object in the entity-building phase (before the entity is actually built!) and not in the save submit handler?

+++ b/core/modules/contact/lib/Drupal/contact/ContactFormController.php
@@ -0,0 +1,136 @@
+    unset($form_state['values']['selected']);

This because 'selected' is not a part of entity and should not be in entity-array. OTOH submit could only care about redirect when delete pressed, and all other logic would be moved to save. I made it specially to leave only entity related core for save.

I think a patch is ready to be commited, but we need a bit more reviews, also I suppose here is enough tests for.

Also it needs re-roll after #1785974: Move ConfigEntity into a Core component

plach’s picture

Contact or ContactCategory - we could rename it latter in follow-up after neverending bikeshedding... as always

I don't want to start a bikeshed either, but aiming for consistency since the beginning is not bikeshedding: all the existing form controllers have the entity in their names, or at least the operation in the case of the profile form controller, for instance. I won't insist on this, however.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.31 KB
45.84 KB

Quick re-roll with rename to CategoryFormController
Also moved update of selected category to save() from submit()

andypost’s picture

Merged head, there's a cleanup happens in #500866: [META] remove t() from assert message

Actual change is

     // Test validation of empty category and recipients fields.
     $this->addCategory('', '', '', '', TRUE);
-    $this->assertText(t('Category field is required.'), 'Caught empty category field');
+    $this->assertText(t('Label field is required.'), 'Caught empty category label field');
+    $this->assertText(t('Machine-readable name field is required.'), 'Caught empty category name field');
     $this->assertText(t('Recipients field is required.'), 'Caught empty recipients field.');

EDIT: Needs re-roll because commited #1785974: Move ConfigEntity into a Core component

podarok’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +API change, +Configuration system, +Configurables

The last submitted patch, 1588422-contact-config-105.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
2.38 KB
45.89 KB

Chasing head, fixed namespace after #1785974: Move ConfigEntity into a Core component

Status: Needs review » Needs work

The last submitted patch, 1588422-interdiff-105vs106.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

Wrongly posted interdiff (1588422-interdiff-105vs106.patch) with .patch extension

andypost’s picture

andypost’s picture

Now this entity should be converted to plugin! For example of conversion #1763974-15: Convert entity type info into plugins

tim.plunkett’s picture

#112, that can and should happen after this initial effort. At a minimum, it's blocked on the same issue the issue linked is.

Please proceed as usual, the plugin conversion will be simple after this gets in.

andypost’s picture

Small cleanup patch, no need to use id() and label() in controller

andypost’s picture

And removed dependency on config module

Status: Needs review » Needs work

The last submitted patch, 1588422-contact-config-115.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
658 bytes
44.75 KB

Fixed broken test

cosmicdreams’s picture

Mostly this is RTBC, I have a quibble about the test to delete contacts. But this patch is green and it looks solid. It's also a great example for anyone else who is planning on using configurables.

+++ b/core/modules/contact/contact.installundefined
@@ -95,3 +33,42 @@ function contact_update_8000() {
+function contact_update_8002() {
+  db_drop_table('contact');

haha, YES!

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -290,22 +296,11 @@ function submitContact($name, $mail, $subject, $cid, $message) {
   function deleteCategories() {
-    $categories = $this->getCategories();
-    foreach ($categories as $category) {
-      $category_name = db_query("SELECT category FROM {contact} WHERE cid = :cid", array(':cid' => $category))->fetchField();
-      $this->drupalPost('admin/structure/contact/delete/' . $category, array(), t('Delete'));
-      $this->assertRaw(t('Category %category has been deleted.', array('%category' => $category_name)), 'Category deleted successfully.');
+    $categories = entity_load_multiple('contact_category');
+    foreach ($categories as $id => $category) {
+      $this->drupalPost('admin/structure/contact/delete/' . $id, array(), t('Delete'));
+      $this->assertRaw(t('Category %label has been deleted.', array('%label' => $category->label())), 'Category deleted successfully.');
     }

This is a bit of an indirect test. You're checking to see if you're getting the proper status message after a deletion. It would be better to:

1. prove that a category exists.
2. delete that category
3. prove that the category no longer exists.

andypost’s picture

Added check to make sure that category actually deleted
@cosmicdreams testSiteWideContact() depends on that categories are deleted so:
1. entity_load_multiple()
2. drupalPost()
3. see attached intediff

tim.plunkett’s picture

+++ b/core/modules/contact/contact.moduleundefined
@@ -147,21 +147,87 @@ function _contact_personal_tab_access($account) {
 /**
+ * Implements MODULE_config_import_create().
+ */
+function contact_config_import_create($name, $new_config, $old_config) {
+  if (strpos($name, 'contact.category.') !== 0) {
+    return FALSE;
+  }
+
+  $category = entity_create('contact_category', $new_config->get());
+  $category->save();
+  return TRUE;
+}
+
+/**
+ * Implements MODULE_config_import_change().
+ */
+function contact_config_import_change($name, $new_config, $old_config) {
+  if (strpos($name, 'contact.category.') !== 0) {
+    return FALSE;
+  }
+
+  list(, , $id) = explode('.', $name);
+  $category = entity_load('contact_category', $id);
+
+  $category->original = clone $category;
+  foreach ($old_config->get() as $property => $value) {
+    $category->original->$property = $value;
+  }
+
+  foreach ($new_config->get() as $property => $value) {
+    $category->$property = $value;
+  }
+
+  $category->save();
+  return TRUE;
+}
+
+/**
+ * Implements MODULE_config_import_delete().
+ */
+function contact_config_import_delete($name, $new_config, $old_config) {
+  if (strpos($name, 'contact.category.') !== 0) {
+    return FALSE;
+  }
+
+  list(, , $id) = explode('.', $name);
+  entity_delete_multiple('contact_category', array($id));
+  return TRUE;

What is the reasoning for these hooks?

andypost’s picture

@tim.plunkett as docs says, we should for each config entity hook_config_import_delete

Modules should implement this callback if they manage configuration data (such as image styles, node types, or fields) which needs to be prepared and passed through module API functions to properly handle a configuration change.

EDIT: Suppose we requires this to properly clean caches

Anonymous’s picture

these hooks are necessary to get ConfigStorageController::save() called?

Anonymous’s picture

talked to timplunkett about this in IRC and he was concerned that every module that just needs ConfigStorageController::save() would be implementing the same boilerplate. i guess we could make some helpers like this:


function foo_config_import_create($op, $name, $new_config, $old_config) {
  // work out your entity type from $name
  entity_thingie_config_create_helper($entity_type, $new_config, $old_config);
}

function entity_thingie_config_create_helper($entity_type, $new_config, $old_config) {
  entity_create($entity_type, $new_config->get())->save();
  return TRUE;
}

that wouldn't require any changes to config_import_invoke_owner(), but doesn't save us that much. i'm not sure how to eliminate the need for these hooks without completely changing config_import_invoke_owner().

tim.plunkett’s picture

The same bit of code from #120 is in most of the ConfigEntity issues that have cropped up, and will likely make it into all of them.

So, a rewrite of config_import_invoke_owner() to make that easier would be great, or a helper.

Possibly would be best served as a static method on ConfigStorageController?

andypost’s picture

I think this out of scope this patch and should be done in follow-up. This issue already has 125 comments so I filed #1806178: Remove code duplication for hook_config_import_*() implementations of config entities

Gábor Hojtsy’s picture

I agree with @andypost, getting this in should also help provide plenty more testing data for the multilingual initiative so further cleanup / generalization work would be great to do in a followup. Any other concerns?

sun’s picture

The battle plan was to implement these hooks for a couple of more Configurables, and only afterwards check which parts of the implementations are actually identical, which parts are similar/equal-ish, and which parts need to remain in individual implementations. And yes, this should be discussed in #1806178: Remove code duplication for hook_config_import_*() implementations of config entities - thanks for creating it ahead of time :)

tim.plunkett’s picture

+1 to that, sorry for partially derailing this issue :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

So looks like since all concerns were resolved since @cosmicdreams' review and no other concerns standing, this should be RTBC then.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

This circumvents EntityListController by loading and sorting entities directly, and then adding operations links, instead of providing its own subclass.

In addition, the URLs for these actions are in the form of $prefix/$op/%id, not $prefix/%id/$op, which is inconsistent with the rest of core.

For example, this is Field UI:
admin/structure/types
admin/structure/types/manage/%node_type/edit
admin/structure/types/manage/%node_type/fields
admin/structure/types/manage/%node_type/display
admin/structure/types/manage/%node_type/delete

Meaning that admin/structure/types/manage is the prefix

This has
admin/structure/contact
admin/structure/contact/manage/%contact (for edit)
admin/structure/contact/delete/%contact

The $op is before it, which is incompatible with the operations provided by the list controller.

I would propose
admin/structure/contact/manage/%contact/edit
admin/structure/contact/manage/%contact/delete

----
Much of this confusion is because we never had a proper change notice for ConfigEntity, so each implementation solved problems on its own. I'm going to work on that now.

xjm’s picture

Meta issue for these: #1802750: [Meta] Convert configurable data to ConfigEntity system

Let's get the stuff about how to convert paths to the list controller documented there.

tim.plunkett’s picture

Assigned: sun » tim.plunkett

Assigning to myself for a reroll.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
5 KB
46.7 KB

This is what I meant.

Status: Needs review » Needs work

The last submitted patch, contact-1588422-133.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

tests are broken because off changed admin urls.

This requires new testAdminOverview() testing method to ensure that categories are sorted by their weight

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.36 KB
46.73 KB

I fixed the tests, and also extended the correct class, since ConfigEntityListController::load() calls ConfigEntityBase::sort() for you.

Until the category needs custom sorting other than weights, there's no need for a test.

andypost’s picture

Just a few fixes, should be grean and RTBC

tim.plunkett’s picture

+++ b/core/modules/contact/contact.pages.incundefined
@@ -28,7 +28,7 @@ function contact_site_form($form, &$form_state) {
-  $categories = entity_load_multiple('contact_category');
+  $categories = entity_list_controller('contact_category')->load();
 
   // If there are no categories, do not display the form.
   if (!$categories) {
@@ -41,7 +41,6 @@ function contact_site_form($form, &$form_state) {

@@ -41,7 +41,6 @@ function contact_site_form($form, &$form_state) {
   }
 
   // Prepare array for select options.
-  uasort($categories, 'Drupal\Core\Config\Entity\ConfigEntityBase::sort');

I'm not sure that this is a proper usage of the list controller... Just because it happens to sort it.

+++ b/core/modules/contact/lib/Drupal/contact/CategoryFormController.phpundefined
@@ -60,7 +60,7 @@ public function form(array $form, array &$form_state, EntityInterface $category)
-      '#default_value' => $default_category === $category->id(),
+      '#default_value' => $default_category === $category->id,

This should be reverted.

Also, why wasn't contact_site_form() converted as well?

andypost’s picture

Status: Needs review » Needs work
FileSize
6.65 KB
47.21 KB

So reverted controller and made usage of id() all over formController
Also:
- changed message to be more descriptive about operation (added/updated)
- Changed a execution of parent form method to continue work on d8mi issues in follow-ups

There should be follow-up to make ConfigEntity support enforceIsNew to make UI messages more consistend

andypost’s picture

Status: Needs work » Needs review

Enough for today

sun’s picture

+++ b/core/modules/contact/contact.admin.inc
@@ -34,10 +34,6 @@ function contact_category_add() {
-  $form['id'] = array(
-    '#type' => 'value',
-    '#value' => $category->id(),
-  );

This is required to stay; common for all entity delete confirmation forms: The entity ID is put into form value that uses the key of the entity ID.

sun’s picture

One more thing:

For this patch and Contact module it might be acceptable, but for all other ConfigEntity/Configurable conversion issues, please do NOT perform the EntityListController and EntityFormController migrations in the same patch.

The conversion to configuration is complex enough. As in this patch, that conversion usually requires test changes already. The additional entity system changes are moving huge parts of the code into different locations, making the entire thing impossible to review. Due to the test changes required for the config conversion, that means we cannot trust the test results anymore.

So please do not merge the additional entity system changes into a config system conversion patch. The entity system changes can happily be follow-up issues to the config conversion. Mixing them is scope creep and detrimental for making progress.

Thanks!

andypost’s picture

So reverted back id key of confirm_form()

andypost’s picture

andypost’s picture

Patch and interdiff

podarok’s picture

#145 looks very nice!

tim.plunkett’s picture

Issue tags: -Needs tests
+++ b/core/modules/contact/contact.moduleundefined
@@ -147,21 +147,104 @@ function _contact_personal_tab_access($account) {
+function contact_category_load($id) {
+  return entity_load('contact_category', $id);

+++ b/core/modules/contact/contact.pages.incundefined
@@ -155,7 +140,7 @@ function contact_site_form_submit($form, &$form_state) {
+  $values['category'] = contact_category_load($values['category']);

+++ b/core/modules/contact/lib/Drupal/contact/CategoryFormController.phpundefined
@@ -0,0 +1,148 @@
+        'exists' => 'contact_category_load',

Ideally the wrapper for entity_load would only exist for menu load arguments (see #1757586: Remove MODULE_load*() & Co functions in favor of entity_*() functions) but there is the 'exists' for machine_name too.

Still, the middle call should probably be entity_load()

I'd say this is very very close to RTBC.

andypost’s picture

So here's a change

andypost’s picture

#148: 1588422-contact-config-148.patch queued for re-testing.

podarok’s picture

Priority: Normal » Major

#148 looks good for me
Looks like this very usefull for GTD #1777070: Refactor and clean up source string location handling here (Translating all the config)
making this major
Yet one review for RTBC

podarok’s picture

Status: Needs review » Reviewed & tested by the community

gm
didn`t see #129
making this RTBC

tim.plunkett’s picture

Priority: Major » Normal

This is not major, we have a major meta for them.

But, RTBC +1. Awesome work @andypost et al!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome!! It's SO great to have a D8MI-affecting patch in core, as well as a ConfigEntity! :D

Committed and pushed to 8.x.

andypost’s picture

As it was mentioned above about follow-up for contact forms #1816540: Convert both contact forms to FormControllers

andypost’s picture

Issue summary: View changes

Added related issues

sun’s picture

Status: Fixed » Needs review
FileSize
13.87 KB

Happy to see this in.

But I'm a bit surprised that this got committed. The identical state (or actually a better one) was RTBC two months ago already, but got blocked on missing upgrade path tests. Suddenly, this patch went in without. I surely won't complain though.

What I do complain about are various bogus changes in the committed patch. Fixes attached.

Status: Needs review » Needs work

The last submitted patch, drupal8.contact-config.155.patch, failed testing.

Lars Toomre’s picture

@sun I noticed in your patch in #155 that you are eliminating most assert messages in ContactSitewideTest.php. I am not sure what your reasoning for that is. Could you elaborate?

andypost’s picture

Status: Needs work » Fixed

@sun Please leave the issue fixed and file another follow-ups for:
- default_category + tests
- tests clean-up
- configEntity itself

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -222,6 +222,7 @@ public function create(array $values) {
+    $entity->enforceIsNew();

This does not works for configurables...

+++ b/core/modules/contact/config/contact.settings.ymlundefined
@@ -1,3 +1,4 @@
+default_category: feedback

Magically this was lost...

+++ b/core/modules/contact/lib/Drupal/contact/CategoryFormController.phpundefined
@@ -92,28 +92,12 @@ public function validate(array $form, array &$form_state) {
-   * Overrides Drupal\Core\Entity\EntityFormController::submit().
-   */
-  public function submit(array $form, array &$form_state) {
-    // @todo We should not be calling contact_category_delete_form() from

Let's fix it in follow-up

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -47,7 +47,7 @@ function testSiteWideContact() {
-    $this->assertText(t('The configuration options have been saved.'), 'Setting successfully saved.');
+    $this->assertText(t('The configuration options have been saved.'));

this clean-up needs special follow-up

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -265,7 +265,7 @@ function updateCategory($id, $label, $recipients, $reply, $selected) {
-    $this->drupalPost("admin/structure/contact/manage/$id/edit", $edit, t('Save'));
+    $this->drupalPost('admin/structure/contact/manage/' . $id, $edit, t('Save'));

this broken

xjm’s picture

Test coverage is a gate; I'd agree with marking it Needs work on that. It's not "Test cleanups"; it's "upgrade path tests, period". I'm not going to play issue status tug-of-war, but if not, then that's a major or critical.

@sun's changes in #155 look correct to me, other than the enforceIsNew() which I do not know about, and of course the test failures.

@Lars, for background on why the assertion messages are removed, see #1601146: Allow custom assertion messages using predefined placeholders and #1601202: Make simpletest assertions output both the default and custom message texts. Custom assertion messages that are more vague than the default for the assertion make debugging more difficult. I'm not as adamant on this point as @sun is but I have also had to waste time on unspecific assertion messages.

andypost’s picture

This issue has a lot of comments so I filed #1817182: Add upgrade path tests for contact category config conversion
Let's leave this fixed

webchick’s picture

Status: Fixed » Needs work
Issue tags: +Needs tests

xjm is of course right. Sorry, I was a bit too hasty on this one. I want to keep the commit in so D8MI can make use of it, but marking back to needs work for test coverage (also tagging, so I don't miss that again :)). We can go ahead and incorporate sun's improvements here too.

That said, I'm not sure how good in general we have been doing with making sure these CMI conversions all have proper upgrade test coverage. We might need a major task to go back and clean that up.

webchick’s picture

Status: Needs work » Fixed

Oops. Missed #160. Yeah 100+ comments in is a good point, so we can handle at #1817182: Add upgrade path tests for contact category config conversion I guess. Let's get a follow-up for sun's changes too.

sun’s picture

Status: Fixed » Needs review
FileSize
17.2 KB

That issue is about upgrade path tests. The follow-up patch here is about fixing bogus code that was introduced in this issue.

Let's do ourselves a favor and just fix the mess.

re #158:

  1. $enforceIsNew and enforceIsNew() is fixed in a proper way in this follow-up patch, removing ugly hacks/workarounds from the committed code, which shouldn't have been contained in the first place. Please familiarize yourself with ConfigEntity and ConfigStorageController.
  2. CategoryFormController::submit() is entirely bogus and should have raised plenty of red flags before commit. Please familiarize yourself with EntityFormController.
  3. The contact_menu() definition for router paths to manage categories does not follow the common/standard path layout for entities.

Attached patch gets us back to sanity.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

I think all the improvements from #163 are good, and 2 or 3 more comments in this issue won't hurt anyone :)

xjm’s picture

#163 looks correct to me as well.

xjm’s picture

Issue tags: +Needs change record

We should probably also add to the existing change notifications for each converted ConfigEntity type (similarly to what was done for the entity conversion in http://drupal.org/node/1400186):
http://drupal.org/node/1818734
http://drupal.org/node/1734556
http://drupal.org/node/1799642

andypost’s picture

FileSize
18.75 KB

This patch introduces wrong UI pattern so Delete is displayed as Tab when you edit any category

+++ b/core/modules/contact/contact.admin.incundefined
@@ -16,23 +16,41 @@
+  drupal_set_title(t('Contact form categories'));

Why not move this to hook_menu()

+++ b/core/modules/contact/contact.moduleundefined
@@ -72,17 +72,24 @@ function contact_menu() {
-  $items['admin/structure/contact/manage/%contact_category/edit'] = array(
+  $items['admin/structure/contact/manage/%contact_category'] = array(
     'title' => 'Edit contact category',
-    'page callback' => 'entity_get_form',
+    'page callback' => 'contact_category_edit',
     'page arguments' => array(4),
     'access arguments' => array('administer contact forms'),
+    'file' => 'contact.admin.inc',
+  );
+  $items['admin/structure/contact/manage/%contact_category/edit'] = array(
+    'title' => 'Edit',
+    'type' => MENU_DEFAULT_LOCAL_TASK,
+    'weight' => -10,
   );
   $items['admin/structure/contact/manage/%contact_category/delete'] = array(
-    'title' => 'Delete contact category',
+    'title' => 'Delete',
     'page callback' => 'drupal_get_form',
     'page arguments' => array('contact_category_delete_form', 4),
     'access arguments' => array('administer contact forms'),
+    'type' => MENU_LOCAL_TASK,

This displays Delete tab which was deprecated in D7. DO we wanna to return this back?

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Hmm, not sure about the UI change. @amateescu points out that taxonomy terms and comments have a delete tab.

We should probably use a separate title callback for the PASS_THROUGH rather than drupal_set_title(); that's a good point.

andypost’s picture

As I see all over a core we have Delete tab only for entities that have View tab...

Also about enforceIsNew()

<timplunkett> andypost: because following the original intention for enforceIsNew, its pointless
<timplunkett> andypost: if a node has an NID, it's not new, right?
<andypost> probably, but maybe it's not saved yet
<timplunkett> andypost: but what if a migration wants to create a new node with a specific pre-defined NID? it uses enforceIsNew
<timplunkett> andypost: that whole approach doesn't affect configuration, since its based on machine name
tim.plunkett’s picture

#169 reflects my understanding of enforceIsNew(), and the above patch changes that, and I'm not sure what it means :)

sun’s picture

1) The page title is not the menu link title. Entirely different can of worms. See ContentEntity menu router info and page callbacks for reference.

2) An entity's tab_root normally bundles view, edit, delete, and potentially other tasks. view == edit, the default local task in case of ConfigEntity. We should follow that pattern. And very potentially consider to drop the "Delete" button from the form instead - if anything is misplaced, then it's rather that destructive button in an otherwise constructive interface.

3) enforceIsNew() obviously works only for Configurables that do not have an ID yet. It does not work for Configurables that have an ID already, since a Configurable is singular, unique, and cannot be duplicated.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

There was #183678: Select Category via URL in Contact Form that was commited but lost somehow. In this context tabs makes sense

fago’s picture

andypost: but what if a migration wants to create a new node with a specific pre-defined NID? it uses enforceIsNew
andypost: that whole approach doesn't affect configuration, since its based on machine name

Exactly - enforceIsNew() is not necessary for config, if it's not there yet it will be created with the given machine-name anyway I suppose. Thus, for config the method is unnecessary and can be left aside. Why bother with it?

andypost’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -45,22 +45,12 @@ public function getOriginalID() {
-   * Overrides Entity::isNew().
-   *
-   * EntityInterface::enforceIsNew() is not supported by configuration entities,
-   * since each configuration entity is unique.
-   */
-  final public function isNew() {
-    return !$this->id();
-  }

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -222,6 +222,7 @@ public function create(array $values) {
     $entity = new $class($values, $this->entityType);
+    $entity->enforceIsNew();

+++ b/core/modules/contact/lib/Drupal/contact/CategoryFormController.phpundefined
@@ -92,38 +92,22 @@ public function validate(array $form, array &$form_state) {
   public function save(array $form, array &$form_state) {
     $category = $this->getEntity($form_state);
-    // Property enforceIsNew is not supported by config entity. So this is only
-    // way to make sure that entity is not saved.
-    $is_new = !$category->getOriginalID();
+    $is_new = $category->isNew();
+
     $category->save();
     $id = $category->id();
...
     if ($is_new) {
       drupal_set_message(t('Category %label has been added.', array('%label' => $category->label())));

We need a common approach for entity->save() to detect create/update for config Entities to write watchdog/UI messages

sun’s picture

Status: Reviewed & tested by the community » Postponed

Temporarily putting this on hold. I'm working on #1798944: Convert config_test entity forms to EntityFormController and will introduce massive ConfigEntity CRUD tests there.

xjm’s picture

Title: Convert contact categories to configuration system » Change notification: Convert contact categories to configuration system
Priority: Normal » Critical
Status: Postponed » Active

Well, let's take care of adding to the change notification, at least.

andypost’s picture

I've updated all related issues with links to this issue, suppose we could change title and priority

http://drupal.org/node/1818734
http://drupal.org/node/1734556
http://drupal.org/node/1799642

andypost’s picture

@sun please re-roll #163, we got commited #1798944: Convert config_test entity forms to EntityFormController

Also I think we should remove critical priority because all change notices are updated #177

Gábor Hojtsy’s picture

Title: Change notification: Convert contact categories to configuration system » Convert contact categories to configuration system
Priority: Critical » Normal
Status: Active » Postponed

The updated change notices look relevant to this issue to me.

aspilicious’s picture

Status: Postponed » Active

Don't see any reason why this should be postponed?

Gábor Hojtsy’s picture

Status: Active » Needs review

@aspilicous: it was RTBC and set to postponed in #175, I just moved back into that state, but seems like the issue it was postponed on was committed. If the latest patch still looks good and passes, this should in fact be back to RTBC, not active, as per above discussion (or some patch review or needs work state at least).

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +API change, +Configuration system, +Needs change record, +Configurables

The last submitted patch, drupal8.contact-config.163.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
16.4 KB

Re-rolled against HEAD, and adjusted to new/correct practices.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
951 bytes
16.67 KB

#184 is RTBC, reviewing it I found a minor changes at doc-blocks so same patch + 2 new hunks

Lars Toomre’s picture

@andypost - I think out new practice according to #1487760: [policy, no patch] Decide on documentation standards for namespaced items is to have all namespaced classes start with a fully qualified path. In other words, the class reference is suppose to start with a '\'.

andypost’s picture

@Lars agree on that, but introducing this changes in this patch will make that one much bigger so let's make it in follow-up after #1487760 would be marked as fixed

Status: Reviewed & tested by the community » Needs work
Issue tags: -API change, -Configuration system, -Needs change record, -Configurables

The last submitted patch, 1588422-contact-config-185.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Configuration system, +Needs change record, +Configurables

#185: 1588422-contact-config-185.patch queued for re-testing.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Yes, this is still ready. Thanks for the phpDoc tweak, @andypost!

catch’s picture

Title: Convert contact categories to configuration system » Change notice: Convert contact categories to configuration system
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

The drupal_set_title() calls are irritating. I'd been thinking/ranting about drupal_set_title() at BADCamp so I've gone ahead and opened #1833342: Use #attached (or similar) for the page title.

Otherwise this looks great so committed/pushed to 8.x.

Will need a change notice.

Gábor Hojtsy’s picture

Opened #1834002: Configuration delete operations are all over the place now that this has both a Delete button and tab (like taxonomy terms) while other things don't work like that (they either have one or the other).

swentel’s picture

Title: Change notice: Convert contact categories to configuration system » Convert contact categories to configuration system
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

This issue is listed at #1802750: [Meta] Convert configurable data to ConfigEntity system which states once it's done the issue number should be added to the change notice at http://drupal.org/node/1818734 - which is the case, so I think this can be marked fixed then - feel free to correct me of course.

Status: Fixed » Closed (fixed)

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

yched’s picture

Unless I'm mistaken, the config files created in the upgrade function do not have a uuid entry ?

yched’s picture

Issue summary: View changes

Updated issue summary.