Skip to content

settings: cleanup#713

Merged
smbader merged 4 commits intoncstate-delta:mainfrom
jrchamp:fix/cleanup-settings
Apr 23, 2026
Merged

settings: cleanup#713
smbader merged 4 commits intoncstate-delta:mainfrom
jrchamp:fix/cleanup-settings

Conversation

@jrchamp
Copy link
Copy Markdown
Collaborator

@jrchamp jrchamp commented Apr 21, 2026

Cleanups:

  • $moodlehashideif is always true because this plugin already requires Moodle 3.7+.
    • Remove "false" branches and tidy.
    • Remove environmentlib.php because we no longer use normalize_version().
  • Use notification constants instead of static strings. Requires Moodle 3.1.
  • Use new lang_string() when not immediately using the string. Requires Moodle 2.3.
  • Use admin_setting_configcheckbox_with_lock for locked fields. Requires Moodle 2.0.
  • Avoid creating unnecessary local variables.

Notes:

  • "use" statements are completely ignored until the code that needs them is run. So every version of PHP that supports "use" statements (5.3 and newer) can have a reference like use fakenamespace\fakeclass; and the code will work just fine (because it ignores references until they are needed).
  • $hassiteconfig offers a small amount of defense-in-depth. It's checked before the file is included, but just in case!
  • While it will probably always be safe to use the modsettingzoom page name, because plugininfo_mod (and admin/settings/plugins.php before that) provide us with an appropriately instantiated admin_settingpage, we should not create our own and should use the name attribute that the class provides and uses as the official section name. One less thing to cause a problem if Moodle were to decide to move all plugins into a single folder.
  • I didn't modify the MOODLE_INTERNAL guard in classes/external.php, because that will be resolved when fix: Use new external class names (requires Moodle 4.2) #673 is merged.

jrchamp added 3 commits April 21, 2026 15:48
Cleanups:

- `$moodlehashideif` is always `true` because this plugin already requires Moodle 3.7+.
  - Remove "false" branches and tidy.
  - Remove `environmentlib.php` because we no longer use `normalize_version()`.
- Use notification constants instead of static strings. Requires Moodle 3.1.
- Use `new lang_string()` when not immediately using the string. Requires Moodle 2.3.
- Use admin_setting_configcheckbox_with_lock for locked fields. Requires Moodle 2.0.
- Avoid creating unnecessary local variables.
@jrchamp jrchamp requested a review from a team April 21, 2026 14:40
@jrchamp jrchamp self-assigned this Apr 21, 2026
@jrchamp jrchamp added the bug Fixes problems or reduces technical debt label Apr 21, 2026
Copy link
Copy Markdown
Contributor

@cbounphengsy cbounphengsy left a comment

Choose a reason for hiding this comment

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

Awesome! By removing unnecessary code use and optimizing the settings, we can increase its efficiency and security.

Copy link
Copy Markdown
Contributor

@smbader smbader left a comment

Choose a reason for hiding this comment

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

A massive Spring cleaning effort!
I learned a couple things as well:

  • new lang_string
  • admin_setting_configcheckbox_with_lock

Thanks Champ!

@smbader smbader merged commit 3f63af7 into ncstate-delta:main Apr 23, 2026
6 checks passed
@jrchamp jrchamp deleted the fix/cleanup-settings branch April 23, 2026 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Fixes problems or reduces technical debt

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants