Problem/Motivation

Currently contact categories are exposed to own URLs contact/{contact_category} so actually they are not categories but separate forms on their pages (except personal)
So managing categories is confusing.

Proposed resolution

Rename contact_category entity to contact_form and all relates strings/menus
That was suggested in #1997692-33: Regression: category selector missing on site wide contact form

Remaining tasks

discuss and file patch

User interface changes

UI strings

API changes

tbd

Files: 

Comments

Berdir’s picture

The rename makes a lot of sense to me and while it's a huge API change for anyone already using it, I doubt that many are doing that...

sun’s picture

I can see how "Category" might be confusing/ambiguous, but "Page" is very ambiguous, too... (?)

If you were to place a contact form into a block, then the selector would ask you which "Page" you want to use.


I'd recommend to just simply name this a "Contact Form", because that's what it is...

That name would only get a bit confusing at the Entity API level under the hood - a "contact form" has multiple "form modes" etc.pp. - but not sure whether that low-level detail really matters.

crowdcg’s picture

Assigned: Unassigned » crowdcg

@sun good point "form" makes the most sense. Going to start work on this patch today.

Berdir’s picture

Contact form works as well for me.

andypost’s picture

+1 to "contact form"

Berdir’s picture

Title: Rename contact category to contact page » Rename contact category to contact form

Contact form it is.

larowlan’s picture

@crowdcg are you still interested in working on this?

crowdcg’s picture

@larowlan, yes initial patch tomorrow. Have been working on it piecemeal, but think I need to just get a first patch up to iterate and get feedback on.

crowdcg’s picture

Issue summary: View changes
andypost’s picture

@crowdcg is there any progress? I'm planing to fix it

crowdcg’s picture

Assigned: crowdcg » Unassigned
Status: Active » Needs work
FileSize
56.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

@andypost, et al. This is a simple patch, mainly focusing on the UI. I guess my big question is if all the "category" used in code should be changed to use "form" or "contactform" or something else. Also should the file names, classes, functions, etc. all be changed as well. I tried this holistic approach earlier in developing this patch but was unsuccessful. Feedback and direction greatly appreciated, at least this patch makes the interface less confusing.

er.pushpinderrana’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: 2285083-11.patch, failed testing.

andypost’s picture

+++ b/core/modules/contact/tests/drupal-7.contact.database.php
@@ -9,13 +9,13 @@
diff --git a/sites/default/settings.php b/sites/default/settings.php

diff --git a/sites/default/settings.php b/sites/default/settings.php
new file mode 100755

this breaks setup

crowdcg’s picture

FileSize
29.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,178 pass(es), 10 fail(s), and 0 exception(s). View

Ouch, that's what I get for submitting that so late at night. Patch that doesn't break setup attached.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: 2285083-15.patch, failed testing.

Berdir’s picture

Yes, the entity type should be renamed to contact_form, form for the config prefix.

Not sure about the class name, just naming it "Form" seems like a bad idea, so possibly "ContactForm" ?

crowdcg’s picture

Assigned: Unassigned » crowdcg

@berdir, thanks for the direction. Agreed on the second point as well. Will work on another patch incorporating those suggestions.

