Add redirect option to site-wide contact forms

The patch format is also wrong. Please be sure to use diff -up. See http://drupal.org/patch/create for more info.

I feel like the best way to implement this would to have the contact.module provide an trigger of 'form completed' and you could assign it the action 'redirect to url'. No database field needed. Reuse some great stuff we have already!

Patch attached that adds the trigger described in #7 to the contact.module. This patch lets you assign the "redirect to URL" action to be run when the contact form gets submitted. Additionally it also lets you assign the "Display a message to the user" and "send e-mail" actions to this trigger.

EDIT: fixed typo.

Probably undefined function call on the _trigger_get_hook_aids() call. Moved the hook_trigger_name implementation.

I'm not sure about using a trigger for this. It feels like over-engineering / complicating things. Why not add a textarea to write a custom thank you message?

<2-cents mode>

@Dries: the idea of using a trigger was offered in #7 by Dave Reid, and I agree with him: I personally think the trigger/action mechanism is fantastic. I also think it's underused. We can certainly add a custom thank you message to the contact form, but the beauty of using a trigger is that we are giving the (non-coding) site administrator free reign to do what he/she wants:

• Want to send the user to a thank you page? Just assign a redirect action to the trigger (this is also great to let the administrator override the current behavior of sending the user to the front page after submission)
• Want to display a thank you message in the messages block? Just assign the "Display a message to the user" action to the trigger
• Want to send the user someplace else after submitting the contact form? Use the trigger
• Want to send a seperate email someplace after the contact for is used?
• Want to have the server beep twice when somebody submits the contact form? Just download the Beep Action module...
• ...

Additionally, the fact that the contact.module still does not have a "Thank You"-message textarea to me indicates that there is little demand for it, maybe too little to warrant adding an extra textarea to the admin page. Going the trigger-route enables this functionality transparently, without cluttering the interface for a small percentage of users want this.

</2-cents mode>

Applies cleanly and works well, BUT:
If I assign a message to the user as an action to perform when the contact form is submitted, the standard:
"Your message has been sent." message still shows and is not overriden. This message should not be hardcoded. If we use something as flexible as the trigger/action system (big +1 on that, btw) then we should truely make it configurable.
Would it be a problem, to make contact.module dependant on trigger.module and then define a default action upon install.
That would then lead to the problem of "What to do?" if the user deletes that default trigger. Hmm...

@frankcarey:

Will the trigger system allow you to set relevant actions per contact category (sales, inforation, etc)?

Not by default: the trigger passes information about the submission (including the contact form category) to any actions that it triggers, but there is no way to fire actions just for some categories (although one could write a custom action that does just that).

(Actually, I just noticed that I need to re-work the trigger to make sure this information is passed to the actions)

Another issue I'm hearing in my issue queue is changing the redirect based on localization language. (If in spanish, redirect here, if french there). Thanks, trying to keep my module in line with what ever the future of this patch is.

Interesting point. I think that setting a path in the Redirect to URL trigger will by default take you to that page in the user's language (if available). Would that be acceptable?

@tsoeckler

"Your message has been sent." message still shows and is not overriden. This message should not be hardcoded.

This might be a separate issue: I think it would be bad for usability if we remove the message altogether and have users use the trigger mechanism to display it. I agree that this message shouldn't be hardcoded. It makes sense to allow users to enter the message through the interface (or specify <none> if they want no message at all). This falls outside the scope of this issue though...

Would it be a problem, to make contact.module dependant on trigger.module and then define a default action upon install.

I think that might be a problem. Contact.module is used on a large number of sites, while trigger (I think) is used a lot less. I like the flexibility of this trigger provides, but I feel it should still be possible to run the contact.module even when triggers are disabled.

@ mr. baileys: Sold.

So what keeps this issue from being RTBC?

So what keeps this issue from being RTBC?

