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.

Sign up for GitHub

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

Jump to bottom

Fix the front end preview #1972

Merged
merged 4 commits into from Jul 24, 2020
Merged

Fix the front end preview #1972

merged 4 commits into from Jul 24, 2020

Conversation

leofeyer
Copy link
Member

Q A
Fixed issues Fixes #1968
Docs PR or issue -

@leofeyer leofeyer added the bug label Jul 23, 2020
@leofeyer leofeyer added this to the 4.10 milestone Jul 23, 2020
@leofeyer leofeyer requested a review from a team July 23, 2020 14:24
@leofeyer leofeyer self-assigned this Jul 23, 2020
@leofeyer leofeyer linked an issue Jul 23, 2020 that may be closed by this pull request
Preview link does not open FE preview #1968
Closed
@leofeyer leofeyer mentioned this pull request Jul 23, 2020
Closed
m-vo
m-vo previously approved these changes Jul 23, 2020
View reviewed changes
@leofeyer leofeyer requested a review from richardhj July 23, 2020 15:16
@aschempp
Copy link
Member

RequestContext::getBaseUrl() is what I mean. That should include the script (regardless if preview or app_dev or whatever someone might create) and also a path.

@leofeyer
Copy link
Member Author

I don't think this will work, because by the time the method is called, there is no preview.php fragment in the URL yet. The failing unit test seems to confirm this:

return new RedirectResponse($request->getBaseUrl() ?: '/');

1) Contao\CoreBundle\Tests\Controller\BackendPreviewControllerTest::testRedirectsToRootPageWitPreviewScript
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'preview.php'
+'/'

I think the current fix is correct.

@leofeyer leofeyer requested a review from aschempp July 24, 2020 07:56
aschempp
aschempp requested changes Jul 24, 2020
View reviewed changes
Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

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

the new test is wrong, it falls into the first if-condition in BackendPreviewController. Also, the request object does not have a getBaseUrl() mock.

@leofeyer leofeyer requested a review from aschempp July 24, 2020 08:17
@leofeyer leofeyer merged commit 67e4d8f into contao:4.10 Jul 24, 2020
@leofeyer leofeyer deleted the fix/front-end-preview branch July 24, 2020 08:22
AlexejKossmann pushed a commit to AlexejKossmann/contao that referenced this pull request Apr 6, 2021
@leofeyer
Fix the front end preview (see contao#1972)
fc4e562 
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes contao#1968
| Docs PR or issue | -

Commits
-------

8f44044 Fix the front end preview
0c321e4 Use Request::getBaseUrl()
9e3c402 Reset the tests
3148aee Adjust the test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschempp aschempp aschempp approved these changes

@m-vo m-vo m-vo left review comments

@richardhj richardhj Awaiting requested review from richardhj

Assignees

@leofeyer leofeyer

Labels
bug
Projects
None yet
Milestone
4.10
Development

Successfully merging this pull request may close these issues.

Preview link does not open FE preview
3 participants
@leofeyer @aschempp @m-vo