sun’s picture

  1. +++ b/core/modules/contact/config/schema/contact.schema.yml
     contact.category.*:
    
    @@ -29,7 +29,7 @@ contact.settings:
         default_category:
    
    +++ b/core/modules/contact/contact.local_actions.yml
     contact.category_add:
    
    +++ b/core/modules/contact/contact.menu_links.yml
     contact.category_list:
    
    +++ b/core/modules/contact/contact.routing.yml
         _entity_list: 'contact_category'
         _entity_form: 'contact_category.add'
         _entity_form: 'contact_category.edit'
    ...
     contact.site_page_category:
       path: '/contact/{contact_category}'
    ...
       requirements:
         _entity_access: 'contact_category.view'
    
    +++ b/core/modules/contact/src/Entity/Message.php
       public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
         $fields['category'] = FieldDefinition::create('entity_reference')
    

    Hm. Don't we need to update all of these, too?

  2. -      '#title' => t('Make this the default category.'),
    +      '#title' => t('Make this the default form.'),
    

    (and elsewhere)

    A straight s/category/form/g replacement results in a lack of context in the UI strings. They should be replaced with "contact form" instead (which is a unique term).

  3.    public function buildHeader() {
    -    $header['category'] = t('Category');
    +    $header['category'] = t('Form');
    

    The administrative listing is the only case where "form" may not be accurate, or at least results in a suboptimal language/presentation. Changing it into "Contact form" won't improve it either...

    We may want to ignore this issue; just wanted to point it out.

  4. +++ b/core/modules/contact/src/Controller/ContactController.php
    -   *   contact category form.
    +   *   contact form form.
    

    Duplicate "form".

  5. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
    @@ -69,13 +69,13 @@ function testSiteWideContact() {
    +    // Delete old fiorms to ensure that new forms are used.
    

    Typo in "fiorms"

crowdcg’s picture

Assigned: crowdcg » Unassigned
Status: Needs work » Needs review
FileSize
65.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2285083-21.patch. Unable to apply patch. See the log in the details link for more information. View
52.28 KB

Thanks @sun! Resolved all five I believe in this patch.

Status: Needs review » Needs work

The last submitted patch, 21: 2285083-21.patch, failed testing.

crowdcg’s picture

Status: Needs work » Needs review
FileSize
65.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,441 pass(es), 260 fail(s), and 18 exception(s). View
52.28 KB

Need to pay more attention...trying again!

Status: Needs review » Needs work

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

crowdcg’s picture

Status: Needs work » Needs review
FileSize
76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2285083-25.patch. Unable to apply patch. See the log in the details link for more information. View

Hopefully this patch resolves most of the errors and exceptions.

Status: Needs review » Needs work

The last submitted patch, 25: 2285083-25.patch, failed testing.

crowdcg’s picture

Status: Needs work » Needs review
FileSize
75.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,326 pass(es), 264 fail(s), and 30 exception(s). View

One more time with feeling!

Status: Needs review » Needs work

The last submitted patch, 27: 2285083-27.patch, failed testing.

Berdir’s picture

  1. +++ b/core/modules/contact/config/schema/contact.schema.yml
    @@ -1,8 +1,8 @@
    -contact.category.*:
    +contact.form.*:
    
    +++ b/core/modules/contact/src/Entity/ContactForm.php
    @@ -0,0 +1,80 @@
    + *   id = "contact_form",
    ...
    + *   config_prefix = "contact_form",
    

    The schema doesn't match with the config_prefix.

    Not 100% sure, but I do think that a config prefix of "config_form" is better than just form (which means you can leave out the explicit definition, as the default is the entity type ID anyway). So update the schema and the default config files to contact.contact_form.

  2. +++ b/core/modules/contact/src/ContactForm.php
    @@ -0,0 +1,125 @@
    +class ContactForm extends EntityForm {
    

    I guess this is really a ContactFormForm class now? A form for editing contact forms :) Or maybe ContactFormEditForm?

    Also, please set up your git configuration to better track renames, look for renames = copies in https://www.drupal.org/documentation/git/configure. Makes the patch much smaller and easier to review.

Berdir’s picture

1. That said, form is probably not more generic than "type", so that might work too and is possibly easier to change. @larowlan, @andypost?

sun’s picture

"type" isn't really clear on what it is. A [contact] "form" is a form. A [contact] type is… a "friend", "co-worker", etc category. — Let's avoid the "node type" trap (double-meaningless).

I think we should keep "contact form" in UI strings. However, if "form" presents an issue at the code level, then alternative synonyms are possible - whichever synonym we choose though, it should transport the same meaning. E.g.: channel, funnel, tunnel, connection, junction, etc.

Berdir’s picture

I think you misunderstood me :)

I did not mean to change form to type, I'm very +1 on form, the UI strings, entity class and entity type ID should be contact form/contact_form/ContactForm.

I'm *only* wondering if the config prefix should be config_form or just form. form seems very generic, but I was just making the point that if node.type is OK (and custom_block.type, and comment.type), then contact.form is probably OK too.

crowdcg’s picture

@Berdir, thank you for the feedback and helpful link to git configuration. I've adjusted my .gitconfig and .gitignore, so patches should be smoother from here on out!

1. I've updated the contact.schema.yml, but just so I'm clear, the files contact.form.* under contact/config/install should also now be contact.contact_form.* correct?

2. Personally, I think ContactFormEditForm is clearer than ContactFormForm even if it is more verbose :)

Berdir’s picture

1. Yeah, whether we chose to go with contact_form or just form, the schema, default config files and config_prefix/id in the entity type annotation all need to align. Then hopefully the test fails will go down :)

2. Yeah, the only problem with that is that form would also be used for adding contact forms :)

sun’s picture

Sounds like we really need a different noun under the hood. "form" is definitely problematic. And before renaming it to "type", I'd rather keep the current "category"... :P

We also need to watch out: English is both a colorful but also poor language, so it's easy to fall into the trap of using words that don't exist, don't have a meaning, and worse, which are not translatable into other languages.

How about this? — contact point

In essence, we're talking about a thing that defines a communication recipient and available information that may be communicated to the recipient for a certain purpose. The fact that communication happens through a web form is not really relevant (and might not necessarily be true, who knows…?). We're defining a possible point of contact, or contact point.

andypost’s picture

Suppose #30 exposes good idea to unify this to type as all over core does.

This is a really type of contact that accessed via message form

larowlan’s picture

I'm not fussy, but if I had to choose I'd keep the prefix as contact_form instead of form or type.
+1 to ContactFormEditForm instead of ContactForm for the form.

radiit’s picture

go listen to the discussion

larowlan’s picture

Status: Needs work » Needs review
FileSize
5.02 KB
67.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,310 pass(es), 183 fail(s), and 60 exception(s). View

Lets see whats still broken

larowlan’s picture

FileSize
1.45 KB
67.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,453 pass(es), 14 fail(s), and 8 exception(s). View

Local tests came back

Status: Needs review » Needs work

The last submitted patch, 40: contact-rename-2285083.40.patch, failed testing.

The last submitted patch, 39: contact-rename-2285083.39.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
4.38 KB
69.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,518 pass(es). View
andypost’s picture

FileSize
7.64 KB
69.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,163 pass(es). View

Re-roll of #43 with shorter config prefix s/contact_form/form/
Also

