-
Notifications
You must be signed in to change notification settings - Fork 986
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 #18961: {SURVEYURL} didn't work with plugin using cli event #3454
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issue in code (seems cool) BUT : seems not related to the issue.
OK, totally related to this issue. More clear now. |
My own test Checked with no update on sendMail Cron
With 55d9eda (parent of 3c70454 ) :
|
* @return string the relative or the configured public URL for the application | ||
*/ | ||
public function getPublicBaseUrl($absolute = false) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it broke with current sendMailCron because there are no use of $baseUrl = CWebApplication::getBaseUrl($absolute); ? unsure ?
Depends on what KO means or how it fails. |
senfMailCron plugin didn't work with the new version, need update. Before 3c70454 it work. sendMailCron use Yii
Your solution disable this usage. I really think it must allow it … (but i'm not the tester) @maziminke have other plugins to send email via CLI. |
Please don't blame on me. Not fair to blame. Also
|
Yes, sorry … i didn't want to blame you. I mean : the current fix didn't allow usage of Yii1 internal function.
Looking at the difference : seems the new updated version (current PR) didn't use |
Host Info could be something to enhance the url methods added on Application component/trait. |
see https://www.yiiframework.com/doc/api/1.1/CUrlManager#baseUrl-detail |
Yes, BUT : some user need only publicurl, admin user access with limesurvey.intra/admin . with baseUrl : you set ALL url using the server, for example css link use absolute url. It's not the same objective here. publicurl used ONLY for survey link or other publi link. Not for css, js etc … |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Place for the Trait
@@ -0,0 +1,44 @@ | |||
<?php | |||
|
|||
trait LSApplicationTrait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to model … core/Traits seem better
Please consider the following when reviewing:
Had to add these to the config:
As this is only required when running ConsoleApplication, maybe there is the need of having specific config for console?