Select Category via URL in Contact Form

Too bad, as this is so often requested.

This is something that I do in my Contact Forms module so you might like to have a look at that. The other feature that I would like to see (which also is done in Contact Forms) is the hiding of the drop down menu as I think it make the contact form look soooo dorky.

What version are the patches against? HEAD? Isn't that still being changed for Version 6 or does the code freeze mean we can start thinking about 7?

Regards
Geoff

What are we patching against?

No longer works against HEAD.

Reroll.

It's not to good. If my site language not an english or I use special charates in the category name, this make ugly url. (If you want it, use path module)

I made some changes:
- if you view contact url you see category select form item, if you view contact/$cid (contact/1) url you don't see select(it's hidden field), but title change category name.
- contact edit form table add a colon call 'link' and It in the linkt to the category contact page.

I corrected the patch.

I hope it's good patch. (but not included new testcase for new functionality)

Goba and Pasqualle suggest me:
Not use "if(is_numeric($contact)){...}".
Use better page title, and use title callback (it's not dokumented the api.drupal.org)
Add some comment and documentation.

This looks like a good change.

Couple of things - although they're already in the code, the very long array on one line for $rows[] should be split into multiple lines while we're in here. You mentioned a test in #15 - are you planning to write one?

#17 catch
"...the very long array on one line..."
Thanks your suggestion! I split it, and split $header line also.

"You mentioned a test in #15 - are you planning to write one?"
Yes, but I don't know how. ( I stared learning it.)

pp

Over the last three years this function has been provided by my module Contact Forms so it is nice to see it being added to Core. I have a few other things to add which people have requested over the years, then I can retire my module, but I think it would be a good idea to wait until this patch has been added then start a new issue. What do you think?

The features that I have in mind are
1. Setting the subject via URL in Contact Form - which won't take much code
2. Ability to set Additional information for each category or to use default additional information.

In the past I submitted a patch and I don't think it was even looked at so maybe you guys might be interested to test my future patches. If there is interest I will get stuck into them.

Regards
Geoff

1. Setting the subject via URL in Contact Form - which won't take much code

I saw your module. It's nice but it has one problem:

$subject = str_replace( '_' , ' ' , arg(2));

It works only simple English sentence. Doesn't work following:
I'm a hero!
Yes, I want it now!
(and other language e.g. Hungarian)
Érdeklődnék az árvíztűrő tükörfúrógép iránt.

If you see the pathauto module, and i18n-ascii.example.txt, You can see good solution. There is one problem: It's one way method.
I'm a hero! -> im-a-hero -> ???
I'm afraid this problem is not simple!

2. Ability to set Additional information for each category or to use default additional information.
It's a good suggestion! I see it.

pp

#19-2 gpdinoz:
here, but not ready for update. (if you like probe it make a clean install)

here "update ready" patch (thanks Pasqualle)

pp - Thanks for your response.

I tested all your examples and my module copes with them all

These all work!
I'm a hero!
Yes, I want it now!
(and other language e.g. Hungarian)
Érdeklődnék az árvíztűrő tükörfúrógép iránt.

The key is replacing all the spaces with underscores in the path e.g.

contact/2/I'm_a_hero!
contact/2/Yes,_I_want_it_now!
contact/2/Érdeklődnék_az_árvíztűrő_tükörfúrógép_iránt

$subject = str_replace( '_' , ' ' , arg(2)); replaces the underscore with a space and leaves the rest untouched.

I will set up a working exqmple on http://contact.behindthepage.com.au so you can play with it.

Regards
Geoff

Perhaps!

Last night I thought for this.

First test following:

contact/2/I'm a hero!
contact/2/Yes, I want it now!
contact/2/Érdeklődnék az árvíztűrő tükörfúrógép iránt

The underscore isn't necessary! ;)
The browsers are intelligent. Those replace the url string to urlencoded string. Copy the url and look:

contact/2/Érdeklődnék_az_árvíztűrő_tükörfúrógép_iránt
http://contact.behindthepage.com.au/contact/Bill/%C3%89rdekl%C5%91dn%C3%...

contact/2/Érdeklődnék az árvíztűrő tükörfúrógép iránt
http://contact.behindthepage.com.au/contact/Bill/%C3%89rdekl%C5%91dn%C3%...

It's simple. Just add arg(2) to subject's #default_value. But I have a few questions:

1.
I just hope we not open the door for XSS attack. I test it:
contact/2/something">attack
It worked fine, but I not sure It's a one possibility! ;)
I look the http://api.drupal.org/api/function/theme_textfield/7 and I saw check_plain for value. So It isn't problem.

2.
What happen when user want to use / in the subject?

3.
What happen when user add path alias? It isn't work! I probe it and see "Page not found" (perhaps).

Yes you are right you don't need the underscores but I prefer them to the %20 that some browsers insert. The underscores are much tidier but as you say they are not essential.

1. Glad you thought of that. I don't know anything about XSS attacks so I will accept what you say.

2. You can't use / or % or ?. I think people can live with that.

3. Interesting. It seems that when you use URL Alias Drupal loses the ability to search back through the url. This means that they have a choice to either use an alias or use contact/2/subject. Again I think having this feature is worth that compromise.

In my module I use the format contact/Jill Jones/Subject subject instead of contact/2/Subject subject.
i use arg(1) and query the db for any matching category rather than array(1).

The advantages are
1. the Path looks good and is meaningful so people are less likely to want to use an alias (no one has posted an issue about problems with an alias)
2. It is easy to remember the categories so creating links is easy. You don't have to find out the cid
3. if there is no match I then redirect to the standard contact form. Which I think is polite rather than get a 404.

Regards
Geoff

"2. You can't use / or % or ?. I think people can live with that. "
I can live without this whole feature ;) But If this feature exists then It must work good.

My idea is the following:
Use path to parameter. (not user friendly, just coder friendly) If you want a nice url use path module and path_auto module.
I don't know whereof you want to use the subject. If It is countable(not infinite possibility) add path alias. E.g. category 1 is "Sales" and you have three products add three path alias:
contact/1/Prodact_1 -> contact/Sales/Prodact_1
contact/1/Prodact_2 -> contact/Sales/Prodact_2
contact/1/Prodact_3 -> contact/Sales/Prodact_3

If I add path alias to every category setting page I duplicate this functionality!

Let's talk about your coding method (replace every space to underscore)
You change it because some browser change space to %20. %20 is the url encoded space. There are many character which not allowed in the url. Space one of those(and underscore also. Path_auto module uses "-" instead of "_" ;) )! Your code resolves a little piece of the big problem. It isn't a universal solution!

"1. the Path looks good and is meaningful so people are less likely to want to use an alias (no one has posted an issue about problems with an alias)
ok, you win ;)"
I add this functionality to the contact_load. Please test the patch.

"2. It is easy to remember the categories so creating links is easy. You don't have to find out the cid"
Please test my patch. (every category row have a link to specified contact form)

"3. if there is no match I then redirect to the standard contact form. Which I think is polite rather than get a 404."
Use "Default 404 (not found) page" ( admin/settings/error-reporting ) for resolving this problem. If we resolve it in contact module the error message will be missing. I think it isn't a good idea. You have a wrong link everywhere and you haven't got information for it. It's polite for website reader but it isn't polite for website maintainer.

ehh... there is the patch

I have tested this patch and it works well. I think it is excellent.

In response
3. I see your point. 404 is better.

2 & 1. Thanks for listening and you have been very clever with the code. Excellent solution.

Subject - I am not sure what you mean but I think using '#default_value' => arg(2), is the best we can do and is a good solution. I can live without this feature too but I think many people like being able to set the subject via the url.

Maybe "catch" can have a look at this again. I think it is ready for core. (IMHO)

Excellent work Palócz.

Regards
Geoff

Here's a first pass at the patch, I've not tried it yet.

A few minor things:

+ '#title' => t('Additional infromation for this category'), -- typo.

db_fetch_array(db_query("SELECT * FROM {contact} WHERE category = '%s'", $cid));

-- We should probably rename $cid to something else to reflect the fact it takes a name or a cid now.

-- While we're here, these queries should use the new database layer - i.e.

db_query('SELECT * FROM {contact} WHERE category = :category', array(':category' => $category))->fetchAssoc();

-- also, do we want to make this case-insensitive so you can have 'Category' as the category name, and 'category' in your url?

Since this is primarily a UI patch I'm going to move it to the usability queue for a look there. If you could attach a couple of screenshots for the new admin pages and the contact page itself that'd probably help to get some more reviews.

One last thing - since the text for each individual category is configurable now, is there any need for the default text to be stored as a variable now? I guess if it's optional per-category then we should leave it as is, but worth asking.

Overall it looks like good changes to me, and good to see contact module getting some improvements - for anything other than a single category it's quite unwieldy at the moment. Marking to needs work for some of the nitpicks.

"+ '#title' => t('Additional infromation for this category'), -- typo."
I don't understand it. Please, give me more information.

"-- We should probably rename $cid to something else to reflect the fact it takes a name or a cid now.
-- While we're here, these queries should use the new database layer - i.e."

I corrected it, see the patch.

"-- also, do we want to make this case-insensitive so you can have 'Category' as the category name, and 'category' in your url?"
No. If you want it, you need to make a path alias. ;) (plus, if you use mysql the case-insensitive is working. ;))
It's not just a case-in/sensitive problem. (http://drupal.org/node/183678#comment-1131953)

"One last thing - since the text for each individual category is configurable now, is there any need for the default text to be stored as a variable now? I guess if it's optional per-category then we should leave it as is, but worth asking."

Which text should be shown when the user sees the /contact path? I think default text is necessary.

"If you could attach a couple of screenshots for the new admin pages and the contact page itself that'd probably help to get some more reviews."

I created the screencast:
http://www.viddler.com/explore/tanarurkerem/videos/7/

Thank's catch, for your suggestions.

Thanks pp, the typo was "infromation for this category" - should be "information for this category".

Couple more things -

+    array('data' => t('Operations'),
+    'colspan' => 2),

The array should either be on one line, or it needs the 'data' split to a new line and more indentation.

Thanks!
I corrected it.

"Couple more things - The array should either be on one line, or it needs the 'data' split to a new line and more indentation."

Ok, but I just copy and paste this line from another place in the contact module. Do you suggest me to correct all the similar typos in this patch?

pp

corrected this one line.

Hi Palócz,

I think it would be good to have wildcards in the additional information so people could have a message like " Please leave Jill Jones a message" . It could be done something like this.

In contact_mail_page just after $information is rerieved

if (is_array($contact) && trim($contact['information']) != '') {
      $information = $contact['information'];
    }
    else {
      $information = variable_get('contact_form_information', t('You can leave a message using the contact form below.'));
    }
    // new code with !category as wildcard
    if (count($categories) == 1){
      $category = array_values($category);
      $information = str_replace( '!category', array_shift($category), $information);
    }

If you approve I will add this to the patch.

Regards
Geoff

The video looks great! I think these are nice imrovements on the user interface (without looking at the code itself).

This line is just a cut and paste, but we should convert to the new database layer while we're in here:

