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

Removed unused system settings for "Lexicon and Language" section #14848

Merged
merged 3 commits into from
Nov 29, 2019
Merged

Removed unused system settings for "Lexicon and Language" section #14848

merged 3 commits into from
Nov 29, 2019

Conversation

Ruslan-Aleev
Copy link
Collaborator

What does it do?

Removed unused system settings for "Lexicon and Language" section.

These settings are found only in the "System Settings", do not participate in the remaining code:

  • fe_editor_lang

In the future, I will check the settings in other sections.

Related issue(s)/PR(s)

#14539 (comment)

@Mark-H
Copy link
Collaborator

Mark-H commented Nov 29, 2019

Rather than adding a bunch of different upgrade scripts, I think it's fine to add any extra settings that are removed into setup/includes/upgrades/common/3.0.0-cleanup-system-settings.php - the code in each "setting cleanup" migration script is practically the same.

@Ruslan-Aleev
Copy link
Collaborator Author

Ruslan-Aleev commented Nov 29, 2019

I agree with this, I even discussed it with @JoshuaLuckers , we decided to make it different files:

If the removal of some system settings require diffrent migrations its easier to have them in separate files. Having a lot of files is not an issue

I think in the next PR I will combine the settings for deletion into one file.

@Ruslan-Aleev
Copy link
Collaborator Author

@Mark-H there is another question.
Some files that are affected in this PR are infused after - #14849
But why doesn’t the conflict appear in this PR or something I don’t understand?

@Mark-H
Copy link
Collaborator

Mark-H commented Nov 29, 2019

Sorry, where did you discuss that with Joshua? I must've missed that. While we need different files for different versions (i.e. 3.1.0 will require another file) this seems like unnecessary duplication. But if that was already discussed maybe I'm missing some good arguments for separated files.

Git is pretty good at avoiding conflicts most of the time, only when it can't automatically figure out how to deal with multiple changes to the same file you'll see a conflict. ;)

@Ruslan-Aleev
Copy link
Collaborator Author

We discussed this in Slack :) Anyway, I think in the next PR I will combine the settings for deletion into one file.

@Mark-H
Copy link
Collaborator

Mark-H commented Nov 29, 2019

Ahh okay. ;)

@JoshuaLuckers
Copy link
Contributor

Via a direct message on Slack, Ruslan asked me:

isn’t it right to place all the settings in one file (indicating all the keys), and remove the extra files
Example:

  • setup/includes/upgrades/common/3.0.0-remove-upload-flash-system-setting.php
  • setup/includes/upgrades/common/3.0.0-cleanup-system-settings.php
  • setup/includes/upgrades/common/3.0.0-remove-tv-eval-system-setting.php

My reaction:

for now it's ok to have them in different files, it's ok to do it for your PR as well.
If the removal of some system settings require different migrations its easier to have them in separate files. Having a lot of files is not an issue
We can always merge them into one eventually

My reasoning:
More settings might be removed before 3.0 is released, some of them might need additional checks (like this one maybe). Having separate files for now makes it easier for the PR author to add those additional checks and migrations.

Conclusion
Yes, having them in one file is better. Separate files cause unnecessary duplication. But it can't harm to have them in separate files.

@Mark-H Mark-H added this to the v3.0.0-beta milestone Nov 29, 2019
@Mark-H
Copy link
Collaborator

Mark-H commented Nov 29, 2019

It's probably easier (for settings that don't require additional migrations) to put them in the same file, so you don't also have to duplicate the include in each database driver folder... but yeah, no real harm in it this way.

Mark-H added a commit that referenced this pull request Nov 29, 2019
Merge remote-tracking branch 'upstream/pr/14848' into 3.x
@Mark-H Mark-H merged commit 64f2e7a into modxcms:3.x Nov 29, 2019
@Ruslan-Aleev Ruslan-Aleev deleted the 3.x-fix-14539-3 branch June 18, 2020 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants