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

Fixed issue 18821: Public url is not used for SURVEYURL #3229

Merged

Conversation

twilligls
Copy link
Contributor

Fixed issue #:
New feature #:
Dev:

@twilligls twilligls added this to the Enhancements milestone Jun 20, 2023
@Shnoulle
Copy link
Collaborator

Another solution can be add a LSYii_Application->getBaseUrl ?

@twilligls
Copy link
Contributor Author

Another solution can be add a LSYii_Application->getBaseUrl ?

Yes, maybe good idea

@Shnoulle
Copy link
Collaborator

Put this on dev mantis maybe, we need to review App->getController()->getAbsoluteUrl()

@twilligls
Copy link
Contributor Author

Another solution can be add a LSYii_Application->getBaseUrl ?

Doesn't work, because you would override the baseUrl all the time with configured publicurl, even when you don't need it.

@twilligls
Copy link
Contributor Author

Put this on dev mantis maybe, we need to review App->getController()->getAbsoluteUrl()

I don't understand

@Shnoulle
Copy link
Collaborator

Put this on dev mantis maybe, we need to review App->getController()->getAbsoluteUrl()

I don't understand

Just create a Development mantis issue : https://bugs.limesurvey.org/bug_report_page.php

Another solution can be add a LSYii_Application->getBaseUrl ?

Doesn't work, because you would override the baseUrl all the time with configured publicurl, even when you don't need it.

👍

Maybe a new function then ? I think we need it for API too.

@gabrieljenik
Copy link
Collaborator

Some comments, maybe for the DEV branch:

  • I agree we need a method on the app but only for the createPublicUrl
    • Make new method based on LSYiiController::createAbsoluteUrl()
    • Make LSYiiController::createAbsoluteUrl() to call the new app method.
    • Probably rename the LSYiiController::createAbsoluteUrl() to LSYiiController::createPublicUrl()

@@ -2351,7 +2357,8 @@ public function getSurveyUrl($language = null, $params = [], $preferShortUrl = t

// If short url is not preferred or no alias is found, return a traditional URL
$urlParams = array_merge($params, ['sid' => $this->sid, 'lang' => $language]);
$url = Yii::app()->createAbsoluteUrl('survey/index', $urlParams);
$url = App()->getController()->createAbsoluteUrl('survey/index', $urlParams);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like to call a controller from a model, but if no code reorg is to be done, I don't see any other way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's hard to draw the line between fixing a bug or refactor a lot of old code

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you can just move the code to a helper?
Maybe now is a good time to create App::createPublicUrl() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, you convinced me now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check my latest commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really like it !

@twilligls twilligls added Tested OK This PR has been tested by QA and works as expected Code review done Version checked for code issue without testing labels Jun 23, 2023
@twilligls twilligls merged commit 3c70454 into master Jun 23, 2023
20 checks passed
@twilligls twilligls deleted the CR-956-18821_LS6_Public_url_is_not_used_for_SURVEYURL branch June 23, 2023 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code review done Version checked for code issue without testing Tested OK This PR has been tested by QA and works as expected
Projects
None yet
3 participants