diff --git a/lib/classes/moodlenet/activity_packager.php b/lib/classes/moodlenet/activity_packager.php index 7e6f7ec862e..96a1c8d6693 100644 --- a/lib/classes/moodlenet/activity_packager.php +++ b/lib/classes/moodlenet/activity_packager.php @@ -21,6 +21,7 @@ use backup_controller; use backup_root_task; use cm_info; use core\context\user; +use stored_file; defined('MOODLE_INTERNAL') || die(); @@ -46,14 +47,14 @@ class activity_packager { */ public function __construct( protected cm_info $cminfo, - protected int $userid + protected int $userid, ) { // Check backup/restore support. if (!plugin_supports('mod', $cminfo->modname , FEATURE_BACKUP_MOODLE2)) { throw new \coding_exception("Cannot backup module $cminfo->modname. This module doesn't support the backup feature."); } - $this->controller = new backup_controller ( + $this->controller = new backup_controller( backup::TYPE_1ACTIVITY, $cminfo->id, backup::FORMAT_MOODLE, @@ -66,10 +67,9 @@ class activity_packager { /** * Prepare the backup file using appropriate setting overrides and return relevant information. * - * @return array Array containing packaged file and stored_file object describing it. - * Array in the format [storedfile => stored_file object, filecontents => raw file]. + * @return stored_file */ - public function get_package(): array { + public function get_package(): stored_file { $alltasksettings = $this->get_all_task_settings(); // Override relevant settings to remove user data when packaging to share to MoodleNet. @@ -106,7 +106,6 @@ class activity_packager { * @param array $alltasksettings All task settings. * @param string $settingname The name of the setting to be overridden (task class name format). * @param int $settingvalue Value to be given to the setting. - * @return void */ protected function override_task_setting(array $alltasksettings, string $settingname, int $settingvalue): void { if (empty($rootsettings = $alltasksettings[backup_root_task::class])) { @@ -123,13 +122,12 @@ class activity_packager { } /** - * Package the activity identified by CMID. + * Package the activity identified by CMID into a new stored_file. * - * @return array Array containing packaged file and stored_file object describing it. - * Array in the format [storedfile => stored_file object, filecontents => raw file]. - * @throws \moodle_exception. + * @return stored_file + * @throws \moodle_exception */ - protected function package(): array { + protected function package(): stored_file { // Execute the backup and fetch the result. $this->controller->execute_plan(); $result = $this->controller->get_results(); @@ -147,7 +145,7 @@ class activity_packager { } // Create the location we want to copy this file to. - $fr = [ + $filerecord = [ 'contextid' => user::instance($this->userid)->id, 'userid' => $this->userid, 'component' => 'user', @@ -159,19 +157,11 @@ class activity_packager { // Create the local file based on the backup. $fs = get_file_storage(); - $packagedfiledata = [ - 'storedfile' => $fs->create_file_from_storedfile($fr, $backupfile), - ]; + $file = $fs->create_file_from_storedfile($filerecord, $backupfile); // Delete the backup now it has been created in the file area. $backupfile->delete(); - // Ensure we can handle files at the upper end of the limit supported by MoodleNet. - raise_memory_limit(activity_sender::MAX_FILESIZE); - - // Get the actual file content. - $packagedfiledata['filecontents'] = $packagedfiledata['storedfile']->get_content(); - - return $packagedfiledata; + return $file; } } diff --git a/lib/classes/moodlenet/activity_sender.php b/lib/classes/moodlenet/activity_sender.php index 2ce5b0fdb2a..0031d4550d6 100644 --- a/lib/classes/moodlenet/activity_sender.php +++ b/lib/classes/moodlenet/activity_sender.php @@ -21,6 +21,7 @@ use core\event\moodlenet_resource_exported; use core\oauth2\client; use moodle_exception; use stdClass; +use stored_file; /** * API for sharing Moodle LMS activities to MoodleNet instances. @@ -65,9 +66,8 @@ class activity_sender { protected int $userid, protected moodlenet_client $moodlenetclient, protected client $oauthclient, - protected int $shareformat = self::SHARE_FORMAT_BACKUP + protected int $shareformat = self::SHARE_FORMAT_BACKUP, ) { - [$this->course, $this->cminfo] = get_course_and_cm_from_cmid($cmid); if (!in_array($shareformat, $this->get_allowed_share_formats())) { @@ -90,7 +90,7 @@ class activity_sender { $issuer = $this->oauthclient->get_issuer(); // Check user can share to the requested MoodleNet instance. - $coursecontext = \context_course::instance($this->course->id); + $coursecontext = \core\context\course::instance($this->course->id); $usercanshare = utilities::can_user_share($coursecontext, $this->userid); if ($usercanshare && utilities::is_valid_instance($issuer) && $this->oauthclient->is_logged_in()) { @@ -108,17 +108,15 @@ class activity_sender { $filedata = $this->prepare_share_contents(); // If we have successfully prepared a file to share of permitted size, share it to MoodleNet. - if (!empty($filedata['storedfile'])) { - + if (!empty($filedata)) { // Avoid sending a file larger than the defined limit. - $filesize = $filedata['storedfile']->get_filesize(); + $filesize = $filedata->get_filesize(); if ($filesize > self::MAX_FILESIZE) { - $filedata['storedfile']->delete(); - throw new moodle_exception('moodlenet:sharefilesizelimitexceeded', 'core', '', - [ - 'filesize' => $filesize, - 'filesizelimit' => self::MAX_FILESIZE, - ]); + $filedata->delete(); + throw new moodle_exception('moodlenet:sharefilesizelimitexceeded', 'core', '', [ + 'filesize' => $filesize, + 'filesizelimit' => self::MAX_FILESIZE, + ]); } // MoodleNet only accept plaintext descriptions. @@ -130,7 +128,11 @@ class activity_sender { ['context' => $coursecontext] ); - $response = $this->moodlenetclient->create_resource_from_file($filedata, $this->cminfo->name, $resourcedescription); + $response = $this->moodlenetclient->create_resource_from_stored_file( + $filedata, + $this->cminfo->name, + $resourcedescription, + ); $responsecode = $response->getStatusCode(); $responsebody = json_decode($response->getBody()); @@ -140,7 +142,7 @@ class activity_sender { // Delete the generated file now it is no longer required. // (It has either been sent, or failed - retries not currently supported). - $filedata['storedfile']->delete(); + $filedata->delete(); } // Log every attempt to share (and whether or not it was successful). @@ -155,20 +157,17 @@ class activity_sender { /** * Prepare the data for sharing, in the format specified. * - * @return array Array of metadata about the file, as well as a stored_file object for the file. + * @return stored_file */ - protected function prepare_share_contents(): array { - + protected function prepare_share_contents(): stored_file { switch ($this->shareformat) { case self::SHARE_FORMAT_BACKUP: - default: // If sharing the activity as a backup, prepare the packaged backup. $packager = new activity_packager($this->cminfo, $this->userid); - $filedata = $packager->get_package(); - break; + return $packager->get_package(); + default: + throw new \coding_exception("Unknown share format: {$this->shareformat}'"); }; - - return $filedata; } /** @@ -181,10 +180,10 @@ class activity_sender { * @return void */ protected function log_event( - \context $coursecontext, + \core\context $coursecontext, int $cmid, string $resourceurl, - int $responsecode + int $responsecode, ): void { $event = moodlenet_resource_exported::create([ 'context' => $coursecontext, @@ -203,7 +202,6 @@ class activity_sender { * @return array Array of supported share format values. */ protected function get_allowed_share_formats(): array { - return [ self::SHARE_FORMAT_BACKUP, ]; diff --git a/lib/classes/moodlenet/moodlenet_client.php b/lib/classes/moodlenet/moodlenet_client.php index 7d897413e23..fabbfc074e5 100644 --- a/lib/classes/moodlenet/moodlenet_client.php +++ b/lib/classes/moodlenet/moodlenet_client.php @@ -18,6 +18,9 @@ namespace core\moodlenet; use core\http_client; use core\oauth2\client; +use stored_file; +use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\StreamInterface; /** * MoodleNet client which handles direct outbound communication with MoodleNet instances. @@ -54,22 +57,31 @@ class moodlenet_client { /** * Create a resource on MoodleNet which includes a file. * - * @param array $filedata The file data in the format [storedfile => stored_file object, filecontents => raw file]. + * @param stored_file $file The file data to send to MoodleNet. * @param string $resourcename The name of the resource being shared. * @param string $resourcedescription A description of the resource being shared. - * @return Psr\Http\Message\ResponseInterface The HTTP client response from MoodleNet. + * @return \Psr\Http\Message\ResponseInterface The HTTP client response from MoodleNet. */ - public function create_resource_from_file(array $filedata, string $resourcename, $resourcedescription) { + public function create_resource_from_stored_file( + stored_file $file, + string $resourcename, + string $resourcedescription, + ): ResponseInterface { // This may take a long time if a lot of data is being shared. \core_php_time_limit::raise(); $moodleneturl = $this->oauthclient->get_issuer()->get('baseurl'); $apiurl = rtrim($moodleneturl, '/') . self::API_CREATE_RESOURCE_URI; - $requestdata = $this->prepare_file_share_request_data($filedata, $resourcename, $resourcedescription); + $requestdata = $this->prepare_file_share_request_data( + $file->get_filename(), + $file->get_mimetype(), + $file->get_psr_stream(), + $resourcename, + $resourcedescription, + ); return $this->httpclient->request('POST', $apiurl, $requestdata); - } /** @@ -81,7 +93,13 @@ class moodlenet_client { * @param string $resourcedescription A description of the resource being shared. * @return array Data in the format required to send a file to MoodleNet using \core\httpclient. */ - protected function prepare_file_share_request_data(array $filedata, string $resourcename, $resourcedescription): array { + protected function prepare_file_share_request_data( + string $filename, + string $mimetype, + StreamInterface $stream, + string $resourcename, + $resourcedescription, + ): array { return [ 'headers' => [ 'Authorization' => 'Bearer ' . $this->oauthclient->get_accesstoken()->token, @@ -99,11 +117,10 @@ class moodlenet_client { ], [ 'name' => 'filecontents', - 'contents' => $filedata['filecontents'], + 'contents' => $stream, 'headers' => [ - 'Content-Disposition' => 'form-data; name=".resource"; filename="' . - $filedata['storedfile']->get_filename() . '"', - 'Content-Type' => $filedata['storedfile']->get_mimetype(), + 'Content-Disposition' => 'form-data; name=".resource"; filename="' . $filename . '"', + 'Content-Type' => $mimetype, 'Content-Transfer-Encoding' => 'binary', ], ], diff --git a/lib/classes/moodlenet/utilities.php b/lib/classes/moodlenet/utilities.php index 28fa6cf81d3..f47d2f8b0aa 100644 --- a/lib/classes/moodlenet/utilities.php +++ b/lib/classes/moodlenet/utilities.php @@ -16,7 +16,6 @@ namespace core\moodlenet; -use context_course; use core\oauth2\issuer; /** @@ -46,11 +45,11 @@ class utilities { /** * Check whether a user has the capabilities required to share activities from a given course to MoodleNet. * - * @param context_course $coursecontext Course context where the activity would be shared from. + * @param \core\context\course $coursecontext Course context where the activity would be shared from. * @param int $userid The user ID being checked. * @return boolean */ - public static function can_user_share(context_course $coursecontext, int $userid): bool { + public static function can_user_share(\core\context\course $coursecontext, int $userid): bool { return (has_capability('moodle/moodlenet:shareactivity', $coursecontext, $userid) && has_capability('moodle/backup:backupactivity', $coursecontext, $userid)); } diff --git a/lib/filestorage/file_system.php b/lib/filestorage/file_system.php index 0f9c726add0..53f37d93abb 100644 --- a/lib/filestorage/file_system.php +++ b/lib/filestorage/file_system.php @@ -14,15 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . -/** - * Core file system class definition. - * - * @package core_files - * @copyright 2017 Andrew Nicols - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - -defined('MOODLE_INTERNAL') || die(); +use Psr\Http\Message\StreamInterface; /** * File system class used for low level access to real files in filedir. @@ -631,6 +623,16 @@ abstract class file_system { } } + /** + * Get a PSR7 Stream for the specified file which implements the PSR Message StreamInterface. + * + * @param stored_file $file + * @return StreamInterface + */ + public function get_psr_stream(stored_file $file): StreamInterface { + return \GuzzleHttp\Psr7\Utils::streamFor($this->get_content_file_handle($file)); + } + /** * Retrieve the mime information for the specified stored file. * diff --git a/lib/filestorage/stored_file.php b/lib/filestorage/stored_file.php index d04b236b9b0..9ce788ad630 100644 --- a/lib/filestorage/stored_file.php +++ b/lib/filestorage/stored_file.php @@ -23,6 +23,8 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use Psr\Http\Message\StreamInterface; + defined('MOODLE_INTERNAL') || die(); require_once($CFG->dirroot . '/lib/filestorage/file_progress.php'); @@ -437,6 +439,18 @@ class stored_file { return $this->filesystem->get_content_file_handle($this, $type); } + /** + * Get a read-only PSR-7 stream for this file. + * + * Note: This stream is read-only. If you want to modify the file, create a new file and delete the old one. + * The File API creates immutable files. + * + * @return StreamInterface + */ + public function get_psr_stream(): StreamInterface { + return $this->filesystem->get_psr_stream($this); + } + /** * Dumps file content to page. */ diff --git a/lib/filestorage/tests/file_system_test.php b/lib/filestorage/tests/file_system_test.php index e7f1cad1310..bf01aa41423 100644 --- a/lib/filestorage/tests/file_system_test.php +++ b/lib/filestorage/tests/file_system_test.php @@ -1077,6 +1077,24 @@ class file_system_test extends \advanced_testcase { $fs->get_content_file_handle($file, -1); } + /** + * Ensure that get_content_file_handle returns a valid file handle. + * + * @covers ::get_psr_stream + */ + public function test_get_psr_stream(): void { + $file = $this->get_stored_file(''); + + $fs = $this->get_testable_mock(['get_remote_path_from_storedfile']); + $fs->method('get_remote_path_from_storedfile') + ->willReturn(__FILE__); + + $stream = $fs->get_psr_stream($file); + $this->assertInstanceOf(\Psr\Http\Message\StreamInterface::class, $stream); + $this->assertEquals(file_get_contents(__FILE__), $stream->getContents()); + $this->assertFalse($stream->isWritable()); + } + /** * Test that mimetype_from_hash returns the correct mimetype with * a file whose filename suggests mimetype. @@ -1248,4 +1266,3 @@ class file_system_test extends \advanced_testcase { ]; } } - diff --git a/lib/filestorage/tests/stored_file_test.php b/lib/filestorage/tests/stored_file_test.php index d21150bfa8f..1f6d02a54c9 100644 --- a/lib/filestorage/tests/stored_file_test.php +++ b/lib/filestorage/tests/stored_file_test.php @@ -91,4 +91,33 @@ class stored_file_test extends advanced_testcase { $this->assertEquals(1200, $size['width']); $this->assertEquals(297, $size['height']); } + + /** + * Ensure that get_content_file_handle returns a valid file handle. + * + * @covers ::get_psr_stream + */ + public function test_get_psr_stream(): void { + global $CFG; + $this->resetAfterTest(); + + $filename = 'testimage.jpg'; + $filepath = $CFG->dirroot . '/lib/filestorage/tests/fixtures/' . $filename; + $filerecord = [ + 'contextid' => context_system::instance()->id, + 'component' => 'core', + 'filearea' => 'unittest', + 'itemid' => 0, + 'filepath' => '/', + 'filename' => $filename, + ]; + $fs = get_file_storage(); + $file = $fs->create_file_from_pathname($filerecord, $filepath); + + $stream = $file->get_psr_stream(); + $this->assertInstanceOf(\Psr\Http\Message\StreamInterface::class, $stream); + $this->assertEquals(file_get_contents($filepath), $stream->getContents()); + $this->assertFalse($stream->isWritable()); + } + } diff --git a/lib/tests/moodlenet/activity_packager_test.php b/lib/tests/moodlenet/activity_packager_test.php index 7c53b1f3455..b2fc2ef2999 100644 --- a/lib/tests/moodlenet/activity_packager_test.php +++ b/lib/tests/moodlenet/activity_packager_test.php @@ -33,9 +33,8 @@ class activity_packager_test extends \advanced_testcase { * * @covers ::override_task_setting * @covers ::get_all_task_settings - * @return void */ - public function test_override_task_setting() { + public function test_override_task_setting(): void { global $USER; $this->resetAfterTest(); $this->setAdminUser(); @@ -99,9 +98,8 @@ class activity_packager_test extends \advanced_testcase { * * @covers ::get_package * @covers ::package - * @return void */ - public function test_get_package() { + public function test_get_package(): void { global $USER; $this->resetAfterTest(); $this->setAdminUser(); @@ -120,25 +118,18 @@ class activity_packager_test extends \advanced_testcase { $packager = new activity_packager($cminfo, $USER->id); $package = $packager->get_package(); - $this->assertEquals(2, count($package)); - - // Confirm there are backup file contents returned. - $this->assertTrue(array_key_exists('filecontents', $package)); - $this->assertNotEmpty($package['filecontents']); - // Confirm the expected stored_file object is returned. - $this->assertTrue(array_key_exists('storedfile', $package)); - $this->assertInstanceOf(\stored_file::class, $package['storedfile']); + $this->assertInstanceOf(\stored_file::class, $package); // Check some known values in the returned stored_file object to confirm they match the file we have packaged. - $this->assertNotEmpty($package['storedfile']->get_contenthash()); - $this->assertEquals(user::instance($USER->id)->id, $package['storedfile']->get_contextid()); - $this->assertEquals('user', $package['storedfile']->get_component()); - $this->assertEquals('draft', $package['storedfile']->get_filearea()); - $this->assertEquals('assign_backup.mbz', $package['storedfile']->get_filename()); - $this->assertGreaterThan(0, $package['storedfile']->get_filesize()); - $timecreated = $package['storedfile']->get_timecreated(); + $this->assertNotEmpty($package->get_contenthash()); + $this->assertEquals(user::instance($USER->id)->id, $package->get_contextid()); + $this->assertEquals('user', $package->get_component()); + $this->assertEquals('draft', $package->get_filearea()); + $this->assertEquals('assign_backup.mbz', $package->get_filename()); + $this->assertGreaterThan(0, $package->get_filesize()); + $timecreated = $package->get_timecreated(); $this->assertGreaterThanOrEqual($currenttime, $timecreated); - $this->assertEquals($timecreated, $package['storedfile']->get_timemodified()); + $this->assertEquals($timecreated, $package->get_timemodified()); } } diff --git a/lib/tests/moodlenet/activity_sender_test.php b/lib/tests/moodlenet/activity_sender_test.php index 8646f4aa373..0e2603169e2 100644 --- a/lib/tests/moodlenet/activity_sender_test.php +++ b/lib/tests/moodlenet/activity_sender_test.php @@ -29,10 +29,6 @@ use ReflectionMethod; use stdClass; use testing_data_generator; -defined('MOODLE_INTERNAL') || die(); - -require_once('helpers.php'); - /** * Unit tests for {@see activity_sender}. * @@ -56,6 +52,12 @@ class activity_sender_test extends \advanced_testcase { /** @var MockObject $mockoauthclient Mock OAuth client. */ private MockObject $mockoauthclient; + public static function setUpBeforeClass(): void { + parent::setUpBeforeClass(); + + require_once(__DIR__ . '/helpers.php'); + } + /** * Set up function for tests. */ @@ -83,9 +85,8 @@ class activity_sender_test extends \advanced_testcase { * Test prepare_share_contents method. * * @covers ::prepare_share_contents - * @return void */ - public function test_prepare_share_contents() { + public function test_prepare_share_contents(): void { global $USER; $this->setAdminUser(); @@ -126,13 +127,9 @@ class activity_sender_test extends \advanced_testcase { activity_sender::SHARE_FORMAT_BACKUP )); $this->assertNotEmpty($package); - // Confirm there are backup file contents returned. - $this->assertTrue(array_key_exists('filecontents', $package)); - $this->assertNotEmpty($package['filecontents']); // Confirm the expected stored_file object is returned. - $this->assertTrue(array_key_exists('storedfile', $package)); - $this->assertInstanceOf(\stored_file::class, $package['storedfile']); + $this->assertInstanceOf(\stored_file::class, $package); } /** @@ -141,13 +138,12 @@ class activity_sender_test extends \advanced_testcase { * @dataProvider share_activity_provider * @covers ::share_activity * @covers ::log_event - * @covers \core\moodlenet\moodlenet_client::create_resource_from_file + * @covers \core\moodlenet\moodlenet_client::create_resource_from_stored_file * @covers \core\moodlenet\moodlenet_client::prepare_file_share_request_data * @param ResponseInterface $httpresponse * @param array $expected - * @return void */ - public function test_share_activity(ResponseInterface $httpresponse, array $expected) { + public function test_share_activity(ResponseInterface $httpresponse, array $expected): void { global $CFG, $USER; $this->setAdminUser(); diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 0e46975287b..f0798061f7c 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -138,6 +138,8 @@ information provided here is intended especially for developers. - taskslimit - to limit the number of tasks in one run - failed - to limit the run to only the tasks that failed in their previous run Can be mixed, apart from 'id' of course. +* The file_system class now declares a new, optional, `get_psr_stream` function which allows a Stream implementing `\Psr\Http\Message\StreamInterface` to be returned. + A default implementation which uses the existing file handle resource is used by default, but this should be extended by file_system implementations where relevant. === 4.1 ===