+    $result = db_query('SELECT cid, category, selected FROM {contact} ORDER BY weight, category');
+    while ($category = db_fetch_object($result)) {

You can just do foreach ($result as $record) instead of the while loop, db_fetch_object() is deprecated.

I'll try to have another pass through to see if there's any more nitpicks, overall it looks pretty decent, and like Gabor says, the screencast is very nice.

Catch,

"You can just do foreach ($result as $record) instead of the while loop, db_fetch_object() is deprecated."
I found it two places and I corrected it.

pp

Hi Geoff,

What happen when we look the /contact path?
If I use: "Please leave !category a message"
The "Jill Jones"'s contact forms work good "Please leave Jill Jones a message", but site wide contact forms work wrong: "Please leave !category a message"

I'm afraid it's not a good idea.

pp

I think the !contact token could be left to another issue - there's already a few changes in here.

One more nitpick:

+/**
+ * Implementation of hook_update_N().
+ */
+function contact_update_7000() {
+  $ret = array();
+  db_add_field($ret, 'contact', 'information',
+      array(
+        'type' => 'text',
+        'not null' => TRUE,
+        'size' => 'big',
+        'description' => 'Additional information.',
+        'initial' => '',
+      ));
+  return $ret;
+}

The array is indented four spaces instead of two. Otherwise this looks good to me - should go back to CNR once that's cleared up to get some more eyes on it.

Hi Catch,

Thanks!

"The array is indented four spaces instead of two."
I corrected it.

quick code review:

'#description' => t("Leave empty if you want to use default"),

in the description field the sentence should end with a dot.

$information = variable_get('contact_form_information', t('You can leave a message using the contact form below.'));

As I see this is original functionality, but I don't like storing a dynamic string into the variables table. It won't work with multilingual sites. But I do not have a solution for this..

+ * @param $contact
+ *   A keyed array ...
...
function contact_mail_page($form_state, $contact = NULL) {
...
if (is_array($contact) ...

I don't like when the function parameter type can't be specified. So is it an array or not? If yes then a type hinting it would be better. #318016: Type hint array parameters

function contact_load($category) {
  if (is_numeric($category)) {
     ...
  }
  else

And I am hesitant with this function also as the function parameter $category can be anything..

crosspost

@Pasqualle: i18n.module does have support for multilingual variables - and it is too easy to use that people often overlook it, besides it is well documented in the handbook.

Corrected the patch. ( contact.admin.inc changed and I don't update it)

Pasqualle,
Thanks!
I corrected "description" and "type hintig".

pp

another quick look:
this should be chaged

  $form['information'] = array('#type' => 'textarea',

to

  $form['information'] = array(
    '#type' => 'textarea',

function contact_load

$cid_or_category_name

I would rather break the API and support only 1 variable.. I am not familiar with contact module, can you explain when we need cid and when category_name. Could it be done with supporting only 1 variable?

Pasqualle,
Every line in contact_admin_edit function looks like similar.

  $form['category'] = array('#type' => 'textfield',
    '#title' => t('Category'),
...
  $form['recipients'] = array('#type' => 'textarea',
    '#title' => t('Recipients'),
...
  $form['reply'] = array('#type' => 'textarea',
    '#title' => t('Auto-reply'),
...
  $form['weight'] = array('#type' => 'weight',
    '#title' => t('Weight'),

I just follow this style.

contact_load
You can use cid or category_name. (see, the screencast)
If you have a category which name of "Sales" (and it is a first category -> cid ==1) you can see it two way:
contact/1
contact/Sales (this feature is asked by Geoff)

Where do i place this logic? I think it will bee in the contact_load function.

then a code style cleanup patch for the contact module would be nice in the followup issue..

you can see it two way:
contact/1
contact/Sales

If you can give me an another example where Drupal core supports such feature then I am ok with it. Otherwise my personal opinion that this feature is too much for core, this is just a simple path duplication. I think this can be done with pathauto, which may be included in D7 core..

And I am not fan of the other feature request either, about getting contact form subject from the url. It is outside of the original issue, and it was not really discussed why do we need such feature..

Since we don't support either contact/1 or contact/sales now, we should indeed decide on just one. Due to the case sensitivity with the categories, I'd go for contact/1 and let path module and/or pathauto handle the name. This would simplify a lot of the code, and you can still do the same thing using only core modules.

I think Pasqualle is also right about the contact form subject - at least we should move that functionality to a different issue.

I don't have time to review the added/changed strings right now, so am tagging this for later. On a quick glance through the patch, I didn't notice any revised text on the contact help page, though I haven't tried this yet so perhaps no changes are necessary.

While I think the extra features are good I can accept

- !contact token to be left to another issue
- Only have paths like "contact/1" - leave "contact/Sales" for another issue or module
- "Subject via URL" leave to another issue or module
- Changes to "Contact Help Page" are probably neccessary - will add it to my to do list

Regards
Geoff

ok, here the new patch.

pp

I think the contact_form_information variable should be removed, as the patch creates a new db column for that in the {contact} table..

in the update_7000 hook changing initial to

'initial' => variable_get('contact_form_information', t('You can leave a message using the contact form below.')); // I am unsure about the t() call..

removes the need for this variable. The variable is used in 4 files, included also in a test file. So there might be some more work with the removal..

And my question is how can I translate the 'information' field? I still do not know what is the direction in D7 about translating dynamic strings stored in different db tables. If someone could point me to the issue, I would like to read about it..

Pasqualle: I think we probably need something similar to tt() from the i18nstrings module in core to handle translation of dynamic strings, I've been doing some work on i18n recently and it 'not being in core' makes for a lot of complexity which shouldn't be necessary.

That shouldn't hold this issue up though, which probably means removing it from this patch as well. I'd suggest posting followup issues and/or refreshing any existing patches/feature requests against contact.module for the stuff that's being stripped out of this issue - it makes it a lot easier to get things in when discrete areas of functionality are handled in distinct issues.

I have nothing against the contact.information db column, I just wanted to point out that there is a little duplication with the contact_form_information variable. After the patch the variable is nothing more than a default value for the new column..

Drupal core already contains dynamic strings which can be translated only with the i18nstrings module. I don't know what is the current situation about adding new string columns to Drupal core, but I do not believe that the good feature should be denied (or held) just because there is no solution for the translation yet.
As I know the tt() function is able to handle such column. I really miss that massive internationalization improvements stopped after the D6 release..

I agree, it is easier to accept a patch concentrating on a simple issue, but it is hard to leave out a nice and requested feature. I rather leave the decision on this matter to someone else as I did not really worked on this patch, just criticized..

Guys, let's not try to solve user provided text translation here. Focus on being able to store categories in one language, and whatever system (either in core or contrib) should take care of the rest.

Back to CNR, I'm sure there's other places where variables are put into user interface text, so we can equally defer this to another issue.

Hi,

I worked hard, and I have not time, sorry.
I am creating the corrected patch now.

Please test it.

I corrected the patch, and add tests.

There are still translation issues with this patch, 'Additional information.' at least. I think catch's comment in #56 was also not given enough thought yet.

Feature requests are for D8 and the Contact project now.

Subscribe

Subscribe

Is this RTBC?

According to #64, and by absence of a technical review on the latest patch, no.

Here is the patch.

This patch contains only "Select category via url" functionality. Because jhodgdon and Bojhan added an alternative way to add "Additional information" (use block :) #599124: Cleanup contact_help()) I droped it.

My English isn't good enough, please help correcting my sentences in the code.

Thanks for your work on this patch! I am marking this as a novice issue to help with the English text. Here are the comments that need to be fixed:

+++ b/modules/contact/contact.moduleundefined
@@ -255,3 +264,10 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+ * Set the page title for the contact pages.

This should use "sets" rather than "set."

+++ b/modules/contact/contact.pages.incundefined
@@ -8,10 +8,13 @@
+ *   A keyed array containing the current data of the contact.

This should be rewritten a bit:

 * @param $contact
 *   (optional) An associative array containing the current data for the contact:
 *   - cid: The contact ID.
 *   - ...(are there other expected keys?  Please check and document.)
 *   Defaults to array().

+++ b/modules/contact/contact.pages.incundefined
@@ -23,16 +26,23 @@ function contact_site_form($form, &$form_state) {
+    // Set an array of the categories if set $contact array.
...
+    // Get an array of the categories and the current default category.

These comments should be rewritten a bit to be clearer.

+++ b/modules/contact/contact.testundefined
@@ -153,6 +153,16 @@ class ContactSitewideTestCase extends DrupalWebTestCase {
+    //Test per category contact form page
...
+    //Test infromation
...
+    //Test form

These comments need to be rewritten. Also, there should be a space between // and the start of the comment, and a period at the end. E.g.:
// This is a well-formatted comment.

+++ b/modules/contact/contact.testundefined
@@ -153,6 +153,16 @@ class ContactSitewideTestCase extends DrupalWebTestCase {
+    $category_id = $this->updateCategory($categories, $category = $this->randomName(16), $recipients_str = implode(',', array($recipients[0], $recipients[1])), $reply = $this->randomName(30), FALSE);

This line is confusing and hard to read.... let's refactor it so the variable definitions, etc. are on their own lines.

+++ b/modules/contact/contact.testundefined
@@ -153,6 +153,16 @@ class ContactSitewideTestCase extends DrupalWebTestCase {
+    $this->assertNoText(t('Category'), t('When view per category contact form page, the category selection element is hidden.'));

This assertion text should also be rewritten.

Note that the Drupal 8.x patch will also need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

xjm thanks for your review and your suggestions.

Rerolled a patch and corrected which I can.

hmm... I attached a wrong patch.. corrected it.

I got some suggestions on drupal.hu irc. I am working on it.

[12:07] wrong comments: http://drupal.org/node/1354#inline
[12:08] t/'. $
[12:08] space hiba
[12:08] ' utan space kell
[12:09] $this->assertText($category, t('Category presents'));
[12:09] present
[12:09] + $default_category = db_query("SELECT cid FROM {contact} WHERE selected = 1")->fetchField();
[12:09] single aposztrof
[12:09] // komment utan . kell

#74: 183678-22.patch queued for re-testing.

There's some changes happening in #1588422: Convert contact categories to configuration system
Suppose we could use machine-name except CID without troubles

rerolling patch from #74- patch applies cleanly to b08c8b4

Let's try this again - rerolling patch from #74- patch applies cleanly to b08c8b4

+++ b/core/modules/contact/contact.pages.incundefined
@@ -8,12 +8,20 @@
+<<<<<<< HEAD
+ * @param $category
+ *   (optional) An associative array containing the current data for the category:
+ *   - cid: The category ID
+ *   - category: The name of the category
+ *
+=======
  * @see contact_menu()
+>>>>>>> 8.x

+++ b/core/modules/contact/contact.testundefined
@@ -1,7 +1,11 @@
+<<<<<<< HEAD
+ * Tests for contact.module.
+=======
  * Tests for the Contact module.
+>>>>>>> 8.x

Oops, looks like some unresolved merge conflicts here. Also, very strange that this isn't failing testbot...

This should remove the merge conflicts.

Actually in review, the tests provided in the above patch are lacking. The assertText will find the category name in the select list when we actually don't want it to. Better to look to ensure that the category name is the title. We can also add another check to make sure the general page is behaving the way it should. The following patch should better handle these cases.

+++ b/core/modules/contact/contact.admin.incundefined
@@ -34,6 +35,7 @@ function contact_category_list() {
+      l(t('contact form'), 'contact/'. $category->cid),

t('Contact form')

+++ b/core/modules/contact/contact.moduleundefined
@@ -257,3 +266,10 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+ * Sets the page title for the contact pages.

This should follow http://drupal.org/node/1354#menu-callback and start with 'Title callback: '

+++ b/core/modules/contact/contact.pages.incundefined
@@ -11,12 +11,17 @@ use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+ *   (optional) An associative array containing the current data for the category:

This looks over 80 characters

+++ b/core/modules/contact/contact.pages.incundefined
@@ -27,16 +32,23 @@ function contact_site_form($form, &$form_state) {
+    // If the function got the $category then the $categories array contains only that.

Over 80 characters

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -160,6 +160,24 @@ class ContactSitewideTest extends WebTestBase {
+    // Test per category contact form page

Missing trailing full stop

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -160,6 +160,24 @@ class ContactSitewideTest extends WebTestBase {
+    // Test infromation

Missing trailing full stop, could probably be expanded

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -160,6 +160,24 @@ class ContactSitewideTest extends WebTestBase {
+    // Test form

Same.

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -160,6 +160,24 @@ class ContactSitewideTest extends WebTestBase {
+    // Test the form without the category id in the url.

ID, URL

Changed code according to last comment.
Needs review.

Excellent, thanks @iflista! That looks pretty good. I have a few, really minor additional recommendations (based on reading the interdiff).

+++ b/core/modules/contact/contact.moduleundefined
@@ -268,7 +268,15 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+ * @param $contact
+ *   Array describing the contact category.
+ * ¶

Bit of trailing whitespace on the blank line here.

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -160,7 +160,7 @@ class ContactSitewideTest extends WebTestBase {
-    // Test per category contact form page
+    // Test per category contact form page.

For this one, I'd make it a little less terse and add a hyphen, like:
"Test the per-category contact form page."

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -168,12 +168,12 @@ class ContactSitewideTest extends WebTestBase {
-    // Test infromation
+    // Test infromation.

"Information" is typoed here (as "infromation"). Actually though, I might change this comment to something like "Confirm that the category name is in the page title."

Oops, one more thing:

+++ b/core/modules/contact/contact.pages.incundefined
@@ -11,12 +11,18 @@ use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+ *   - cid: The category ID
+ *   - category: The name of the category

Both these lines should end in periods as well.

Done.

Some more added nitpicks. In addition to regular stuff, I added in suggestions to type hint where appropriate.

+++ b/core/modules/contact/contact.moduleundefined
@@ -257,3 +266,18 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+ * @param $contact

@param array $contact

+++ b/core/modules/contact/contact.moduleundefined
@@ -257,3 +266,18 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+ *   Array describing the contact category.

This should match the other docblock for contact_site_form, it's much better

+++ b/core/modules/contact/contact.moduleundefined
@@ -257,3 +266,18 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+ * @return

@return string

+++ b/core/modules/contact/contact.moduleundefined
@@ -257,3 +266,18 @@ function contact_form_user_admin_settings_alter(&$form, &$form_state) {
+ *   Returns page title.

Returns the page title.

+++ b/core/modules/contact/contact.pages.incundefined
@@ -11,12 +11,18 @@ use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+ * @param $category

@param array $category

+++ b/core/modules/contact/contact.pages.incundefined
@@ -11,12 +11,18 @@ use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+ *   (optional) An associative array containing the current data for
+ *   the category:
+ *   - cid: The category ID.
+ *   - category: The name of the category.

Somewhere this needs to say "Defaults to an empty array."

+++ b/core/modules/contact/contact.pages.incundefined
@@ -27,16 +33,24 @@ function contact_site_form($form, &$form_state) {
+    // If the function got the $category then the $categories array
+    // contains only that.

I would move this comment above the if(). And the 'got' sounds weird here, maybe "If a category was specified, $categories will contain only that."

+++ b/core/modules/contact/contact.pages.incundefined
@@ -27,16 +33,24 @@ function contact_site_form($form, &$form_state) {
+    // Get an array of the categories and the current default category.

Also move this above the else, and maybe start it with "Otherwise, "

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -160,6 +160,24 @@ class ContactSitewideTest extends WebTestBase {
+    $this->assertRaw('<h1 class="title" id="page-title">Contact '. $category . '</h1>', 'Category name is present as the page title');

Should this "Contact ' . $category" be in t()? If not, still missing a space between "'."

+ *   (optional) An associative array containing the current data for
+ *   the category:
+ *   - cid: The category ID.
+ *   - category: The name of the category.
+ *   Defaults to an empty array.

I grepped code for "Defaults to an empty array." in drupal core, they exists after (optional) description.

+  // If a $category was specified, $categories will contain only that.

Still sounds weird.

So close!

+++ b/core/modules/contact/contact.pages.incundefined
@@ -33,15 +34,14 @@ function contact_site_form($form, &$form_state, $category = array()) {
   }
+	// Otherwise get an array of the categories and the current default category.
   else {

This has a tab.

Otherwise I think this is good to go. Remember to set to "needs review" after your next patch.

Thanks, I forgot. :)

Looks great. Congratulations @iflista on your first core patch! This looks RTBC to me aside from one thing:

Since we're referencing array keys of an array that's returned based on the URL, I wanted to make sure that the argument autoloader properly resulted in a 404 for garbage input (no PHP errors, properly sanitized, etc.). I tested that manually and it worked fine; existing categories are properly loaded and nonexistent ones result in a Page Not Found. I decided to add an additional assertion to the test to that end. Also tweaked the comment formatting a little more.

Thanks @xjm.

Awesome! Thanks @iflista and @xjm

Thanks. Committed/pushed to 8.x, just before we got to 100 comments too.

This addition was lost somehow so needs re-roll

EDIT Probably this was never commited so need re-roll

I grepped the git log and could not find a commit for this issue.

Tests are been modified a bit.

Also contains a fix that could lead to failures

-      $this->addCategory($this->randomName(16), $this->randomName(16), $invalid_recipient, '', FALSE);
+      $this->addCategory(drupal_strtolower($this->randomName(16)), $this->randomName(16), $invalid_recipient, '', FALSE);

I didn't know of this issue — I've done more or less the exact same in:

#1825044: Turn contact form submissions into full-blown Contact Message entities, without storage

As explained over there, that issue standardizes much more and goes much further than simply registering a router path to preselect a category — the router path essentially becomes the category-specific page callback for the form.

This has been fixed via #1825044: Turn contact form submissions into full-blown Contact Message entities, without storage and is part of latest D8 core now.

That said, the actual category-specific forms and their URLs are currently not exposed anywhere, since that functionality was removed from the committed patch.

Let's discuss how we want to handle and expose contact category forms in:
#1849158: Expose contact category-specific forms and/or their URLs somewhere

When it comes to passing the category through arguments in site contact form, why not using hook_form_alter in a custom module?

I tried with such a little piece of code and, for me, it works:

function mymodule_form_alter(&$form, &$form_state, $form_id) {

   if (($form_id == "contact_site_form") && arg(1) ){
     if (is_numeric(arg(1))) {  
	   if (isset($form['cid'])) {
	     $catarray = $form['cid'];		 
	     $options = $catarray['#options'];
	     if (isset($options[arg(1)])) {
		 $form['cid']['#default_value'] = arg(1);
		 } 
	   }
	 }
    }
  
}

I guess it can be made more simple.