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

Issue #65: fix: DB error - Linkid variable string numeric value checked #69

Closed
wants to merge 3 commits into from

Conversation

marcghaly
Copy link

This PR fixes issues #65,

Regards,

Marc-Alexandre

@marcghaly marcghaly self-assigned this Dec 19, 2023
@marcghaly marcghaly requested a review from dmitriim December 19, 2023 03:57
@marcghaly marcghaly force-pushed the MOODLE_39_STABLE-65-biginterr branch 3 times, most recently from 3882ea1 to 2d7ad10 Compare December 19, 2023 05:26
classes/external.php Outdated Show resolved Hide resolved
@marcghaly marcghaly force-pushed the MOODLE_39_STABLE-65-biginterr branch from 2d7ad10 to c73e696 Compare December 19, 2023 05:31
@marcghaly marcghaly force-pushed the MOODLE_39_STABLE-65-biginterr branch from c73e696 to b1198ef Compare December 19, 2023 05:33
@@ -73,7 +73,7 @@ public static function delete_link_history($linkids) {
public static function count_clicked_links($userid, $noticeid, $linkid = 0) {
global $DB;
$params = [];
if ($linkid > 0) {
if ($linkid > 0 && is_numeric($linkid)) {
Copy link
Member

Choose a reason for hiding this comment

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

how about comparison reported in the issue? Should we also fix its logic?

Copy link
Member

Choose a reason for hiding this comment

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

Actually I think the right spot for fixing it should be acknowledged_notice::other_cols.
The current method should be fixed if it would have a type hint for $linkid. And it will get one at some point.

Copy link
Member

Choose a reason for hiding this comment

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

I have created #70

@dmitriim
Copy link
Member

dmitriim commented Nov 1, 2024

Would be fixed as part of #81 Closing this one off

@dmitriim dmitriim closed this Nov 1, 2024
@dmitriim dmitriim deleted the MOODLE_39_STABLE-65-biginterr branch November 1, 2024 02:05
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