Problem/Motivation
There is no way to specify either a custom message, or where the user should be redirected to after the contact form is submitted.
Proposed resolution
Allow user to configure message and redirect route.
Implement protected properties, postpone getter/setter til 9.x #148
Use post update for config #125
Remaining tasks
- Investigate how Views handles selecting a route, in particular with parameters (e.g. node path)
User interface changes
- Form element for user to enter "thank you" message
- Allow user to select a redirect route
API changes
Original report by @frankcarey
This core module is just inches from implementing this and I think it should be a feature moving forward, not a contrib module. Many people would want to direct users to a thank you page, or some additional information instead of sending them back to square 1 (the homepage). I made the changes in a 5.x version for a project at work. Attached are the patches for contact.module and contact.install (uses hook_update_N() so that we could can dump this our sites/all/modules and not hack core)
Functionality: It adds a column to the database and a setting on the site-wide contact admin form. Then the contact form returns that setting instead of and empty sting. This sends users to the appropriate page, if it is set, and defaults to the homepage if it is not.
If you need the 6.x version made, I can do that quick too.
Cheers
Sponsored by http://www.gamefacewebdesign.com
Comment | File | Size | Author |
---|---|---|---|
#220 | 306662-220.patch | 19.95 KB | alexpott |
#220 | 210-220-interdiff.txt | 2.26 KB | alexpott |
Comments
Comment #3
frankcarey CreditAttribution: frankcarey commentedhmm. looks like things changed around in 6.. has this feature been implemented? (don't think so).. anyone know where i could find more info about the contact form changes in 6 and 7?
Comment #4
frankcarey CreditAttribution: frankcarey commentedpatches look ok.. are they supposed to be 1 patch? they should work for those that need it, only have a minute to glance at the patch creation stuff. I created them with a diff gui.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedFeature requests get moved to the version being currently worked. The TestBot set it to CNW for a reason; the patches no longer apply.
Comment #6
Dave ReidThe patch format is also wrong. Please be sure to use diff -up. See http://drupal.org/patch/create for more info.
Comment #7
Dave ReidI 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!
Comment #8
frankcarey CreditAttribution: frankcarey commentedOK, I'll keep working on it. I implemented this a a contrib module in the mean time.
Comment #9
mr.baileysPatch 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.
Comment #11
mr.baileysProbably undefined function call on the _trigger_get_hook_aids() call. Moved the hook_trigger_name implementation.
Comment #12
Dries CreditAttribution: Dries commentedI'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?
Comment #13
mr.baileys<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:
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>
Comment #14
frankcarey CreditAttribution: frankcarey commentedI wrote the contact_redirect module and the initial issue. Will the trigger system allow you to set relevant actions per contact category (sales, inforation, etc)? 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.
Comment #15
tstoecklerApplies 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...
Comment #16
mr.baileys@frankcarey:
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)
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
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...
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.
Comment #17
tstoeckler@ mr. baileys: Sold.
So what keeps this issue from being RTBC?
Comment #18
mr.baileysComment #19
mr.baileysUpdated patch:
back to CNR...
Comment #20
tstoecklerI'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".
Comment #21
mr.baileys#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.
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...
Comment #22
tstoecklerThat'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?
Comment #23
frankcarey CreditAttribution: frankcarey commentedmy 2cents : Being able to redirect users from different contact forms differently is my main concern. I like the way that the actions system will make it easier to customize what happens when the form is submitted, but for me having things set per contact form is vital, I'll look into the redirect action you mentioned.
Comment #24
jmlavarenne CreditAttribution: jmlavarenne commentedThere 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.
Comment #26
andypostDepends on #408434: Actions.inc: fatal error after DBTNG conversion
Comment #27
andypost#408434: Actions.inc: fatal error after DBTNG conversion landed so need reroll
Comment #28
gpk CreditAttribution: gpk commented@16
or use Rules module maybe?
Comment #29
juliendorra CreditAttribution: juliendorra commentedI'm with mr.baileys ! A trigger would be a more universal solution.
Comment #30
andypostJust 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
Comment #31
andypostNow message and redirect passed as parameters to hook so other modules can modify them
Comment #32
Dave ReidUsing module_invoke_all() does not fire triggers, it fires hooks. You would need to use trigger_get_assigned_actions and actions_do().
Comment #33
andypost@Dave Reid yes it fire hook and define trigger
so implementing hook or assign action to trigger contrib can control message and redirect destination
Comment #34
andypostAnother approach - just move message and redirect into form values so contrib can easy alter_form and change this values
Comment #36
andypostAnother try - message and redirect inside
Comment #37
sunmr.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
Comment #38
andypostback to triggers/actions approach
Comment #39
Dave ReidA 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:
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.
Comment #40
andypostNew 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.
Comment #41
sunMissing function body for this PHPDoc.
Missing blank line.
Please read http://drupal.org/node/1354
This hook name sounds a bit weird to me. hook_contact_submit() or similar would be more appropriate.
I don't think there is a need to have a 'contact_' prefix in here.
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.
These comments can be removed.
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.
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.
Please read http://drupal.org/coding-standards
I'm on crack. Are you, too?
Comment #42
andypostMostly fixed.
Added a test with redirect.
This was a try to unify a parameters... so only 'account' and 'category' are different
This is copy-paste bug but now I fixed in previous places
Comment #43
andypostReroll after #601016: Remove contact_site_page() and contact_personal_page() and use the forms directly
Comment #44
webchickWhile 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...
Comment #45
andypostTag removed
Comment #46
Dave ReidLet'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.
Comment #47
hexen CreditAttribution: hexen commentedcontact.install.patch queued for re-testing.
Comment #48
hexen CreditAttribution: hexen commentedcontact.module.patch queued for re-testing.
Comment #49
Lars Toomre CreditAttribution: Lars Toomre commentedI think we should address adding this feature to D8 now that the development branch is open.
Comment #50
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThis 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.
Comment #51
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThis time with the
contact.api.php
file.Comment #52
andypostNice, but trigger would be removed from core #764558: Remove Trigger module from core
Comment #54
andypostTrigger 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
Comment #55
NikLP CreditAttribution: NikLP commentedSeehttps://groups.drupal.org/node/433803#comment-1050843 and my D7 module at https://www.drupal.org/project/ContactPlus
Comment #56
andypostSo 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)
Comment #57
tim-e CreditAttribution: tim-e commentedComment #58
tim-e CreditAttribution: tim-e commentedComment #59
larowlanWe 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.
Comment #60
tim-e CreditAttribution: tim-e commentedok here is my wip on this. Im just working on some test but welcome any feedback on the work so far.
Comment #61
larowlanLooks great @tim-e - just some minor nitpicks
nitpick:extra space here
I think the 'leave blank' bit isn't needed if we default it to . Perhaps change to
Enter <front> to redirect to front page
?Needs one more space?
Missing param comment
needs a blank line above
Do these need to use get() and set() - I think you can use $this->message direct for Config Entities.
Do we need to call Xss::filterAdmin on this?
Comment #63
tim-e CreditAttribution: tim-e commentedQuestion: 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?
Comment #64
andypostgeters/setters is needed only if used
Comment #65
NikLP CreditAttribution: NikLP commentedSeeing 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.. :(
Comment #66
larowlanHi 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
Comment #67
larowlanSpoke 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.
Comment #68
larowlanno need for $this->t() here
Comment #69
larowlanre-roll and fixes 68
Comment #70
larowlanComment #72
larowlanfixes for failing tests
Comment #73
larowlanAdds test
Comment #75
larowlanFix for failing new test
Comment #76
tstoecklerI 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 calledurl
then. (TheUrl
class is a terrible misnomer, but that's not for this issue to fix.)Comment #77
andypostSuppose string should be wrapped into ""
this really confusing
needs "use" and and just Url()
maybe this require to use language somehow?
Comment #78
larowlan@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
Comment #79
tstoecklerSo @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.
Comment #80
larowlanSo in summary
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
Comment #81
tstoecklerYes, that sounds great.
larowlan++
Comment #82
tim-e CreditAttribution: tim-e commentedFixes and tests from larowlan.
Comment #84
tim-e CreditAttribution: tim-e commentedI 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.
Comment #85
tim-e CreditAttribution: tim-e commentedI 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?
Comment #87
jibranEOF missing.
It would be much easier if you'd store \Drupal\Core\Url here instead of route.
This will become CONVERT_URL
It should be return $this->message.
You can get rid of all this by using \Drupal\Core\Url object. Just replace it with $this->url.
this should be $this->message= $message.
Same here $this->url = $url;
You can also create hasRedirctUrl() method to ContactForm for such conditions.
This will become $form_state->setRedirectUrl
Please rename it to $contact_form :)
After the change you can use \Drupal\Core\Url::fromRoute() here.
Comment #88
larowlanIn my opinion redirect to external URL is security risk, also see 78 and 79
Comment #89
larowlaninterdiff from 75-84 for reviewers
Comment #90
larowlanthis will be why some of the tests are failing
Comment #91
tim-e CreditAttribution: tim-e commentedFixes suggestions from jibran and larowlan
Comment #93
jibranOverall it looks good.
You can use setRedirectUrl($url); here
Comment #94
tim-e CreditAttribution: tim-e commentedok, latest fixes.
Comment #96
andypostIt makes sense to use formatted text after #2144413: Config translation does not support text elements with a format
Comment #97
larowlanre-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.
Comment #99
andypostI think if patch will be BC so acceptable for 8.0
Comment #100
deepakaryan1988re-rolling the patch#97
Comment #102
YesCT CreditAttribution: YesCT commented@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.."
Comment #103
andypostAt this point this goes in next release after stabilization in contrib module
Comment #104
naveenvalechalet 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.
Comment #106
naveenvalechafixing tests
Comment #108
naveenvalechaStandardInstallerTest and DefaultConfigTest tests is still failing. I digged into this but did not find any reason for its failing.
Comment #109
BerdirThey 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.
Comment #111
naveenvalechaI 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.
Comment #112
naveenvalechaThe tests were failing b/c the keys order of the entities were not correct, Thanks berder!
Comment #113
BerdirI know it sounds weird but you must use type: label. Only that is a translatable string that can vary per language.
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.
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.
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?)
why not move this logic into getRedirectUrl() ? the comment also needs work.
Comment #115
hussainwebI 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.
Comment #117
hussainwebI don't know how I missed this. Still, I am not sure why there were lesser failures.
Comment #118
hussainwebfor #113
Comment #119
naveenvalecha#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.
Comment #121
naveenvalecha#113.4,
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.
Comment #122
naveenvalechaWell 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.
Comment #123
andypostSuppose upgrade path tests are needed, also few remarks
maybe we need upgrade path test
add save() call
message should go after required recipients
unneeded
Comment #124
naveenvalechaChange record drafted for publishing https://www.drupal.org/node/2656940
#123.2
Done
#123.3
Done
#123.4
Done
Comment #125
alexpottThis 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.
Comment #126
naveenvalechaworking on upgrade path test.
Comment #127
naveenvalechaUpgrade path tests addded
Comment #128
alexpottIs the blank message behaviour tested?
If the default value $this->redirect was '' then this logic would be unnecessary. I think the field should also be required.
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.
Comment #129
alexpottI'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.Can we set this to like a user would?
Comment #130
naveenvalecha#128.1
No, will add in next patch.
#128.2
The default value was front,
+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.
#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}
Comment #131
naveenvalecha#128.1 ?
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
Comment #132
alexpottThis 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.
Comment #133
andypost@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 valuethis should be different, because route
<front>
could point to existing path on live sitethe same here, maybe better to assign in in install hook
like
$default_url = Url(<front>)->toString()
Comment #135
naveenvalecha#133.1
Set the front page path after getting the current one in contact.post_update.php
#133.2
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 ?
The default front page is /user b/c we are not installing the system_test module and not manually setting the front page.
Comment #136
naveenvalecha#133.2
Added an hook_install.
Comment #138
naveenvalechaWhy 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
Comment #139
andypostshould not use entities, only plain config
Comment #140
naveenvalechaIn #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
Comment #141
andypostSuppose 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
Comment #143
andypostRemoved debug code...
Comment #144
naveenvalechaI 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
Otherwise all good to me to be in. RTBC +1
Comment #145
andypostYep, makes sense...
core/modules/system/templates/status-messages.html.twig
defines only one wrapper to theme messagesComment #146
naveenvalechaWell all looks good to me for in.Still RTBC +1 as I have own code in this so can't do RTBC
unassigning myself.
Comment #147
jibranHere we go.
Comment #148
catchThis 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.
Comment #149
larowlanworks for me
Comment #150
naveenvalechaFine, working for #149
Comment #151
catch@alexpott had another idea which was to not add these methods at all, and just use ->get().
Comment #152
naveenvalecha#151 : Okay, let's ignore this patch.
will reroll it with the alex idea
Comment #153
andypost151 is nice BC idea
I think we need postponed issue for 9.x to extend current interface
Comment #154
naveenvalechaWell let's try with all the approaches.
Comment #155
naveenvalechaUsed the configEntityInterface getter and setter instead of CM
Comment #156
tim.plunkettThis 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.
Comment #157
andypostPatch 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
Comment #158
tim.plunkettThese calls aren't in the upgrade path... Not sure what you were referring to.
Missing space before (
Comment #160
andypostFixed 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 ContactFormInterfaceComment #161
naveenvalechaAdded getMessage() and getRedirect()
Comment #162
naveenvalechaoops needs work for adding validation for the redirect path.
Comment #163
webchickSo 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 thanreturn $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:
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)
Comment #164
BerdirThe 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.
Comment #165
BerdirNote 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.
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.
Comment #166
bojanz CreditAttribution: bojanz commentedI 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.
Comment #167
tim.plunkettAlso +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?Comment #168
BerdirI 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.
Comment #169
alexpottI'm a fan of
But to me this also means that the interfaces are pointless. We should type hint against the entity class because that enforces this.
Comment #171
naveenvalechaStraight 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 ?
Comment #172
naveenvalechaAdded the documentation as well as checked using substr check / exists at first char.
Comment #174
jibranWell we can do the same thing as menu link ui.
Comment #175
naveenvalechaFixed the failures.
Comment #176
naveenvalechaFixed the failures.
Comment #177
naveenvalecha#174, Nice sugggestion filed the followup here https://www.drupal.org/node/2697411
Comment #178
naveenvalechacorrect interdiff
Edit :
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
Comment #179
jibranThank you @naveenvalecha great work. I think this is ready.
Comment #180
naveenvalechaReroll the patch due to this #2665992: @file is not required for classes, interfaces and traits
Comment #181
alexpottNot needed. The one in the post update function file is correct.
Comment #182
naveenvalechaTaken care #181 that was remaining culprit.
Comment #183
jibranBack to RTBC.
Comment #185
andypostrtbc +1, failures looks not related
Comment #186
andypostback to RTBC
Comment #187
alexpottI think 'you would like' is redundant. Also no space after fullstop. Also I think the text should be modelled on the other path entry descriptions... eg.
Specify an alternative path by which this data can be accessed. For example, type "/about" when writing an about page. Use a relative path with a slash in front.
. Also I wonder if we should use the same autocomplete as we do on the add menu link page.What happens on an invalid path?
Comment #188
andypostThat's about redirect, not path alias
#187.2 needs work
Comment #189
alexpottRe #187.1 I was talking about the last part of the sentence - dealing with telling people about leading slashes.
Comment #190
naveenvalecha#187.1 Fine, Added the suggested description.
#187.2 The path validator service will take care if the path will not be valid. We only handing here that the path should start with a leading slash.
Comment #192
naveenvalechaComment #193
andypostThen it looks good for me, except now version is 8.2 (
8.1.x-8.2.x now
Comment #194
naveenvalechaUpdated addtogroup
Comment #195
andypostNow looks ready again
Comment #196
alexpottOut-of-scope and just wrong.
No need to call ->set() or ->get() here just use the properties.
This test looks wrong because the before and after tests are the same. You probably need to assert you have more than 0 entities each time too. Plus the values need to be tested... these properties are not public. Plus you might be better just to read the underlying config objects.
Comment #197
larowlanComment #198
naveenvalecha#196.1
Removed the out of scope thing.
#196.2
Taken care
#196.3
Updated tests.Also asserted that the entities are more than 0
Comment #199
alexpottThis is still wrong as the properties are protected. As i said in #196.3 it might be better to load the underlying config and test the properties there.
Comment #201
pguillard CreditAttribution: pguillard commentedJust a rereoll at the moment.
Comment #203
pguillard CreditAttribution: pguillard commentedComment #204
pguillard CreditAttribution: pguillard commentedRerolled again, since 2 modifications where made already done with this commit :
alexpott committed 4b6d125 on 8.2.x
Issue #2653318 by Wim Leers, aneek, marthinal, dawehner: While in...
Comment #207
pguillard CreditAttribution: pguillard commentedRerolled again, astestSiteMaintenance() was defined twice.
Comment #208
naveenvalechaWrong patch in #207 and #204.
#199,
About the usage of configFactory, 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.
See the changes below :
The get() function will return the array because the message key does not exist.So an is_string check will fail here.
The get() function will return the string because the message key exist.So an is_string check will pas here.
Comment #209
andypostIt should return NULL according implementations of Config::get()
Comment #210
alexpottPatch attached fixes:
Given that I've reviewed this patch several times and my additions are to test coverage and coding standards I think I can rtbc this one.
Comment #211
alexpottComment #212
alexpottComment #213
jibranI don't think we need any assertions before running updates.
Comment #214
alexpott@jibran that proves the updates have done something - which is important because maybe the initial DB gets updated or something weird.
Comment #215
naveenvalecha#213,
@alexpott +1
The assertions before update proves that the previously in drupal 8 these keys(contact & message) does not exist.
Looks good RTBC +1
$contact_form_data would be the Configuration object returned by the ConfigFactory. We should use the getter method to fetch the value of a particular key?
Or is there anything specific that has been taken care by treating it as array ?
Comment #216
alexpott@naveenvalecha it is just doing less - there is no harm in just getting the complete array representation and asserting against that.
$contact_form_data
is not an object - it is an array representing the entire config object...$contact_form_data = $config_factory->get($contact_config_name)->get();
- the last->get()
returns the entity array.Comment #217
naveenvalecha#216,
It explains everything that I was looking for.
Thanks!
Comment #218
effulgentsia CreditAttribution: effulgentsia at Acquia commentedJust a drive-by review, but this looks like maybe an incorrect leftover from a copy/paste? I don't think it describes what a redirect is.
Comment #219
alexpott@effulgentsia you are completely correct! Nice spot.
Comment #220
alexpottUpdated strings in the patch to be better.
Comment #221
xjm@alexpott, @effulgentsia, @catch, @Cottser, and I discussed this issue and agreed that it would be a valuable addition for 8.2.0 that we want to consider for a beta target. This means that we will still commit this change following the first beta once it is ready, ideally before we release the second beta.
Comment #222
xjmComment #223
jibran@alexpott If someone will update the initial DB then update hook will not run and
$this->runUpdates();
will automatically fail.I'm just being overly cautious here. Running any api calls before running update hooks is a risky thing to do. The site can be broken. I think you told me that when I was struggling with fails before #2464427-166: Replace CacheablePluginInterface with CacheableDependencyInterface. It is a new feature these configs never existed before in Drupal 8 release cycle checking them is kind like asserting universal truth.
Comment #224
alexpott@jibran we've deemed that direct access to config via the factory will always work. If it does not many hook_Update_n's will break.
Comment #225
andypostNice clean-up of strings!
Comment #226
catchLooks great to me now.
Committed/pushed to 8.2.x, thanks!
Comment #228
alexpottComment #229
andypostYay! fixed twice! //interesting bug with comments
Comment #230
naveenvalechaYay!!!! Finally one of the issue from the contact form roadmap #2582955: Contact module roadmap: 80% usecase of webforms in core has landed.
Great Thanks everyone
Comment #231
xjmThis landed before the beta, so untagging. Glad to see this feature!
Comment #234
larowlanThis is a BC break. Ideally the second argument would default to NULL and we'd have a protected accessor that accessed the container in case of NULL. See #2780763: Add missing parameter in ContactFormCloneForm::__construct()
Comment #235
BerdirFollow-up for empty default message: #2786085: Contact form submission message should exist by default.
Comment #237
frankcarey CreditAttribution: frankcarey as a volunteer and at DEVINCI commentedAmazing! My first official Drupal core commit has come from an 8 year old issue. Thanks to everyone who helped bring this to D8 :)