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

Move the existing value of mail_smtp_prefix to mail_smtp_secure #16065

Merged
merged 1 commit into from Feb 22, 2022
Merged

Move the existing value of mail_smtp_prefix to mail_smtp_secure #16065

merged 1 commit into from Feb 22, 2022

Conversation

Jako
Copy link
Collaborator

@Jako Jako commented Feb 18, 2022

What does it do?

Copy the existing value of mail_smtp_prefix to mail_smtp_secure if the new mail_smtp_secure key exist and remove the mail_smtp_prefix setting on success.

Why is it needed?

The system setting mail_smtp_secure can be created during the update before the upgrade script is executed.

Related issue(s)/PR(s)

#16062

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Feb 18, 2022
@Ibochkarev Ibochkarev added the pr/review-needed Pull request requires review and testing. label Feb 18, 2022
@Ruslan-Aleev
Copy link
Collaborator

Ruslan-Aleev commented Feb 18, 2022

Maybe the script should be called wider (3.0.0-update-system-settings-keys.php, for example) so that in the future it can be used for other system settings?

@opengeek
Copy link
Member

Maybe the script should be called wider (3.0.0-update-system-settings-keys.php, for example) so that in the future it can be used for other system settings?

No. These are specific to a change and are thus atomic to that change. They should not be reused.

@Ruslan-Aleev
Copy link
Collaborator

Ruslan-Aleev commented Feb 18, 2022

No. These are specific to a change and are thus atomic to that change. They should not be reused.

I understand, yes, but, for example, there are already too many atomic scripts for updating system settings (see the current state).
Wouldn't it be easier to create a common script (one for updating settings, another for deleting settings, a third for updating keys), and add sys keys in this files in the future? And for future versions, just copy the structure of these scripts (but without the code).
It's just a suggestion, maybe it's overkill.

@opengeek
Copy link
Member

opengeek commented Feb 18, 2022

I understand, yes, but, for example, there are already too many atomic scripts for updating system settings (see the current state). Wouldn't it be easier to create a common script (one for updating settings, another for deleting settings, a third for updating keys), and add sys keys in this files in the future? And for future versions, just copy the structure of these scripts (but without the code). It's just a suggestion, maybe it's overkill.

Actually, the only reason these are even broken out is because we used to have to have separate scripts for MySQL and sqlsrv and these files served as a way to share certain specific parts that were "common" to both. This is no longer the case. So technically, moving forward, we can put all of the upgrade logic directly into the single upgrade file.

@JoshuaLuckers JoshuaLuckers added this to the v3.0.0-pl milestone Feb 19, 2022
@alroniks
Copy link
Collaborator

So technically, moving forward, we can put all of the upgrade logic directly into the single upgrade file.

Better to move to approach with migrations, and every migration should be atomic and revertable (usually rollback is rare case, but ideally it would be nice to have such ability).

@opengeek opengeek merged commit ac29520 into modxcms:3.x Feb 22, 2022
@Jako Jako deleted the smtp_lexicon_part_2 branch February 22, 2022 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR. pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants