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

Reworked widget plugin system and fixed minor issues #13

Merged
merged 2 commits into from
Aug 24, 2024

Conversation

cli-ish
Copy link
Contributor

@cli-ish cli-ish commented Jul 31, 2024

Reworked plugin system and fixed minor issues.

Basic

  1. rewrite class loading, utilize the moodle namespaces.
  2. added privacy provider for local_accessibility_configs
  3. changed settings.php get_string to new lang string for better loading time.
  4. implemented extra interface for widget plugins apply_style, makes life easier.

Somewhat security releveant

  1. Self xss over webservice local_accessibility_savewidgetconfig, since protected with sesskey not as important but was fixed anyway.
  2. File resetall.php used PARM_URL should be PARM_LOCALURL even when its protected with a sesskey.
  3. File manageenabledwidgets.php misses sesskey, allows partial management of widgets by tricking an admin to visit a url.

Additional

Since the widget plugins needed some rework too. We created a branch here: https://github.com/sudile/moodle-local_accessibility/tree/master-with-widgets. I was unable to create a PR for it since there are only tags for the widgets repo. The base for the master-with-widgets branch was v1.0.3-with-widgets and should be merged too to a new tag too.

If there is any question feel free to ask.

Kind regards

@cli-ish
Copy link
Contributor Author

cli-ish commented Aug 14, 2024

Hello @ponlawat-w,

if something is missing, I am happy to provide additional explanations.

@ponlawat-w
Copy link
Owner

Hi @cli-ish, thank you for your pull request, and apologise for my late response.
I'll need sometime to fix CI issue, and then I'll look into the deeper details of your commit.

Regarding widget plugins, each of them has their own GitHub repositories, so feel free to create a pull request there:

Thank you so much.

@cli-ish
Copy link
Contributor Author

cli-ish commented Aug 17, 2024

Hi @ponlawat-w,
I create the PR for each subplugin like you suggested.
If there are any issue let me know :)

@ponlawat-w
Copy link
Owner

Hi @cli-ish, thank you again for your contribution.
I have run the changes locally and see no problems, so I think we can have this as a part of next version of the plugin.

The latest support version of this plugin is Moodle 4.3, I am planning to look and combine this change with pull request #14 as well to support Moodle 4.4.

Regarding your changes, however, after running Moodle code checker, it reported the following. If possible, could you please make an update to this PR to solve the following messages? (The report also includes two widget plugins that would need to be updated in the pull requests of their repositories respectively.) Thank you very much.

Total: 6 error(s) and 2 warning(s)

local/accessibility/classes/privacy/provider.php
#166: }
File must end with a newline character (Generic.Files.EndFileNewline.NotFound)

local/accessibility/classes/widgets/apply_style.php
#26: interface·apply_style·{
Missing docblock for interface apply_style (moodle.Commenting.MissingDocblock.Interface)
#27: ····public·function·apply_style():·string;
Missing docblock for function apply_style (moodle.Commenting.MissingDocblock.Function)

local/accessibility/lib.php
#277: ····/**·@var·\moodle_page·$PAGE·*/
Inline doc block type-hinting for '$PAGE' does not match next code line '$widgetinstances...' (moodle.Commenting.InlineComment.TypeHintingMatch)

local/accessibility/settings.php
#30: ····$ADMIN->add('modules',·new·admin_category('accessibilitywidgets',·new·lang_string('accessibilitywidgets',·'local_accessibility')));
Line exceeds 132 characters; contains 135 characters (moodle.Files.LineLength.TooLong)

local/accessibility/styles.php
#25: require_once(__DIR__·.·'/../../config.php');
Expected login check (require_login, require_course_login, require_admin, admin_externalpage_setup) following config inclusion. None found. (moodle.Files.RequireLogin.Missing)

local/accessibility/widgets/backgroundcolour/classes/backgroundcolour.php
#64: ····public·function·apply_style():·string·{
Missing docblock for function apply_style (moodle.Commenting.MissingDocblock.Function)

local/accessibility/widgets/textcolour/classes/textcolour.php
#64: ····public·function·apply_style():·string·{
Missing docblock for function apply_style (moodle.Commenting.MissingDocblock.Function)

PS: The release of a new version of this plugin relies on Moodle Plugin CI with GitHub Actions. Unfortunately, there is currently an issue with Moodle Plugin CI that makes GitHub actions workflow to fail, we might need to wait until the issue is solved before we can release a new version.

@cli-ish
Copy link
Contributor Author

cli-ish commented Aug 18, 2024

Hi @ponlawat-w,

don't know exactly how these slipped my attention i am sorry. I create a commit for this PR and the PR's in ponlawat-w/moodle-accessibility_backgroundcolour#7 and ponlawat-w/moodle-accessibility_textcolour#3.

If there are any more issues feel free to ask :)

Ps: it would be nice if this PR would be useable with Moodle 4.1, i have seen that the other PR you linked had set the requirements to 4.3 :/

@ponlawat-w ponlawat-w changed the base branch from master to dev/2.0/13 August 24, 2024 03:16
@ponlawat-w ponlawat-w merged commit 56cc11b into ponlawat-w:dev/2.0/13 Aug 24, 2024
@ponlawat-w
Copy link
Owner

Thank you @cli-ish, I have merged the changes and included them into version 2.0.

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.

2 participants