URL Alias Support

Updated OP

I added a new URL alias field to the contact form and test coverage for it.

I rerun the test locally and it works...not sure what testbot needs to make this to work.

+++ b/contact_storage.module
@@ -46,21 +52,38 @@ function contact_storage_form_contact_form_form_alter(&$form, FormStateInterface
+  if(strcmp($form_state->getValue('contact_storage_url_alias'),"") && (strpos($form_state->getValue('contact_storage_url_alias'), '/') !== 0)) {
+    $form_state->setError($form['contact_storage_url_alias'], 'The alias path has to start with a slash.');
+  } else if ($form_state->hasValue('contact_storage_url_alias') && (strpos($form_state->getValue('contact_storage_url_alias'), '/') === 0)) {
+    $filename = $form_state->getValue('id');
+    \Drupal::service('path.alias_storage')->save('/contact/' .$filename, $form_state->getValue('contact_storage_url_alias'));

Suppose here should be a language!
hasValue() makes sense first!
compare with "/" only once

The test fails are likely due to a change in 8.2.x. Update to the latest versio and it will fail like that for you as well. Open a separate issue about fixing that.

1. +++ b/contact_storage.module
   @@ -39,6 +39,12 @@ function contact_storage_form_contact_form_form_alter(&$form, FormStateInterface
   +  $form['contact_storage_url_alias'] = [
   +    '#type' => 'textfield',
   +    '#title' => t('Add URL alias'),
   +    '#description' => t("Optionally add an URL alias to the contact form."),
   +    '#default_value' => $contact_form->getThirdPartySetting('contact_storage','url_alias',FALSE),
   +  ];
   

   There is no third party setting for this. You need to get the default value from the alias storage instead.

2. +++ b/contact_storage.module
   @@ -46,21 +52,38 @@ function contact_storage_form_contact_form_form_alter(&$form, FormStateInterface
    }
   -/**
   - * Entity builder for the contact form edit form with third party options.
   - *
   - * @see contact_storage_test_form_contact_form_edit_form_alter()
   - */
   + /**
   +  * Entity builder for the contact form edit form with third party options.
   +  *
   +  * @see contact_storage_test_form_contact_form_edit_form_alter()
   +  */
   

   unrelated change.

3. +++ b/contact_storage.module
   @@ -46,21 +52,38 @@ function contact_storage_form_contact_form_form_alter(&$form, FormStateInterface
   +function contact_storage_form_contact_form_validate(&$form, FormStateInterface $form_state) {
   +  if(strcmp($form_state->getValue('contact_storage_url_alias'),"") && (strpos($form_state->getValue('contact_storage_url_alias'), '/') !== 0)) {
   +    $form_state->setError($form['contact_storage_url_alias'], 'The alias path has to start with a slash.');
   +  } else if ($form_state->hasValue('contact_storage_url_alias') && (strpos($form_state->getValue('contact_storage_url_alias'), '/') === 0)) {
   +    $filename = $form_state->getValue('id');
   +    \Drupal::service('path.alias_storage')->save('/contact/' .$filename, $form_state->getValue('contact_storage_url_alias'));
   +  }
   +}
   

   do not save something in a validate function. Maybe some other validate function fails then and then the form is not submitted.

   Instead, do the save in the form builder above for example. (but keep the validation here)

   $filename is also a weird variable name. In the form builder, you have $contact_form, then you can just do $contact_form->toUrl()->getInternalPath() and just have to prepend /.

I thinks is not need it. Do you have example?
When I'm using 'hasValue()' for empty it returns me true.

suppose you need to check isEmpty()

+++ b/contact_storage.module
@@ -72,11 +72,14 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
+      $filename = $form_state->getValue('id');

s/$filename/$id or $entity_id

A few code style nitpicks. See https://www.drupal.org/coding-standards/ for reference. It helps running code sniffer (https://www.drupal.org/node/1419988)

+++ b/config/schema/contact_storage.schema.yml
@@ -11,6 +11,9 @@ contact.form.*.third_party.contact_storage:
+      label: 'URl alias'

URl -> URL

+++ b/contact_storage.module
@@ -39,6 +39,12 @@ function contact_storage_form_contact_form_form_alter(&$form, FormStateInterface
+    '#default_value' => $contact_form->getThirdPartySetting('contact_storage','url_alias',FALSE),

nitpick: space after ,

+++ b/contact_storage.module
@@ -46,21 +52,41 @@ function contact_storage_form_contact_form_form_alter(&$form, FormStateInterface
+        \Drupal::service('path.alias_storage')->save('/contact/' .$filename, $form_state->getValue('contact_storage_url_alias'));

space around .

I removed the function "contact_storage_form_contact_form_validate", because in "contact_storage_contact_form_form_builder" I need that check and is point less to have again the same condition.

Created followup #2778937: Fix contact storage test.

1. +++ b/config/schema/contact_storage.schema.yml
   @@ -11,6 +11,9 @@ contact.form.*.third_party.contact_storage:
          label: 'Show preview button'
   +    url_alias:
   +      type: string
   +      label: 'URL alias'
   

   no longer needed.

2. +++ b/contact_storage.module
   @@ -56,8 +64,14 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
   +  $contact_form->setThirdPartySetting('contact_storage', 'url_alias', $form_state->getValue('contact_storage_url_alias'));
   

   and this as well.

3. +++ b/contact_storage.module
   @@ -56,8 +64,14 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
   +  if ($form_state->hasValue('contact_storage_url_alias')) {
   +    $alias = $form_state->getValue('contact_storage_url_alias');
   +    if (!empty($alias) && (strpos($form_state->getValue('contact_storage_url_alias'), '/') === 0)) {
   +      \Drupal::service('path.alias_storage')->save('/contact/' . $form_state->getValue('id'), $alias);
   +    }
   +  }
   

   Two things here:

   a) you still need the validate function. this will just silently do nothing if it is not valid, that's bad for user experience. Instead, you want to display a validation error if a value is set and is not correct. You should also test for that.

   b) I was wrong about the builder function. this called too early, we need a submit callback (additionally to the save callback) that stores it.

4. +++ b/src/Tests/ContactStorageTest.php
   @@ -242,6 +242,10 @@ class ContactStorageTest extends ContactStorageTestBase {
        $this->assertLinkByHref('/admin/structure/contact/messages?form=test_id_2');
   +
   +    $this->addContactForm('form2', 'contactForm', $mail, '', FALSE, ['contact_storage_url_alias' => '/form51']);
   +
   +    $this->drupalGet('form12');
   

   as I said above, try saving with an invalid value. And also add an assert on the contact form title or so additionally to the drupalGet() and an assertResponse(). the drupalGet() will not fail if this is a 404.

Added callback functions for validation and submit. Test coverage.

I created an issue #2780763: Add missing parameter in ContactFormCloneForm::__construct() to fix this test failed.

Good work overall. Here are some points to improve :

1. +++ b/contact_storage.module
   @@ -56,6 +65,33 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
   +  //if ($form_state->hasValue('contact_storage_url_alias')) {
   +    //$alias = $form_state->getValue('contact_storage_url_alias');
   +    //if (!empty($alias) && (strpos($form_state->getValue('contact_storage_url_alias'), '/') === 0)) {
   +     // \Drupal::service('path.alias_storage')->save('/contact/' . $form_state->getValue('id'), $alias);
   +    //}
   + // }
   

   Can we remove this old commented code?

2. +++ b/contact_storage.module
   @@ -56,6 +65,33 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
   + * @param $form
   + * @param \Drupal\Core\Form\FormStateInterface $form_state
   

   That's not exactly the correct way to document a function's parameters :

   @param array $form
     An associative array containing the structure of the form.
   @param \Drupal\Core\Form\FormStateInterface $form_state
     The current state of the form.

3. +++ b/contact_storage.module
   @@ -56,6 +65,33 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
   +function contact_storage_contact_form_form_validate(&$form,FormStateInterface &$form_state) {
   

   Please add a space after commas.

4. +++ b/contact_storage.module
   @@ -56,6 +65,33 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
   +  if(!empty($form_state->getValue('contact_storage_url_alias'))) {
   +    if(strpos($form_state->getValue('contact_storage_url_alias'),'/') !== 0) {
   

   Please add a space after if's.

5. +++ b/contact_storage.module
   @@ -56,6 +65,33 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
   +function contact_storage_contact_form_form_submit(&$form, &$form_state) {
   

   Functions have to be documented.

6. +++ b/contact_storage.module
   @@ -56,6 +65,33 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
   +  if(!empty($form_state->getValue('contact_storage_url_alias'))) {
   

   Missing space after if.

7. +++ b/src/Tests/ContactStorageTest.php
   @@ -34,13 +34,9 @@ class ContactStorageTest extends ContactStorageTestBase {
   +  public function login() {
   
   @@ -51,6 +47,17 @@ class ContactStorageTest extends ContactStorageTestBase {
   +    $admin_user = $this->login();
   
   @@ -244,4 +251,49 @@ class ContactStorageTest extends ContactStorageTestBase {
   +    $admin_user = $this->login();
   

   Code that test functions need to execute before doing actual work normally lives in the setUp() function. Please move the user creation and login code there. This function doesn't need to be explicitly called.

8. +++ b/src/Tests/ContactStorageTest.php
   @@ -244,4 +251,49 @@ class ContactStorageTest extends ContactStorageTestBase {
   +   * Test case that we enter correct URL alias
   ...
   +   * Test for URL alias
   

   Comments should be formatted as proper sentences, so it should end with a dot.

We don't really want to write unit tests here. A single function covering all test cases of the URL alias functionality would be more appropriate. Also, it would be more straightforward to assert for the contact form's label than the page title. We are doing this only to make sure we land on the correct page. We also want to assert that :

• If trying to create a contact form with an alias in an incorrect format, an error message is displayed and the alias isn't generated.
• If we have already created an alias and edit it from the contact form's edit page, the previous alias is deleted.

Fixed coding standard, put everything in one function and added new test cases.

1. +++ b/contact_storage.module
   @@ -39,6 +39,13 @@ function contact_storage_form_contact_form_form_alter(&$form, FormStateInterface
   +  $form_data = \Drupal::service('path.alias_storage')->load(['source' => '/contact/' . $form['id']['#default_value']]);
   +  $form['contact_storage_url_alias'] = [
   

   $this is not $form_data, use $alias or so.

   Also, you now also check this if the entity is new, that doesn't make sense, there can't be an alias yet, there is no id yet. And you can access this much easier through $contact_form.

   if (!$contact_form->isNew()) {
   if ($alias = ...->load('/' . $contact_form->toUrl()->getInternal()) {
   // set #default_value here.
   }
   }

2. +++ b/contact_storage.module
   @@ -59,6 +68,39 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
   + * @param $form
   + *  An associative array containing the structure of the form
   + *
   + * @param \Drupal\Core\Form\FormStateInterface $form_state
   + *  The current state of the form.
   

   two spaces indendation for description.

3. +++ b/contact_storage.module
   @@ -59,6 +68,39 @@ function contact_storage_contact_form_form_builder($entity_type, ContactFormInte
   +function contact_storage_contact_form_form_submit(&$form, &$form_state) {
   +  \Drupal::service('path.alias_storage')->delete(['source' => '/contact/' . $form['id']['#default_value']]);
   +  if (!empty($form_state->getValue('contact_storage_url_alias'))) {
   +    \Drupal::service('path.alias_storage')->save('/contact/' . $form_state->getValue('id'), $form_state->getValue('contact_storage_url_alias'));
   

   same here, get the entity with $form_state->getFormObject()->getEntity(), then use the same method as above.

   You should also implement this more like \Drupal\path\Plugin\Field\FieldType\PathItem::postSave()

   Try to load first, only delete if you find something and there is no new alias, if there still is an alias, save it, but pass in the pid.

   This is important so that automatic redirects work, for example, from the old to the new alias.

Fixed coding standards and change login in submit callback function.

1. +++ b/contact_storage.module
   @@ -39,13 +39,18 @@ function contact_storage_form_contact_form_form_alter(&$form, FormStateInterface
   -  $form_data = \Drupal::service('path.alias_storage')->load(['source' => '/contact/' . $form['id']['#default_value']]);
   +  //$alias = \Drupal::service('path.alias_storage')->load(['source' => '/contact/' . $form['id']['#default_value']]);
      $form['contact_storage_url_alias'] = [
   

   you can remove this now.

2. +++ b/contact_storage.module
   @@ -94,10 +99,26 @@ function contact_storage_contact_form_form_validate(&$form, FormStateInterface &
   +      \Drupal::service('path.alias_storage')->save('/contact/' . $form_state->getValue('id'), $form_state->getValue('contact_storage_url_alias'), 'en', $old_alias['pid']);
   

   you updated all other calls but not yet this one.

Removed old comment and using $entity->toUrl()->getInternalPath().

+++ b/src/Tests/ContactStorageTest.php
@@ -295,4 +305,34 @@ class ContactStorageTest extends ContactStorageTestBase {
+
+    $this->addContactForm('form2', 'contactForm', $mail, '', FALSE, ['contact_storage_url_alias' => '/form51']);
+    $this->assertText('Contact form contactForm has been added.');

I think the order of arguments needs to change here now.

1. +++ b/src/Tests/ContactStorageTest.php
   @@ -295,4 +305,34 @@ class ContactStorageTest extends ContactStorageTestBase {
   +    $this->addContactForm('form2', 'contactForm', $mail, '', FALSE, ['contact_storage_url_alias' => 'form51']);
   +    $this->assertText('The alias path has to start with a slash.');
   +    $this->drupalGet('form51');
   +    $this->assertResponse(200);
   

   test the invalid path first and make sure that the page then *does not* exist yet. This order is a bit weird.

2. +++ b/src/Tests/ContactStorageTest.php
   @@ -295,4 +305,34 @@ class ContactStorageTest extends ContactStorageTestBase {
   +    // Test if alias is empty on beginning
   +    $this->addContactForm('form2', 'contactForm', $mail, '', FALSE, ['contact_storage_url_alias' => '']);
   +    $this->drupalGet('form51');
   

   I'm not sure what this is testing exactly. you're not checking if it is empty, you're explicitly setting an empty path.

   I don't think this is very useful. What you wnat to check instead is that for example editing a page with an alias correctly shows that default value, and re-saving it does then still work.

   And, beside removing the alias from a form, we also want to test that deleting a form also deletes the alias in the database, so we don't leave left-overs there.

I created new feature branch, because there was some commit and there are to much merge conflicts. I implement new function to delete alias when we deleting a form. Also change some test case.

Looks good to me. I made some various improvements, most notably in the test function. Please have a look at the interdiff @tonic92.

Nice clean-up. test coverage looks just like I imagined it now :)

+++ b/contact_storage.module
@@ -41,6 +41,17 @@ function contact_storage_form_contact_form_form_alter(&$form, FormStateInterface
+  $form['contact_storage_url_alias'] = [
+    '#type' => 'textfield',
+    '#title' => t('Add URL alias'),

I just wondered why not reuse
\Drupal\Core\Render\Element\Url

Good point, lets use #type url, that should validate automatically.

I'm not too sure. 'url' form element apparently expects absolute url's. See Drupal\Core\Render\Element\Url::validateUrl and then Drupal\Component\Utility\UrlHelper::isValid. We want to be able to create aliases for relative url's.

Confirmed, you are right. #type url supports only external URLs.

@Berdir strange because core's LinkWidget using it to point local routes

No, it actually doesn't, it changes the #type to entity_autocomplete if internal urls are allowed: https://api.drupal.org/api/drupal/core%21modules%21link%21src%21Plugin%2...

thanks!

This issue is not quite fixed because the current implementation does not work with non-English systems. The alias should be created in the correct language (whichever that may be).

Currently, on a German-only system the alias gets created as language 'en', which Drupal does not know and therefore the alias does not work. Furthermore, it seems to have messed with the previously existing (identical) German alias.

<?php
    // Delete old alias if user erased it.
    if ($old_alias && !$new_alias) {
      \Drupal::service('path.alias_storage')->delete(['pid' => $old_alias['pid']]);
    }
    // Only save a non-empty alias.
    elseif ($new_alias) {
      \Drupal::service('path.alias_storage')->save('/' . $entity->toUrl('canonical')->getInternalPath(), $formState->getValue('contact_storage_url_alias'), 'en', $old_alias['pid']);
    }
?>

Yes, the hardcoded en there is wrong, I missed that. Can you open a new issue, though? Re-opening issues quickly gets confusing.

Make sure you include something like this:

Decide if we should use the config entity language or LanguageInterface::LANGCODE_NOT_SPECIFIED. We can't support creating translated aliases properly from the form anyway.

This issue had not been closed, so I didn't technically re-open it ;) In my eyes, fixing the hard coded language is part of this issue (the current implementation breaks url aliases entered on /admin/config/search/path), but whatever. Feel free to open a new issue.

Opened issue about the hardcoded language:
https://www.drupal.org/node/2796335