search

danmarsden / moodle-mod_dialogue

Dialogue Module for Moodle
27 stars 35 forks source link

Incorrect Operator Precedence Causes File Access Error in Dialogue Plugin #132

Closed spicymoodles closed 4 months ago

spicymoodles commented 4 months ago

Environment:

Moodle version: 4.1 PHP versions tested: 7.4, 8.1 Dialogue plugin version: Latest commit 1243d4c

Description:

We are experiencing an issue in the Dialogue plugin where files uploaded in any format cannot be accessed. The problem stems from an incorrect application of the logical OR operator (||). The use of || leads to premature boolean evaluation, which interrupts the expected flow of file handling, resulting in access failures.

Error Message:

Exception: Call to a member function is_external_file() on bool

Debug info: Error code: generalexceptionmessage

Stack Trace: line 460 of /mod/dialogue/lib.php: Error thrown line 5221 of /lib/filelib.php: call to dialogue_pluginfile() line 44 of /pluginfile.php: call to file_pluginfile()

Steps to reproduce:

  1. Create a Dialogue activity within a course and set up test users.
  2. Initiate a new dialogue, choose a test user, complete required fields, and attach a file (e.g., JPG, PDF).
  3. Save the dialogue.
  4. Attempt to open the dialogue and click on the uploaded file to view or download.
  5. The specified exception should be thrown, indicating an issue with file access.

Steps to rectify: (Googled from https://tracker.moodle.org/browse/CONTRIB-9214)

In line 454 in /mod/dialogue/lib.php

Change:

if (!$file = $fs->get_file_by_hash(sha1($fullpath)) || $file->is_directory()) {

To:

if (!$file = $fs->get_file_by_hash(sha1($fullpath)) or $file->is_directory()) {

danmarsden commented 4 months ago

Thanks for the report - Moodle coding guidelines state that we should use || and not "or" but the comparison against $file should be sitting inside some more brackets - surprising it worked with "or" before but not with || - I don't really like that structure so moved the $file = into it's own chunk above which should fix it - please let me know if that's sorted and I'll push out a new release to the plugins db.

spicymoodles commented 4 months ago

Hi Dan, Thank you for this change. We have tested it, it's now working fine. Cheers