+++ b/core/modules/contact/tests/drupal-7.contact.database.php
@@ -9,22 +9,22 @@
-// Add a custom contact category.
+// Add a custom contact form.
 db_insert('contact')->fields(array(
-  'category',
+  'contact_form',
...
-  'category' => 'Upgrade test',
+  'contact_form' => 'Upgrade test',

this change is wrong, d7 database field name is 'category' so it's not clear how this passes a tests and is this used at all...

PS: pushed to contact-form-2285083-andypost branch of https://www.drupal.org/sandbox/larowlan/2304799

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/contact/contact.links.action.yml
    @@ -1,6 +1,6 @@
    -contact.category_add:
    -  route_name: contact.category_add
    -  title: 'Add category'
    +contact.form_add:
    +  route_name: contact.form_add
    +  title: 'Add form'
    

    Should we use "Add contact form" here and possibly in other UI strings?

  2. +++ b/core/modules/contact/contact.links.menu.yml
    @@ -1,8 +1,8 @@
       parent: system.admin_structure
    -  description: 'Create a system contact form and set up categories for the form to use.'
    -  route_name: contact.category_list
    +  description: 'Create a system contact form and set up other forms to use.'
    +  route_name: contact.form_list
    

    I don't think the new description makes sense? It was about a single system contact form with categories.

    Can be simplified to just "Create and manage contact forms" or something like that.

  3. +++ b/core/modules/contact/contact.routing.yml
    @@ -1,51 +1,51 @@
    -    _entity_form: 'contact_category.add'
    -    _title: 'Add category'
    +    _entity_form: 'contact_form.add'
    +    _title: 'Add form'
    

    Same here.

  4. +++ b/core/modules/contact/contact.routing.yml
    @@ -1,51 +1,51 @@
       defaults:
    -    _title: 'Contact category form'
    +    _title: 'Contact form'
    

    Not sure this makes sense either :)

  5. +++ b/core/modules/contact/src/ContactFormInterface.php
    @@ -0,0 +1,17 @@
    + * Provides an interface defining a contact form entity.
    

    Was like this before I guess, but seems strange, it *is* an interface, i doesn't provide one? Just "Interface for contact form entities" or so?

  6. +++ b/core/modules/contact/src/Entity/ContactForm.php
    @@ -2,7 +2,7 @@
      * @file
    - * Definition of Drupal\contact\Entity\Category.
    + * Definition of Drupal\contact\Entity\ContactForm.
    

    While touching, change to Contains \Drupal.. ?

  7. +++ b/core/modules/contact/src/Form/ContactFormDeleteForm.php
    @@ -42,8 +42,8 @@ public function getConfirmText() {
    -    drupal_set_message($this->t('Category %label has been deleted.', array('%label' => $this->entity->label())));
    -    $this->logger('contact')->notice('Category %label has been deleted.', array('%label' => $this->entity->label()));
    +    drupal_set_message($this->t('Form %label has been deleted.', array('%label' => $this->entity->label())));
    +    $this->logger('contact')->notice('Form %label has been deleted.', array('%label' => $this->entity->label()));
    

    "Contact form"?

  8. +++ b/core/modules/system/src/Tests/Ajax/DialogTest.php
    @@ -57,7 +57,7 @@ public function testDialog() {
           'dialogOptions' => array(
             'modal' => TRUE,
    -        'title' => 'Add category',
    +        'title' => 'Add form',
           ),
    

    Looks like this is just a test, but still, "contact form" ? ;)

andypost’s picture

Status: Needs work » Needs review
FileSize
11.38 KB
69.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,186 pass(es). View

fixed 1, 2, 3, 6, 7
4) renamed to 'Contact' as '/contact' route has, the title for this pages is set in controller
5) There's no suggestions for interfaces https://www.drupal.org/node/1354#classes but checking other entity interfaces they are all used to "Provides an interface"
8) fixed, but this string is not used

andypost’s picture

Berdir’s picture

Looks nice to me, would be good to have an API change approval but this is RTBC IMHO, makes it much clearer what those things actually are.

Only part that might need discussion is the config prefx, I'm ok with either form or contact_form.

andypost’s picture

FileSize
69.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,486 pass(es). View
andypost’s picture

FileSize
69.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch contact-form-2285083-50.patch. Unable to apply patch. See the log in the details link for more information. View

re-roll

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
35.11 KB

rtbc
screenshot from manual testing

crowdcg’s picture

Seems to work fine for me as well. A small nitpick...the term "Label" for the title of the form seems strange to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 50: contact-form-2285083-50.patch, failed testing.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Needs change record updates

I think we need a change record to tell everyone contact categories and now contact forms.

Change records that need an update:

andypost’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

CR draft filed https://www.drupal.org/node/2322527

PS: I will update other CRs after commit

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 093ea79 and pushed to 8.0.x. Thanks!

  • alexpott committed 093ea79 on 8.0.x
    Issue #2285083 by crowdcg, andypost, larowlan: Rename contact category...
andypost’s picture

Status: Fixed » Needs work

The last submitted patch, 50: contact-form-2285083-50.patch, failed testing.

andypost’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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

bedlam’s picture

There's an open issue (issue #2091395) related to the hook_help() text of this module that relates to the changes in this issue. Cross-linking them on the advice of sprint mentor.

bedlam’s picture

Issue tags: +Amsterdam2014