• I forgot to add code to the trigger that passes information about the submission to actions (so the actions get fired, but don't really have a lot of information to work with). IMO this is necessary before setting it back to CNR.
• The trigger route is my personal preference, but we need to gather enough support within the community (see Dries' note in #12 about the risk of over-engineering things). Although I'm convinced it's a good solution, it's best to have some reviewers comment on the pro/cons of this approach (we need to make sure that the extra lines of code are worth it.)

Updated patch:

• Added trigger to the personal contact form submission too.
• Actions now get passed "category" information.

back to CNR...

I'm getting some errors with the patch, that are probably related to my local setup, but still someone else should test this patch.
In case someone has similar problems:

1. If I don't assign a redirect action, after submitting I get a WSOD (seems like the system doesn't know where to redirect to. When reversing the page it redirects to the front page as usual)

2. If I assign a redirect AND a message to be shown the redirection takes place but the message isn't shown (only the "Your message has been sent.")

Once again, I've had some problems with my local HTTP and have WSODs and stuff all over the place, so I'm leaving this at "needs review".

#1 is caused by what I think is a bug in actions.inc (possibly resulting from DBTNG conversion #394596: DBTNG: Actions.inc), unrelated to this patch.

PHP Fatal error:  Cannot use object of type DatabaseStatementBase as array in \d7-patch\includes\actions.inc on line 85

I think the $result variable (which is initialised as an array) is accidentally overwritten by a database result. I'll dig deeper and open a separate issue for this.

I'm not an expert when it comes to triggers and actions, but I think #2 is a quirk of the trigger/action mechanism: actions fire in semi-random order. The redirect action uses drupal_goto, and drupal_goto exits execution. If actions have the unfortunate luck of being placed after the redirect action, they'll get ignored. Might be considered a bug in the redirect trigger (although I don't know if it can delay the redirect until all other actions have fired) or a missing feature in actions.inc (need to be able to assign weight to actions so redirect can be made to run after the other actions).

I'm going to leave this CNR because I think both issues, although valid concerns, are outside the scope of this issue...

That's true: It was the first time I used triggers to test this patch, and I really was expecting weights / drag'n'drop on those fieldsets.
...Well, that would make this patch RTBC, right?

Interesting point. I think that setting a path in the Redirect to URL trigger will by default take you to that page in the user's language (if available). Would that be acceptable?

There are many different ways to set up i18l, and I don't think this will work. From my experience it does not.

If you redirect to "thank_you" and you are visiting the site in english, you'll get sent to en/thank-you (LANGUAGE_PREFIX/thank_you).

If there is no existing page in other languages called "thank_you", you'll get 404 when attempting fr/thank_you, de/thank_you etc, unless you specifically create the path alias for each language.

Depends on #408434: Actions.inc: fatal error after DBTNG conversion

#408434: Actions.inc: fatal error after DBTNG conversion landed so need reroll

@16

there is no way to fire actions just for some categories (although one could write a custom action that does just that)

or use Rules module maybe?

I'm with mr.baileys ! A trigger would be a more universal solution.

Just to start a ball

This patch adds hook 'contact_send' after message send and trigger definition

Suppose if call this hook before drupal_set_message() we can change default behavior of message that shown to user after submit, same way we can use change redirect URL.

Need opinions about hook parameters for now this is a form values

Now message and redirect passed as parameters to hook so other modules can modify them

Using module_invoke_all() does not fire triggers, it fires hooks. You would need to use trigger_get_assigned_actions and actions_do().

@Dave Reid yes it fire hook and define trigger
so implementing hook or assign action to trigger contrib can control message and redirect destination

Another approach - just move message and redirect into form values so contrib can easy alter_form and change this values

Another try - message and redirect inside

mr.baileys patch's direction was correct: This needs to leverage trigger/actions.

See also #43483: Add trigger/action for custom after-register, after-login and after-logout events

back to triggers/actions approach

A good start so far and this is a much better future-looking approach.

If we're adding this new hook, it should be a little more descriptive. We are trying to avoid any hook_object type hooks in D7. It should be hook_object_action. This will also need to be documented in a new file contact.api.php.

I'd prefer if modules could easily tell if this was a site-wide or personal contact form submission other than having to check a value like that. We should just pass a parameter to the hook:

function hook_contact_sent($type, $values) {
...
}

where type would be either 'site' or 'personal'.

Possibly we could look into splitting this out further into hook_contact_site_sent() and hook_contact_personal_sent().

This will also need tests written for it.

New patch:
- hook_contact_mail_sent($type, $values) described in contact.api.php
- fixed trigger
- added test for triggering site-wide form

Suppose it's bag idea to make 2 hooks.

PS: Please check comments for grammar.

+++ modules/contact/contact.api.php	11 Oct 2009 00:23:10 -0000
@@ -0,0 +1,46 @@
+/**
+ * The comment passed validation and is about to be saved.
+ *
+ * Modules may make changes to the comment before it is saved to the database.
+ *
+ * @param $comment
+ *   The comment object.

Missing function body for this PHPDoc.

+++ modules/contact/contact.api.php	11 Oct 2009 00:23:10 -0000
@@ -0,0 +1,46 @@
+ */
+/**

Missing blank line.

+++ modules/contact/contact.api.php	11 Oct 2009 00:23:10 -0000
@@ -0,0 +1,46 @@
+ *   Additional keys:
+ *     For $type = 'contact_site':
+ *       - "category" - An array with the contact category's data
+ *     For $type = 'contact_personal':
+ *       - "account" - The user object of recipient
+ *       - "user" - The user object of sender
+ *
+ */

Please read http://drupal.org/node/1354

+++ modules/contact/contact.api.php	11 Oct 2009 00:23:10 -0000
@@ -0,0 +1,46 @@
+function hook_contact_mail_sent($type, $values) {

This hook name sounds a bit weird to me. hook_contact_submit() or similar would be more appropriate.

+++ modules/contact/contact.api.php	11 Oct 2009 00:23:10 -0000
@@ -0,0 +1,46 @@
+  if ($type == 'contact_site') {

I don't think there is a need to have a 'contact_' prefix in here.

+++ modules/contact/contact.pages.inc	11 Oct 2009 00:23:10 -0000
@@ -154,6 +154,12 @@ function contact_site_form_submit($form,
+  $values['user'] = $user;
+  module_invoke_all('contact_mail_sent', 'contact_site', $values);
@@ -245,6 +251,11 @@ function contact_personal_form_submit($f
+  // Fire the contact form trigger.
+  module_invoke_all('contact_mail_sent', 'contact_personal', $values);
+++ modules/trigger/trigger.module	11 Oct 2009 00:23:11 -0000
@@ -604,3 +612,15 @@ function _trigger_get_all_info() {
+function trigger_contact_mail_sent($type, $values) {
+  $aids = trigger_get_assigned_actions($type);
+  $context = array(
+    'group' => 'contact',
+    'hook' => $type,
+    'form_values' => &$values,
+  );
+  actions_do(array_keys($aids), (object) $values, $context);

We need 'user' within $context to make tokens work.

I don't think we need to pass the current user at all.

But what we need to pass into $context for personal contact forms is the $account of the contacted user.

+++ modules/contact/contact.pages.inc	11 Oct 2009 00:23:10 -0000
@@ -154,6 +154,12 @@ function contact_site_form_submit($form,
+  // Show message.
@@ -245,6 +251,11 @@ function contact_personal_form_submit($f
+  // Show message.

These comments can be removed.

+++ modules/trigger/trigger.module	11 Oct 2009 00:23:11 -0000
@@ -158,6 +158,14 @@ function trigger_trigger_info() {
+      'contact_site' => array(
+        'label' => t('After completing the site contact form'),
+      ),
+      'contact_personal' => array(
+        'label' => t("After completing a user's contact form"),
+      ),

If you go with above proposal, then these should be equally renamed accordingly, plus suffix, i.e. contact_submit_site and contact_submit_user.

"personal" is a kinda weird when considering that we're talking about user's contact form.

+++ modules/trigger/trigger.test	11 Oct 2009 00:23:12 -0000
@@ -295,4 +295,32 @@ class TriggerOtherTestCase extends Drupa
+  function testActionsContact() {
+    // Assign an action to the contact site trigger.
+    $test_user = $this->drupalCreateUser(array('administer actions'));
+    $this->drupalLogin($test_user);
+    $action_id = 'trigger_test_generic_action';
+    $hash = md5($action_id);
+    $edit = array('aid' => $hash);
+    $this->drupalPost('admin/structure/trigger/contact', $edit, t('Assign'));

Please assign the most common use-case here: The system's URL redirection action. So the actual primary use-case for this action is properly tested already.

+++ modules/trigger/trigger.test	11 Oct 2009 00:23:12 -0000
@@ -295,4 +295,32 @@ class TriggerOtherTestCase extends Drupa
+    variable_set( $action_id, FALSE );

Please read http://drupal.org/coding-standards

I'm on crack. Are you, too?

Mostly fixed.

Added a test with redirect.

+ $values['user'] = $user;
+ module_invoke_all('contact_mail_sent', 'contact_site', $values);

We need 'user' within $context to make tokens work.
I don't think we need to pass the current user at all.

This was a try to unify a parameters... so only 'account' and 'category' are different

     // Set action variable to FALSE.
-    variable_set( $action_id, FALSE );
+    variable_set($action_id, FALSE);

This is copy-paste bug but now I fixed in previous places

Reroll after #601016: Remove contact_site_page() and contact_personal_page() and use the forms directly

While I agree this would be a nice thing to have, and I am absolutely loving the attention that contact.module is getting right now, it is clearly a new feature, and we shut off the flow of new features on Sept. 1.

So I'm sorry, but I need to bump this to Drupal 8...

Tag removed

Let's mark this postponed for now. Not worth re-rolling until HEAD CVS = 8.x. Also note that we can basicaly provide almost this same exact feature as a contrib module. I'm working on getting it written as a module now and will post to http://drupal.org/project/contact_actions shortly.

contact.install.patch queued for re-testing.

contact.module.patch queued for re-testing.

I think we should address adding this feature to D8 now that the development branch is open.

This would be a nice addition to core, as in my opinion it's a common use-case to want a redirect after the submission of a contact form. Without it the user just stays on the contact page. Who needs to send multiple messages on the same contact form? Spammers and annoying people, that's who ;)

Attaching a re-roll for D8 of the patch in #43.

This time with the contact.api.php file.

Nice, but trigger would be removed from core #764558: Remove Trigger module from core

Trigger module is not a part of core and here goes #1588422: Convert contact categories to configuration system

Suppose this needs deeper refactoring
- #12 Dries mentioned about just a textarea for message
- marked as duplicate of this #62279: Allow user-defined message on personal contact form

So we need a better approach to alter contact form

Seehttps://groups.drupal.org/node/433803#comment-1050843 and my D7 module at https://www.drupal.org/project/ContactPlus

So here's 2 ways to solve the issue
1) provide a "thank you" message that would be stored in contact category
2) provide a Url field to store route/path to redirect

The first one is much easy to implement because second one needs UX research how to properly implement UI to allow user to enter a route (currently there's solution in core's views)

We need #2321745: Add #type => 'path' that accepts path but optionally stores URL object or route name and parameters first, but lets bundle that into this patch for now.

ok here is my wip on this. Im just working on some test but welcome any feedback on the work so far.

Looks great @tim-e - just some minor nitpicks

1. +++ b/core/modules/contact/src/ContactFormEditForm.php
   @@ -55,6 +62,19 @@ public function form(array $form, FormStateInterface $form_state) {
   +     }
   

   nitpick:extra space here

2. +++ b/core/modules/contact/src/ContactFormEditForm.php
   @@ -55,6 +62,19 @@ public function form(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t('The path you would like to redirect to after this form has been submitted. Leave blank to redirect to front page.'),
   

   I think the 'leave blank' bit isn't needed if we default it to . Perhaps change to Enter <front> to redirect to front page?

3. +++ b/core/modules/contact/src/ContactFormInterface.php
   @@ -15,6 +15,14 @@
   +   *  A user message.
   
   @@ -31,6 +39,14 @@ public function getRecipients();
   +   *  Route parameters.
   
   @@ -59,6 +83,15 @@ public function setRecipients($recipients);
   +   *  The desired route.
   

   Needs one more space?

4. +++ b/core/modules/contact/src/ContactFormInterface.php
   @@ -39,6 +55,14 @@ public function getReply();
   +   * @param string $message
   

   Missing param comment

5. +++ b/core/modules/contact/src/ContactFormInterface.php
   @@ -39,6 +55,14 @@ public function getReply();
   +   * @return $this
   
   @@ -59,6 +83,15 @@ public function setRecipients($recipients);
   +   * @return $this
   

   needs a blank line above

6. +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -80,6 +94,13 @@ class ContactForm extends ConfigEntityBundleBase implements ContactFormInterface
   +    return $this->get('message');
   
   @@ -102,6 +123,21 @@ public function getReply() {
   +    return $this->get('route');
   ...
   +    $this->set('message', $message);
   
   @@ -110,6 +146,14 @@ public function setReply($reply) {
   +    $this->set('route', $route);
   

   Do these need to use get() and set() - I think you can use $this->message direct for Config Entities.

7. +++ b/core/modules/contact/src/MessageForm.php
   @@ -245,13 +245,18 @@ public function save(array $form, FormStateInterface $form_state) {
   +      drupal_set_message($this->t($contact_message));
   

   Do we need to call Xss::filterAdmin on this?

Question: I added getter/setters for the new properties because the other properties had them, which seemed new to me as well but I wasn't sure. Does anybody know whether I should be using getter/setters for these properties and why/why not?

geters/setters is needed only if used

Seeing as I've already written ALL this code for D7, is no one even going to look at my module? I'm converting it to D8 and it's just going to be duplicate work.. :(

Hi NikLP - thanks for your questions about this issue on twitter.
Not sure if you saw our roadmap for contact module in core but here it is https://groups.drupal.org/node/433803

We're not sure if this can still make it in for 8.0 but we'll keep trying. Also #2342551: Implement ThirdPartySettingsInterface in contact module for contact form config entity will help us get these incremental improvements in during the various 8.x releases.

If you're interested in helping then all that is needed here is to make the tests pass and add tests for the new functionality.

Lee

Spoke with @catch about this issue - given #2341575: [meta] Provide a beta to beta/rc upgrade path - we don't have to support upgrade path until all the blockers in https://www.drupal.org/project/issues/search/drupal?project_issue_follow... are in - so the door is still open for this to get in.

+++ b/core/modules/contact/src/MessageForm.php
@@ -245,13 +245,18 @@ public function save(array $form, FormStateInterface $form_state) {
+      drupal_set_message($this->t($contact_message));

no need for $this->t() here

re-roll and fixes 68

fixes for failing tests

Adds test

Fix for failing new test

I think it would be preferable to use a Url object directly instead of the old-style array of route name and route parameters. Also, the property should be called url then. (The Url class is a terrible misnomer, but that's not for this issue to fix.)

1. +++ b/core/modules/contact/config/install/contact.form.personal.yml
   @@ -5,3 +5,4 @@ reply: ''
   +message: Your message has been sent.
   

   Suppose string should be wrapped into ""

2. +++ b/core/modules/contact/src/ContactFormEditForm.php
   @@ -55,6 +62,19 @@ public function form(array $form, FormStateInterface $form_state) {
   +    if ($route = $contact_form->getRoute()) {
   ...
   +    $form['route'] = array(
   +      '#type' => 'path',
   
   +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -64,6 +71,13 @@ class ContactForm extends ConfigEntityBundleBase implements ContactFormInterface
   +   * The route to redirect to.
   ...
   +   * @var array
   ...
   +  protected $route = array();
   
   @@ -110,6 +146,14 @@ public function setReply($reply) {
   +  public function setRoute($route) {
   +    $this->set('route', $route);
   
   +++ b/core/modules/contact/src/MessageForm.php
   @@ -189,15 +189,21 @@ public function save(array $form, FormStateInterface $form_state) {
   +      $form_state->setRedirect($route['route_name'], $route['route_parameters']);
   

   this really confusing

3. +++ b/core/modules/contact/src/ContactFormEditForm.php
   @@ -55,6 +62,19 @@ public function form(array $form, FormStateInterface $form_state) {
   +      $url = new \Drupal\Core\Url($route['route_name'], $route['route_parameters']);
   

   needs "use" and and just Url()

4. +++ b/core/modules/contact/src/MessageForm.php
   @@ -189,15 +189,21 @@ public function save(array $form, FormStateInterface $form_state) {
   -    drupal_set_message($this->t('Your message has been sent.'));
   +    if ($contact_message = $contact_form->getMessage()) {
   +      drupal_set_message($contact_message);
   +    }
   

   maybe this require to use language somehow?

@tstoeckler you have asked us to use a url object instead of the array style, but its a config entity so we need to store it in config, there is a route data type in our schema already (from tour amongst others) which is a structure for route_name and params - does that change your thinking? or do you think we should be adding a url schema type too? or should we have setUrl getUrl on the interface but in toArray store as route structure?
thanks

So @larowlan brought me down from my Content Entity ivory tower and reminded me that the things we save into a config entity must have a config schema and all that. So we can't really store a Url object directly as a property.

@larowlan then brought up the idea saving route name and route parameters just like it is now, but making those protected and providing getUrl() and setUrl() methods which do the work internally, thus allowing for a nice interface where developers can directly work with Url objects.

I think that would be much better for DX without actually requiring any hackery in the config entity. So I would find that to be the optimal approach.

So in summary

ContactFormInterface {
- public function getRoute;
+ public function getRedirectUrl;
- public function setRoute(array $route);
+ public function setRedirectUrl(Url $url);
}

internally getRedirectUrl can return Url::fromRoute($this->route['route_name'], $this->route['route_params']) and setRedirectUrl can do similar.

also the #convert_path will need to change to convert to a URL.

Thanks

Yes, that sounds great.

larowlan++

Fixes and tests from larowlan.

I am a little unsure about whether a Url object should require a route_name. I can create a Url object with no route_name and no route_parameters so in getRedirectUrl() I only returned the object if a route_name can be retrieved.

I forgot a return FALSE on the getRedirectUrl method but, can/should I return FALSE in this situation? maybe create a new method such as hasRoute() or something?

1. +++ b/core/modules/contact/config/install/contact.form.personal.yml
   @@ -5,3 +5,4 @@ reply: ''
   \ No newline at end of file
   

   EOF missing.

2. +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -65,6 +75,13 @@ class ContactForm extends ConfigEntityBundleBase implements ContactFormInterface
   +  protected $route = array();
   

   It would be much easier if you'd store \Drupal\Core\Url here instead of route.

3. +++ b/core/modules/contact/src/ContactFormEditForm.php
   @@ -55,6 +63,18 @@ public function form(array $form, FormStateInterface $form_state) {
   +      '#convert_path' => PathElement::CONVERT_ROUTE,
   

   This will become CONVERT_URL

4. +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -81,6 +98,13 @@ class ContactForm extends ConfigEntityBundleBase implements ContactFormInterface
   +    return $this->get('message');
   

   It should be return $this->message.

5. +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -103,6 +127,24 @@ public function getReply() {
   +    $url = Url::fromRoute($this->route['route_name'], $this->route['route_parameters']);
   +    if ($url->getRouteName()) {
   +      return $url;
   +    }
   

   You can get rid of all this by using \Drupal\Core\Url object. Just replace it with $this->url.

6. +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -103,6 +127,24 @@ public function getReply() {
   +    $this->set('message', $message);
   

   this should be $this->message= $message.

7. +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -111,6 +153,17 @@ public function setReply($reply) {
   +    $this->route = array(
   +      'route_name' => $url->getRouteName(),
   +      'route_parameters' => $url->getRouteParameters(),
   +    );
   +    return $this;
   

   Same here $this->url = $url;

8. +++ b/core/modules/contact/src/MessageForm.php
   @@ -189,15 +189,21 @@ public function save(array $form, FormStateInterface $form_state) {
   +    elseif ($route = $contact_form->getRoute()) {
   

   You can also create hasRedirctUrl() method to ContactForm for such conditions.

9. +++ b/core/modules/contact/src/MessageForm.php
   @@ -189,15 +189,21 @@ public function save(array $form, FormStateInterface $form_state) {
   +      $form_state->setRedirect($route['route_name'], $route['route_parameters']);
   

   This will become $form_state->setRedirectUrl

10. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
    @@ -270,6 +272,30 @@ function testSiteWideContact() {
    +    $form = ContactForm::load($contact_form);
    

    Please rename it to $contact_form :)

11. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
    @@ -270,6 +272,30 @@ function testSiteWideContact() {
    +    $form->setRoute([
    +      'route_name' => 'entity.user.canonical',
    +      'route_parameters' => [
    +        'user' => $admin_user->id(),
    +      ],
    +    ]);
    

    After the change you can use \Drupal\Core\Url::fromRoute() here.

In my opinion redirect to external URL is security risk, also see 78 and 79

interdiff from 75-84 for reviewers

+++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -357,19 +357,17 @@ function testAutoReply() {
-  protected function addContactForm($id, $label, $recipients, $reply, $selected, $third_party_settings = [], $message = 'Your message has been sent.') {
+  function addContactForm($id, $label, $recipients, $reply, $selected, $third_party_settings = []) {
...
-    $edit['message'] = $message;

this will be why some of the tests are failing

Fixes suggestions from jibran and larowlan

Overall it looks good.

+++ b/core/modules/contact/src/MessageForm.php
@@ -201,8 +201,8 @@ public function save(array $form, FormStateInterface $form_state) {
+      $form_state->setRedirect($url->getRouteName(), $url->getRouteParameters());

You can use setRedirectUrl($url); here

ok, latest fixes.

It makes sense to use formatted text after #2144413: Config translation does not support text elements with a format

re-roll and fixes
We need to decide if route name and params is still the right option here, now that we have refactored to support user-path: style urls, I suspect it is not.

I think if patch will be BC so acceptable for 8.0

re-rolling the patch#97

@deepakaryan1988 Thanks for working on this.

Sprint Weekend https://groups.drupal.org/node/447258 was in January, so this should not be tagged with that, since that event is past.

Also, note the guidelines on using tags:
"Before adding tags read the issue tag guidelines: https://www.drupal.org/node/1023102 . Do NOT use tags for adding random keywords or duplicating any other fields.."

At this point this goes in next release after stabilization in contrib module

let me fix few this in a first shot.
The tests should fail b/c few things more are needed
1. update path
2. update the default config about the contact form .yml file
3. fix tests.

fixing tests

StandardInstallerTest and DefaultConfigTest tests is still failing. I digged into this but did not find any reason for its failing.

They are failing because you have to update the default config to match what is then written in the end, so add the new key in the default config in the standard profile.

Also note that we added a feature like this to contact_storage.

I have already updated the default config and added the new keys (message and route) in the default config in the standard profile(standard/config/install/contact.form.feedback.yml) and in the contact_test.

The tests were failing b/c the keys order of the entities were not correct, Thanks berder!

1. +++ b/core/modules/contact/config/schema/contact.schema.yml
   @@ -22,6 +22,12 @@ contact.form.*:
          label: 'Weight'
   +    message:
   +      type: string
   +      label: 'Message'
   

   I know it sounds weird but you must use type: label. Only that is a translatable string that can vary per language.

2. +++ b/core/modules/contact/src/ContactFormEditForm.php
   @@ -87,6 +94,17 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['url'] = array(
   +      '#type' => 'path',
   +      '#title' => $this->t('Redirect path'),
   +      '#convert_path' => PathElement::CONVERT_URL,
   +      '#default_value' => $contact_form->isNew() ? '<front>' : $path,
   +      '#description' => $this->t('The path you would like to redirect to after this form has been submitted. Enter <front> to direct to front page.'),
   +    );
   

   Surprised that this just works. I guess thanks to set('url', $value) which happens in EntityForm.

   The double name url and route is IMHO pretty confusing and there's quite a bit of conversion and magic going on. I'd rather be a bit more explicit, see also below.

3. +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -84,6 +101,24 @@ class ContactForm extends ConfigEntityBundleBase implements ContactFormInterface
   +    if (!empty($values['route']['route_name'])) {
   +      $this->url = new Url($values['route']['route_name']);
   +      if (!empty($values['route']['route_params'])) {
   +        $this->url->setRouteParameters($values['route']['route_params']);
   +      }
   +    }
   

   can't we create this when needed? (in getRedirectUrl()). Also note that it can change, e.g. during form submission, so not sure what the active thing then is.

4. +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -124,9 +174,36 @@ public function getWeight() {
   +  public function toArray() {
   +    $properties = parent::toArray();
   +    if ($this->url) {
   +      $properties['route'] = [
   +        'route_name' => $this->url->getRouteName(),
   +        'route_params' => $this->url->getRouteParameters(),
   +      ];
   +    }
   

   We usually don't do this in toArray(). Syncing back things usually happens in preSave, see e.g. ConfigEntityBase::preSave(), the stuff with plugin collections.

   However, there isn't really a need to interact with $this->url and use it as the canonical place of our configuration IMHO.

   I'd rather use $this->route for that, I think that would simplify things. setRedirectUrl() would unset $this->url and just set $this->route.

   getRedirectUrl() would create that if needed.

   The advantage is that you don't need to mess with this method at all and we also don't have to create those URL objects if we don't need them (e.g. when importing/exporting config). Just introduces a risk for breaking. Url objects throw exceptions when e.g. a route doesn't exist.

   (what about options, btw, like query arguments?)

5. +++ b/core/modules/contact/src/MessageForm.php
   @@ -211,15 +211,21 @@ public function save(array $form, FormStateInterface $form_state) {
        // To avoid false error messages caused by flood control, redirect away from
        // the contact form; either to the contacted user account or the front page.
        if ($message->isPersonal() && $user->hasPermission('access user profiles')) {
          $form_state->setRedirectUrl($message->getPersonalRecipient()->urlInfo());
        }
   +    elseif ($url = $contact_form->getRedirectUrl()) {
   +      $form_state->setRedirectUrl($url);
   

   why not move this logic into getRedirectUrl() ? the comment also needs work.

I am only trying to fix the failures. There are many failures seemingly related to this one. Should be NW for comments in #113 after the tests run.

I don't know how I missed this. Still, I am not sure why there were lesser failures.

for #113

#113.2
I have now removed this mess of double names and only "url" for both places.
Taken care of #113.1(++ for this, can it allows to push to 8.0.x as well ?),#113.2,#113.5 in 306662-119-address-113-1-2-5.patch
Taken care #113.1,#113.2,#113.3,#113.4,#113.5 - in 306662-119-address-113-all.patch The tests can probably fail in this.

#113.4,

(what about options, btw, like query arguments?)

+++ b/core/modules/contact/config/schema/contact.schema.yml
@@ -22,6 +22,12 @@ contact.form.*:
+      type: route

can't we use the type path instead of route.It will take care of query parameters as well.Also it will save the cost of converting the path to route and then route to path. Thoughts ?

#113.3,
I tried to remove it but we will have to keep this to set the value of route in construct to provide the default value when there will be no value of it.

Well talked with andypost(contact maintainer) about how to preserve the query parameters in the redirect url and he agreed on keeping the redirect url as path in contact.schema.yml

In this we preserves the query parameters as we are saving the path not route that will address all #113
we are only allowing to store only internal paths,if external paths are needed contrib can extend that.

Suppose upgrade path tests are needed, also few remarks

1. +++ b/core/modules/contact/contact.install
   @@ -0,0 +1,18 @@
   + * Add 'message' and 'redirect' field values to 'contact_form' entities.
   

   maybe we need upgrade path test

2. +++ b/core/modules/contact/contact.install
   @@ -0,0 +1,18 @@
   +  $entities = \Drupal::entityTypeManager()->getStorage('contact_form')->loadMultiple();
   +  foreach($entities as $contact) {
   +    $contact->setMessage('Your message has been sent.');
   +    $contact->setRedirectPath('/');
   

   add save() call

3. +++ b/core/modules/contact/src/ContactFormEditForm.php
   @@ -80,6 +81,12 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form['message'] = array(
   +      '#type' => 'textarea',
   +      '#title' => $this->t('Message'),
   ...
        $form['recipients'] = array(
   

   message should go after required recipients

4. +++ b/core/modules/contact/src/MessageForm.php
   @@ -221,12 +224,13 @@ public function save(array $form, FormStateInterface $form_state) {
   +
   

   unneeded

Change record drafted for publishing https://www.drupal.org/node/2656940

#123.2
Done
#123.3
Done
#123.4
Done

+++ b/core/modules/contact/contact.install
@@ -0,0 +1,19 @@
+/**
+ * Add 'message' and 'redirect' field values to 'contact_form' entities.
+ */
+function contact_update_8001() {
+  /** @var \Drupal\contact\ContactFormInterface[] $entities */
+  $entities = \Drupal::entityTypeManager()->getStorage('contact_form')->loadMultiple();
+  foreach($entities as $contact) {
+    $contact->setMessage('Your message has been sent.');
+    $contact->setRedirectPath('/');
+    $contact->save();
+  }
+}

This needs to be a post update function... using the entity API in a hook_update_N is not allowed.

And we need an upgrade path test.

working on upgrade path test.

Upgrade path tests addded

1. +++ b/core/modules/contact/src/ContactFormEditForm.php
   @@ -87,6 +88,19 @@ public function form(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t('The message to display to the user after submission of this form. Leave blank for no message.'),
   

   Is the blank message behaviour tested?

2. +++ b/core/modules/contact/src/Entity/ContactForm.php
   @@ -109,6 +133,36 @@ public function getReply() {
   +    if ($this->redirect) {
   +      /** @var \Drupal\Core\Url $url */
   +      $url = Url::fromUserInput($this->redirect);
   +    }
   +    else {
   +      /** @var \Drupal\Core\Url $url */
   +      $url = Url::fromRoute('<front>');
   +    }
   +    return $url;
   

   If the default value $this->redirect was '' then this logic would be unnecessary. I think the field should also be required.

3. +++ b/core/modules/contact/src/Tests/Update/ContactUpdateTest.php
   @@ -0,0 +1,55 @@
   +    // Check that contact_form does not have fields redirect and message.
   +    $entities = ContactForm::loadMultiple();
   +    foreach($entities as $contact) {
   +      // Check whether 'message' and 'redirect' property does not exist for this entity.
   +      $this->assertFalse(isset($contact->message), 'Message does not exist');
   +      $this->assertFalse(isset($contact->redirect), 'Redirect does not exist');
   +    }
   ...
   +    // Check that contact_form have fields 'redirect' and 'message'.
   +    $entities = ContactForm::loadMultiple();
   +    foreach($entities as $contact) {
   +      // Check whether 'message' and 'redirect' property exist for this entity.
   +      $this->assertFalse(isset($contact->message), 'Message property exists');
   +      $this->assertFalse(isset($contact->redirect), 'Redirect property exists');
   +    }
   

   Let's not do a foreach here... load each contact form using the config system and check. Don't use the entity system. It is not guaranteed to work before an update.

1. +++ b/core/modules/contact/contact.post_update.php
   @@ -0,0 +1,28 @@
   + * @addtogroup updates-8.0.x-to-8.1.x
   

   I'm not sure about the format of this. Considering this is going to go in 8.1.0 I think this probably should be @addtogroup updates-8.1.0 but this needs checking.

2. +++ b/core/modules/contact/contact.post_update.php
   @@ -0,0 +1,28 @@
   +    $contact->setRedirectPath('/');
   

   Can we set this to like a user would?

#128.1
No, will add in next patch.
#128.2
The default value was front,

I think the field should also be required.

+1 I'm happy to make it mandatory, let see would maintainers opinion here ? cc/ @andypost
#128.3
Okay.will add in next patch.
Edit : However If I'll load the whole config entities using config system, however how'll I check that the message key exists or not in config ? b/c if that key does not exist then get() will return me whole data array.Will typecasting would work here ?Some rough below.

    $config_factory = \Drupal::configFactory();
    foreach ($config_factory->listAll('contact.form.') as $contact_config_name) {
      $contact_form = $config_factory->getEditable($contact_config_name);
      $message = $contact_form->get('message');
    }

#129.1
I used the same addtogroup which was just used for history module. https://www.drupal.org/node/2633334#comment-10776594
but previously we used like this for block.post_update.php @addtogroup updates-8.1.0 So I think we should use the common at all places.
#129.2
It was hardcoded as front page in code.User can update the path later from the http://example.com/admin/structure/contact/manage/{contact_form}

#128.1 ?

Is the blank message behaviour tested?

Added the test for blank message behaviour.The message will not display when there will be blank message. Do we need to print default message ?
I think let's keep it as it is let contrib extend it ?

Added patch.
Needs work b/c there are lots of open questions, #130

Edit : interdiff-306662-16-130.txt is interdiff-306662-126-130.txt

+++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -317,7 +317,27 @@ function testSiteWideContact() {
+    $this->assertNoText('Thanks for your submission');

This should assert that there are no messages on the screen. I've got to be honest I'm not sure about this behaviour now that I think about it because how would the user know. But then again the redirect could be to a page saying thank you for your message and having a whole load of content.

Setting to needs review for testbot to test.

@naveenvalecha that's great! only few nits...
1) I see no /user setting why test using that?
2) "/" is not the default - <front> should be converted for default value

1. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
   @@ -284,6 +284,7 @@ public function testContactConfigEntityTranslation() {
   +      'redirect' => '/user'
   
   +++ b/core/modules/contact/config/install/contact.form.personal.yml
   @@ -6,3 +6,5 @@ label: 'Personal contact form'
   +redirect: '/'
   
   +++ b/core/modules/contact/contact.post_update.php
   @@ -0,0 +1,28 @@
   +    $contact->setRedirectPath('/');
   

   this should be different, because route <front> could point to existing path on live site

2. +++ b/core/modules/contact/tests/modules/contact_test/config/install/contact.form.feedback.yml
   @@ -5,3 +5,5 @@ reply: ''
   +redirect: '/'
   
   +++ b/core/profiles/standard/config/install/contact.form.feedback.yml
   @@ -7,3 +7,5 @@ recipients:
   +redirect: '/'
   

   the same here, maybe better to assign in in install hook
   like $default_url = Url(<front>)->toString()

#133.1
Set the front page path after getting the current one in contact.post_update.php

#133.2

the same here, maybe better to assign in in install hook

Regarding in contact.form.*.yml files : In standard profile, It will install at the time of site installation and the default front page path is "/" . We only need to handle the front page setting in contact_install b/c module can update later ?
Only in contact module or both contact_test and contact ?

1) I see no /user setting why test using that?

The default front page is /user b/c we are not installing the system_test module and not manually setting the front page.

#133.2
Added an hook_install.

+++ b/core/modules/contact/contact.install
@@ -13,7 +13,9 @@
-    $contact->setRedirectPath($page_front);
-    $contact->save();
+    if ($contact->getRedirectPath()) {
+      $contact->setRedirectPath($page_front);
+      $contact->save();
+    }

Why I have to explicitly add the check here for fixing this config ? I investigated but did not find the exact answer.
https://www.drupal.org/pift-ci-job/156314

+++ b/core/modules/contact/contact.install
@@ -0,0 +1,21 @@
+  foreach(ContactForm::loadMultiple() as $contact) {
+    if ($contact->getRedirectPath()) {
+      $contact->setRedirectPath($page_front);
+      $contact->save();

should not use entities, only plain config

should not use entities, only plain config

In #130 I have written why I can't do that b/c I have to do additional check whether message/redirect property exist or not

Suppose patch ready now

1) When redirect is empty we always returns front page so BC is kept and no need in hook_install(), post update just initializes properties.
2) Added "Leave blank for redirect to the site front page." to redirect field description
3) Reordered alphabetically functions in entity and interface.
4) removed 2 useless changes

Removed debug code...

+++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -299,6 +300,45 @@ function testSiteWideContact() {
+    $this->assertNoText('There are no messages available.');

I think we need to check here whether the drupal_set_message is not on the page.
if yes then we need to replace it with

$this->xpath('//div[@class=:class]', array(':class' => 'messages--status'))
$this->assertEqual(count($result), 0, 'Contact Message not found.');

Otherwise all good to me to be in. RTBC +1

Yep, makes sense... core/modules/system/templates/status-messages.html.twig defines only one wrapper to theme messages

Well all looks good to me for in.Still RTBC +1 as I have own code in this so can't do RTBC
unassigning myself.

Here we go.

+++ b/core/modules/contact/src/ContactFormInterface.php
@@ -15,6 +15,14 @@
+  public function getMessage();

This adds several methods to ContactFormInterface, which will break if a module is directly implementing that interface.

If we really expect people to implement the interface directly, then we can't make this change - we'd need to add a new, extending interface then check instanceof when we want to call the new methods.

If we don't, then there's not really any point to having the interface in the first place, but still not sure we can change it - or might need to mark it @internal in a patch release prior to the change in this one.

If we really expect people to implement the interface directly, then we can't make this change - we'd need to add a new, extending interface then check instanceof when we want to call the new methods.

works for me

Fine, working for #149

@alexpott had another idea which was to not add these methods at all, and just use ->get().

#151 : Okay, let's ignore this patch.
will reroll it with the alex idea

151 is nice BC idea
I think we need postponed issue for 9.x to extend current interface

Well let's try with all the approaches.

1. 306662-154-added-extendable-interface-approach-1.patch : Added an extendable interface to the contactform and did not change in existing interface.
2. 306662-154-removed-method-from-interface-and-added-to-class-approach-2.patch : Did not do the changes in interface due to BC, added the new method in class and added a @todo to add these methods in interface later(either 9.x or so)
3. 306662-154-use-config-get-set-for-new-schema-keys-approach-3.patch : As specified by alex in #151, used the config get set for updating new config key values.

Used the configEntityInterface getter and setter instead of CM

+++ b/core/modules/contact/src/Entity/ContactForm.php
@@ -44,6 +45,8 @@
+ *     "message",
+ *     "redirect",

This is a config entity, $message and $redirect should be protected properties. Ideally they would also have dedicated getters/setters as well.

EDIT: I missed the 3 approaches bit. Regardless, there should be corresponding protected properties. Also using set()/get() inside setMessage/getMessage is overkill and wrong IMO.

Patch adds properties, post update functions are used only once #125

PS: getters/setters #148 we can file follow-up for 9.x to add them to interface but not in 8.x, see

+++ b/core/modules/contact/src/ContactFormEditForm.php
@@ -87,6 +88,19 @@ public function form(array $form, FormStateInterface $form_state) {
+      '#default_value' => $contact_form->get('message'),
...
+      '#default_value' => $contact_form->get('redirect'),

+++ b/core/modules/contact/src/MessageForm.php
@@ -211,9 +212,12 @@ public function save(array $form, FormStateInterface $form_state) {
+    if ($submission_message = $contact_form->get('message')) {

@@ -221,7 +225,13 @@ public function save(array $form, FormStateInterface $form_state) {
+      $redirect_path = $contact_form->get('redirect');

These calls aren't in the upgrade path... Not sure what you were referring to.

+++ b/core/modules/contact/contact.post_update.php
@@ -0,0 +1,29 @@
+  foreach(ContactForm::loadMultiple() as $contact) {

Missing space before (

Fixed tests and spaces...
I'm sure there's no need in new interface, $entity->get()/set() is good enough, for the methods there's #2663312: Add the message and redirect key methods to ContactFormInterface

Added getMessage() and getRedirect()

oops needs work for adding validation for the redirect path.

So my read of the most recent comments is that andypost's approach is basically fine, but the protected properties on ContactForm need corresponding getter/setter functions like the rest of the properties in that class. (Not in ContactFormInterface, though, to preserve BC). And these getter/setters should just return $this->foo and the like, rather than return $this->get('foo'), per Tim's "overkill" comment. Makes sense.

In testing out the patch in #160, I entered the redirect path of "admin" ("/admin" was expected, but the text does not indicate this, nor is there any validation on the settings form). When I submitted the form as a user, I got the dread "Website has encountered an unexpected error" white screen error. Error log says:

[Wed Feb 17 01:40:31.323987 2016] [fcgid:warn] [pid 16350:tid 2978000896] [client 127.0.0.1:54473] mod_fcgid: stderr: Uncaught PHP Exception InvalidArgumentException: "The user-entered string 'admin' must begin with a '/', '?', or '#'." at /Users/webchick/Sites/8.x/core/lib/Drupal/Core/Url.php line 234, referer: http://8x.dd/contact/feedback

So let's check for that at config form time so end users don't end up terrified they broke something. :D

Incidentally, the first thing I tried was entering an external URL, but that wasn't allowed. I asked Naveem about this, and he said it was to prevent CSRF attacks, which is a little confusing. This is an admin-entered URL, no? (It's not a huge deal since contrib could always implement this functionality, but if we can get away with artificially limiting users in core it would be nice; I can totally see a use case for redirecting to e.g. a different subdomain)

The whole discussion around methods, interfaces and BC is quite confusing and IMHO misleading. Here are the facts:

For entities, their properties/keys/fields *are* an API. protected or not, that's irrelevant. Our code expects that they exist, they are exported into YAML (for config) and imported again. We expect that to work.

Saying that leaving out the methods from the interface to preserve BC is pointless if we call them in our code and *expect* them to be there. And as said above, it's the same if we use get('message') or getMessage(). So if we call that unconditionally, then we can just as well add it to the interface. At least then someone that replaced the class will get a clear error what he has to do, instead of things just stop to work.

I can see two ways forward:

a) We document and require that replacing entity classes is supported but they must extend the default class and we can add new methods there in minor releases. You can of course not do that, but then you have to live with the fact that your code might break in minor releases. Just like replacing/using something that's @internal.

b) We add a new interface, ContactFormWithRedirect or something, only implement that in ContactForm and not the existing interface, and wherever we access that value, check for that interface and if the object does not implement that, fall back to the current behavior.

b) is the safe option, but I think a) would be preferrable in many ways. We have many, many issues that are trying to add new features/things to (config) entities, and if we go with b), some of them will have 5 additional interfaces and tons of conditional code in 8.4.

1. +++ b/core/modules/contact/contact.post_update.php
   @@ -0,0 +1,29 @@
   +    $contact->set('message', 'Your message has been sent.');
   
   +++ b/core/modules/contact/src/MessageForm.php
   @@ -211,9 +212,12 @@ public function save(array $form, FormStateInterface $form_state) {
        $this->flood->register('contact', $this->config('contact.settings')->get('flood.interval'));
   -    drupal_set_message($this->t('Your message has been sent.'));
   +    if ($submission_message = $contact_form->get('message')) {
   +      drupal_set_message($submission_message);
   +    }
   

   Note that this changes that string from interface translation to config translation.

   Users will likely have that translated on non-en sites, so you at least need to use t() in the update path. But, users might have that translated into multiple languages. I don't know if we need to support an upgrade path for that (loop trough all languages of the site, save language config overrides for each that is different. You also need to save the right one for the default config language).

   Another idea would be to fall back to interface translation if there is no message and don't set anything in the upgrade path. Then we don't support displaying no message, we'd have to do that explicitly, using something like (like block labels in 7.x and older) or a separate checkbox.

2. +++ b/core/modules/contact/src/MessageForm.php
   @@ -221,7 +225,13 @@ public function save(array $form, FormStateInterface $form_state) {
   +      $redirect_path = $contact_form->get('redirect');
   +      if ($redirect_path) {
   +        $form_state->setRedirectUrl(Url::fromUserInput($redirect_path));
   +      }
   +      else {
   +        $form_state->setRedirectUrl(Url::fromRoute('<front>'));
   +      }
        }
   

   as @webchick's example showed, you need to catch exceptions here. You should also validate it in the form already, to prevent user errors early (but still catch exceptions here. you might redirect to a node that is deleted later, for example).

   As I said earlier, we added a similar feature in contact_storage. And there are a few issues, most important is #2629630: When no Redirect Page is specified; redirect to parent entity. Quite a few people there argue that by default, it should stay on the current form. I'm kind of against changing the default behavior in a contrib module, but it might be worth checking for 8.1.0. We could explicitly set it to or / in the upgrade path and new would default to staying on the same page.

a) We document and require that replacing entity classes is supported but they must extend the default class and we can add new methods there in minor releases. You can of course not do that, but then you have to live with the fact that your code might break in minor releases. Just like replacing/using something that's @internal.

I think that doing anything other than this opens us to a world of pain core development wise, basically setting our entities in stone.
So +100.

Also +100 to #164a, or we could end up with a dozen "Extra" interfaces over the course of the D8 cycle. Do we need a separate [policy, no patch] issue to codify that in?

I already add my comment to #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation, which we need to get done anyway. Not sure what's missing exactly for that.

I'm a fan of

We document and require that replacing entity classes is supported but they must extend the default class and we can add new methods there in minor releases. You can of course not do that, but then you have to live with the fact that your code might break in minor releases. Just like replacing/using something that's @internal.

But to me this also means that the interfaces are pointless. We should type hint against the entity class because that enforces this.

Straight reroll agianst 171 from #145, putted some changes in branches. so don't have interdiff

Scenario : if I put a path user and validate this path using validator service $valid = \Drupal::service('path.validator')->isValid('user'); it returns the path is valid either the path has / prefix or not.
The user will get the website encountered error if we specify the path without prefix /
How to handle this case ?
1. should we add a documentation here that provide the path with / before
2. check "/" exist using substr ?

Added the documentation as well as checked using substr check / exists at first char.

Well we can do the same thing as menu link ui.

Fixed the failures.

Fixed the failures.

#174, Nice sugggestion filed the followup here https://www.drupal.org/node/2697411

correct interdiff
Edit :

+++ b/core/modules/contact/contact.post_update.php
@@ -0,0 +1,30 @@
+/**
+ * @addtogroup updates-8.0.x-to-8.1.x
+ * @{
+ */

I have assumed that we will cherry pick the commit to 8.1.x as well, so this needs update when we will commit to 8.2.x

Thank you @naveenvalecha great work. I think this is ready.