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

CRM-21777 - Set readonly for the smtp fields initialised in civicrm_s… #15305

Merged
merged 2 commits into from
Sep 30, 2019

Conversation

yashodha
Copy link
Contributor

…ettings.php

Overview

Load input for smtp fields as non-editable which have been initialized in civicrm.settings.php.

Before

SMTP Server OR SMTP Server settings initialized in civi settings file are still shown as editable in civicrm/setting/smtp?reset=1. This has already been fixed for other places setting urls etc.

After

SMTP fields initialized in civicrm.settings.php are loaded as readonly.

@civibot
Copy link

civibot bot commented Sep 13, 2019

(Standard links)

@civibot civibot bot added the master label Sep 13, 2019
@demeritcowboy
Copy link
Contributor

Hi @yashodha this works well there's just one r-run issue below.

  • General standards
    • [r-explain] PASS It took me a bit to figure out how to set the settings in civicrm.settings.php, since it isn't one of the sample/documented ones. And this PR only applies when you have the outbound choice set to SMTP. But otherwise it's obvious what it's about. Example:
      • $civicrm_setting['Mailing Preferences']['mailing_backend']['smtpPort'] = '587';
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] Issue
      • Warning about invalid parameter to foreach when you have a "normal" install that doesn't have any of these overrides. Need to change line 58 since getMandatory returns NULL. I would change it to this:
        $settings = Civi::settings()->getMandatory('mailing_backend') ?? [];
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
    • [r-maint] Undecided
    • [r-test] PASS

@yashodha
Copy link
Contributor Author

@demeritcowboy Thanks for catching that! I have made the desired changes.

@demeritcowboy
Copy link
Contributor

Groovy. I had already tested the suggested change locally earlier so looks good to me.

@yashodha
Copy link
Contributor Author

@eileenmcnaughton Do you want to merge this?

@eileenmcnaughton
Copy link
Contributor

Ok merging per @demeritcowboy review

@eileenmcnaughton eileenmcnaughton merged commit 592fc2b into civicrm:master Sep 30, 2019
@yashodha
Copy link
Contributor Author

yashodha commented Oct 1, 2019

@eileenmcnaughton thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants