Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FieldtypeComments: default action URL incorrect if template trailing slash setting is "No" #866

Closed
Toutouwai opened this issue Apr 29, 2019 · 2 comments

Comments

@Toutouwai
Copy link

@Toutouwai Toutouwai commented Apr 29, 2019

Short description of the issue

The default action URL defined in CommentForm.php is './', but if the template setting "Should page URLs end with a slash?" is "No" then the comment form submits to the parent page.

Although a custom action can be supplied to renderForm() in the $options array it may not be obvious that this is necessary or what is causing the comment form to submit to the wrong URL.

Related forum topic: https://processwire.com/talk/topic/21427-comments-not-saving/

Suggestion for a possible fix

Set the action to the current page URL in the CommentForm constructor.

At line 80:

'action' => '', 

At line 164:

// default action
$this->options['attrs']['action'] = $this->wire('page')->url;

Setup/Environment

  • ProcessWire version: 3.0.130
ryancramerdesign added a commit to processwire/processwire that referenced this issue Jun 27, 2019
@ryancramerdesign
Copy link
Member

@ryancramerdesign ryancramerdesign commented Jun 27, 2019

Thanks @Toutouwai I've pushed an update for this, close to what you suggested. But I want to be careful only to apply it if the current request lacks a trailing slash because there are a few cases where setting it to the current page URL would break things, like if urlSegments are in use or the form is being rendered for a different page. That's why "./" is such a safe thing for the action attribute. But if the current request lacks a trailing slash, then "./" likely isn't going to be helpful, so it makes sense to use the page URL instead in that case.

@Toutouwai
Copy link
Author

@Toutouwai Toutouwai commented Jun 27, 2019

That makes sense, thanks.

@Toutouwai Toutouwai closed this Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants