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

Use site_url to build form cache #7193

Merged
merged 2 commits into from Apr 12, 2019

Conversation

alanhartless
Copy link
Contributor

Please be sure you are submitting this against the staging branch.

Q A
Bug fix? y
New feature?
Automated tests included?
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs)
BC breaks?
Deprecations?

Description:

When URLs are generated through the router in a web request, the router uses the request to set host, etc. This is normally fine except when you can access the site through multiple URLs (http, https). For forms, the HTML is cached with an action of whatever URL is used to access Mautic. If this mismatches the site_url, the form will fail to submit.

Steps to reproduce the bug:

  1. Edit config file's site_url to some other host (it can be bogus)
  2. Clear the cache
  3. Create a new form
  4. View the form's manual copy HTML and look for the form's action
  5. Notice that it is the host you used to access Mautic and not what is set in site_url

Steps to test this PR:

  1. Repeat and this time the host will match site_url instead

@alanhartless alanhartless added bug Issues or PR's relating to bugs pending-test-confirmation PR's that require one test before they can be merged labels Jan 31, 2019
@alanhartless alanhartless added this to the 2.15.1 milestone Jan 31, 2019
@alanhartless
Copy link
Contributor Author

This has been reviewed by our team in production for a while now so labeling as pending test confirmation.

@kuzmany
Copy link
Member

kuzmany commented Jan 31, 2019

This PR resolve same issue #6752
Should close then?

@escopecz escopecz added this to Needs Second Test in 2.15.1 Jan 31, 2019
@alanhartless alanhartless modified the milestones: 2.15.1, 2.16.0 Mar 11, 2019
@alanhartless alanhartless removed this from Needs Second Test in 2.15.1 Mar 11, 2019
@npracht npracht modified the milestones: 2.16.0, 2.15.2 Mar 28, 2019
Copy link
Member

@npracht npracht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works thanks.

@npracht npracht added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Apr 4, 2019
@npracht npracht added this to Ready to Commit (passed testing) in Mautic 2 Apr 4, 2019
@kuzmany kuzmany merged commit 417079e into mautic:staging Apr 12, 2019
Mautic 2 automation moved this from Ready to Commit (passed testing) to Merged Apr 12, 2019
}

if ('dev' === MAUTIC_ENV) {
$this->baseUrl = '/index_dev.php'.$this->baseUrl;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't appear to work for any dev site that isn't at root.
For example, all mautibox PRs now redirect haphazardly:
https://mautibox.com/index_dev.php/7210/s/dashboard

Can we remove this? I don't see it as needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alanhartless what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be a pain for developing if none of the URLs you click on has index_dev.php in it. I'd rather fix this to work with paths than just remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed... will you be doing that or should I?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged
Projects
No open projects
Mautic 2
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants