MDL-76691 core_h5p: Add core lib changes after upgrading it

Apart from applying the points described in readme_moodle.txt, the following changes have been done too: - The parameter $folderName from the method libraryToString() have been removed and a new method, libraryToFolderName() has been added to the H5PCore API. References to libraryToString() with the $folderName set to true have been replaced to the new method. - missing-main-library has been added and replaces in some cases to missing-required-library. - The framework saveLibraryData method must be called before saveLibrary (h5p.classes.php file has been patched to leave the original order because libraryid is required to save the itemid). - The getLibraryId() method from H5PCore has been rewritten to use MUC, in order to avoid PHPUnit failures.
moodle · Feb 6, 2023 · ca37dd3 · ca37dd3
1 parent d18d906
commit ca37dd3
Show file tree

Hide file tree

Showing 20 changed files with 152 additions and 77 deletions.
diff --git a/contentbank/contenttype/h5p/tests/behat/teacher_upload_content.feature b/contentbank/contenttype/h5p/tests/behat/teacher_upload_content.feature
@@ -156,7 +156,7 @@ Feature: H5P file upload to content bank for non admins
     And I switch to "h5p-player" class iframe
     And I switch to "h5p-iframe" class iframe
     Then I should see "Of which countries"
-    Then I should not see "missing-required-library"
+    Then I should not see "missing-main-library"
     And I switch to the main frame
     Given I log out
     And I log in as "admin"
@@ -178,4 +178,4 @@ Feature: H5P file upload to content bank for non admins
     And I should see "filltheblanks.h5p"
     And I click on "filltheblanks.h5p" "link"
     And I switch to "h5p-player" class iframe
-    And I should see "missing-required-library"
+    And I should see "missing-main-library"
diff --git a/h5p/classes/api.php b/h5p/classes/api.php
@@ -67,6 +67,17 @@ public static function delete_library(factory $factory, \stdClass $library): voi
         $DB->delete_records('h5p_library_dependencies', array('libraryid' => $library->id));
         $DB->delete_records('h5p_libraries', array('id' => $library->id));
 
+        // Remove the library from the cache.
+        $libscache = \cache::make('core', 'h5p_libraries');
+        $libarray = [
+            'machineName' => $library->machinename,
+            'majorVersion' => $library->majorversion,
+            'minorVersion' => $library->minorversion,
+        ];
+        $libstring = H5PCore::libraryToString($libarray);
+        $librarykey = helper::get_cache_librarykey($libstring);
+        $libscache->delete($librarykey);
+
         // Remove the libraries using this library.
         $requiredlibraries = self::get_dependent_libraries($library->id);
         foreach ($requiredlibraries as $requiredlibrary) {

diff --git a/h5p/classes/core.php b/h5p/classes/core.php
@@ -14,14 +14,6 @@
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
-/**
- * H5P core class.
- *
- * @package    core_h5p
- * @copyright  2019 Sara Arjona <sara@moodle.com>
- * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
- */
-
 namespace core_h5p;
 
 defined('MOODLE_INTERNAL') || die();
@@ -35,6 +27,9 @@
 use moodle_url;
 use core_h5p\local\library\autoloader;
 
+// phpcs:disable moodle.NamingConventions.ValidFunctionName.LowercaseMethod
+// phpcs:disable moodle.NamingConventions.ValidVariableName.VariableNameLowerCase
+
 /**
  * H5P core class, containing functions and storage shared by the other H5P classes.
  *
@@ -74,7 +69,7 @@ public function __construct(H5PFrameworkInterface $framework, $path, string $url
     protected function getDependencyPath(array $dependency): string {
         $library = $this->find_library($dependency);
 
-        return "libraries/{$library->id}/{$library->machinename}-{$library->majorversion}.{$library->minorversion}";
+        return "libraries/{$library->id}/" . H5PCore::libraryToFolderName($dependency);
     }
 
     /**
@@ -89,7 +84,7 @@ public function get_dependency_roots(int $id): array {
         $context = \context_system::instance();
         foreach ($dependencies as $dependency) {
             $library = $this->find_library($dependency);
-            $roots[self::libraryToString($dependency, true)] = (moodle_url::make_pluginfile_url(
+            $roots[self::libraryToFolderName($dependency)] = (moodle_url::make_pluginfile_url(
                 $context->id,
                 'core_h5p',
                 'libraries',
@@ -412,10 +407,45 @@ public function is_required_core_api($coreapi): bool {
      * @return string The string name on the form {machineName} {majorVersion}.{minorVersion}.
      */
     public static function record_to_string(stdClass $record, bool $foldername = false): string {
-        return static::libraryToString([
-            'machineName' => $record->machinename,
-            'majorVersion' => $record->majorversion,
-            'minorVersion' => $record->minorversion,
-        ], $foldername);
+        if ($foldername) {
+            return static::libraryToFolderName([
+                'machineName' => $record->machinename,
+                'majorVersion' => $record->majorversion,
+                'minorVersion' => $record->minorversion,
+            ]);
+        } else {
+            return static::libraryToString([
+                'machineName' => $record->machinename,
+                'majorVersion' => $record->majorversion,
+                'minorVersion' => $record->minorversion,
+            ]);
+        }
+
     }
+
+    /**
+     * Small helper for getting the library's ID.
+     * This method is rewritten to use MUC (instead of an static variable which causes some problems with PHPUnit).
+     *
+     * @param array $library
+     * @param string $libString
+     * @return int Identifier, or FALSE if non-existent
+     */
+    public function getLibraryId($library, $libString = null) {
+        if (!$libString) {
+            $libString = self::libraryToString($library);
+        }
+
+        // Check if this information has been saved previously into the cache.
+        $libcache = \cache::make('core', 'h5p_libraries');
+        $librarykey = helper::get_cache_librarykey($libString);
+        $libraryId = $libcache->get($librarykey);
+        if ($libraryId === false) {
+            $libraryId = $this->h5pF->getLibraryId($library['machineName'], $library['majorVersion'], $library['minorVersion']);
+            $libcache->set($librarykey, $libraryId);
+        }
+
+        return $libraryId;
+    }
+
 }
diff --git a/h5p/classes/file_storage.php b/h5p/classes/file_storage.php
@@ -29,6 +29,8 @@
 use Moodle\H5peditorFile;
 use Moodle\H5PFileStorage;
 
+// phpcs:disable moodle.NamingConventions.ValidFunctionName.LowercaseMethod
+
 /**
  * Class to handle storage and export of H5P Content.
  *
@@ -86,15 +88,27 @@ public function saveLibrary($library) {
             'contextid' => $this->context->id,
             'component' => self::COMPONENT,
             'filearea' => self::LIBRARY_FILEAREA,
-            'filepath' => '/' . H5PCore::libraryToString($library, true) . '/',
-            'itemid' => $library['libraryId']
+            'filepath' => '/' . H5PCore::libraryToFolderName($library) . '/',
+            'itemid' => $library['libraryId'],
         ];
 
         // Easiest approach: delete the existing library version and copy the new one.
         $this->delete_library($library);
         $this->copy_directory($library['uploadDirectory'], $options);
     }
 
+    /**
+     * Delete library folder.
+     *
+     * @param array $library
+     */
+    public function deleteLibrary($library) {
+        // Although this class had a method (delete_library()) for removing libraries before this was added to the interface,
+        // it's not safe to call it from here because looking at the place where it's called, it's not clear what are their
+        // expectation. This method will be implemented once more information will be added to the H5P technical doc.
+    }
+
+
     /**
      * Store the content folder.
      *
@@ -162,7 +176,7 @@ public function exportContent($id, $target) {
      * @param string $target Where the library folder will be saved
      */
     public function exportLibrary($library, $target) {
-        $folder = H5PCore::libraryToString($library, true);
+        $folder = H5PCore::libraryToFolderName($library);
         $this->export_file_tree($target . '/' . $folder, $this->context->id, self::LIBRARY_FILEAREA,
                 '/' . $folder . '/', $library['libraryId']);
     }
@@ -432,7 +446,7 @@ public function moveContentDirectory($source, $contentid = null) {
 
         // Move all temporary content files to editor.
         $it = new \RecursiveIteratorIterator(
-            new \RecursiveDirectoryIterator($contentsource,\RecursiveDirectoryIterator::SKIP_DOTS),
+            new \RecursiveDirectoryIterator($contentsource, \RecursiveDirectoryIterator::SKIP_DOTS),
             \RecursiveIteratorIterator::SELF_FIRST
         );
 
@@ -583,7 +597,7 @@ public function delete_library(array $library): void {
         global $DB;
 
         // A library ID of false would result in all library files being deleted, which we don't want. Return instead.
-        if ($library['libraryId'] === false) {
+        if (empty($library['libraryId'])) {
             return;
         }
 

diff --git a/h5p/classes/framework.php b/h5p/classes/framework.php
@@ -447,6 +447,9 @@ public function t($message, $replacements = array()) {
             'Keywords already exists!' => 'keywordsExits',
             'Some of these keywords already exist' => 'someKeywordsExits',
             'Assistive Technologies label' => 'a11yTitle:label',
+            'width' => 'width',
+            'height' => 'height',
+            'Missing main library @library' => 'missingmainlibrary',
         ];
 
         if (isset($translationsmap[$message])) {

diff --git a/h5p/h5plib/v124/joubel/core/h5p-default-storage.class.php b/h5p/h5plib/v124/joubel/core/h5p-default-storage.class.php
@@ -1,5 +1,7 @@
 <?php
 
+namespace Moodle;
+
 /**
  * File info?
  */
@@ -9,13 +11,13 @@
  * operations using PHP's standard file operation functions.
  *
  * Some implementations of H5P that doesn't use the standard file system will
- * want to create their own implementation of the \H5P\FileStorage interface.
+ * want to create their own implementation of the H5PFileStorage interface.
  *
  * @package    H5P
  * @copyright  2016 Joubel AS
  * @license    MIT
  */
-class H5PDefaultStorage implements \H5PFileStorage {
+class H5PDefaultStorage implements H5PFileStorage {
   private $path, $alteditorpath;
 
   /**
@@ -39,10 +41,10 @@ function __construct($path, $alteditorpath = NULL) {
    *  Library properties
    */
   public function saveLibrary($library) {
-    $dest = $this->path . '/libraries/' . \H5PCore::libraryToFolderName($library);
+    $dest = $this->path . '/libraries/' . H5PCore::libraryToFolderName($library);
 
     // Make sure destination dir doesn't exist
-    \H5PCore::deleteFileTree($dest);
+    H5PCore::deleteFileTree($dest);
 
     // Move library folder
     self::copyFileTree($library['uploadDirectory'], $dest);
@@ -64,7 +66,7 @@ public function saveContent($source, $content) {
     $dest = "{$this->path}/content/{$content['id']}";
 
     // Remove any old content
-    \H5PCore::deleteFileTree($dest);
+    H5PCore::deleteFileTree($dest);
 
     self::copyFileTree($source, $dest);
   }
@@ -76,7 +78,7 @@ public function saveContent($source, $content) {
    *  Content properties
    */
   public function deleteContent($content) {
-    \H5PCore::deleteFileTree("{$this->path}/content/{$content['id']}");
+    H5PCore::deleteFileTree("{$this->path}/content/{$content['id']}");
   }
 
   /**
@@ -137,7 +139,7 @@ public function exportContent($id, $target) {
    *  Folder that library resides in
    */
   public function exportLibrary($library, $target, $developmentPath=NULL) {
-    $folder = \H5PCore::libraryToFolderName($library);
+    $folder = H5PCore::libraryToFolderName($library);
 
     $srcPath = ($developmentPath === NULL ? "/libraries/{$folder}" : $developmentPath);
     self::copyFileTree("{$this->path}{$srcPath}", "{$target}/{$folder}");
@@ -297,7 +299,7 @@ public function getContent($file_path) {
    * Save files uploaded through the editor.
    * The files must be marked as temporary until the content form is saved.
    *
-   * @param \H5peditorFile $file
+   * @param H5peditorFile $file
    * @param int $contentid
    */
   public function saveFile($file, $contentId) {

diff --git a/h5p/h5plib/v124/joubel/core/h5p-development.class.php b/h5p/h5plib/v124/joubel/core/h5p-development.class.php
@@ -1,5 +1,7 @@
 <?php
 
+namespace Moodle;
+
 /**
  * This is a data layer which uses the file system so it isn't specific to any framework.
  */

diff --git a/h5p/h5plib/v124/joubel/core/h5p-event-base.class.php b/h5p/h5plib/v124/joubel/core/h5p-event-base.class.php
@@ -1,5 +1,7 @@
 <?php
 
+namespace Moodle;
+
 /**
  * The base class for H5P events. Extend to track H5P events in your system.
  *

diff --git a/h5p/h5plib/v124/joubel/core/h5p-file-storage.interface.php b/h5p/h5plib/v124/joubel/core/h5p-file-storage.interface.php
@@ -1,5 +1,7 @@
 <?php
 
+namespace Moodle;
+
 /**
  * File info?
  */
@@ -145,7 +147,7 @@ public function getContent($file_path);
    * Save files uploaded through the editor.
    * The files must be marked as temporary until the content form is saved.
    *
-   * @param \H5peditorFile $file
+   * @param H5peditorFile $file
    * @param int $contentId
    */
   public function saveFile($file, $contentId);

diff --git a/h5p/h5plib/v124/joubel/core/h5p-metadata.class.php b/h5p/h5plib/v124/joubel/core/h5p-metadata.class.php
@@ -1,4 +1,7 @@
 <?php
+
+namespace Moodle;
+
 /**
  * Utility class for handling metadata
  */

diff --git a/h5p/h5plib/v124/joubel/core/h5p.classes.php b/h5p/h5plib/v124/joubel/core/h5p.classes.php
@@ -1,4 +1,9 @@
 <?php
+
+namespace Moodle;
+
+use ZipArchive;
+
 /**
  * Interface defining functions the h5p library needs the framework to implement
  */
@@ -1649,12 +1654,14 @@ private function saveLibraries() {
         H5PMetadata::boolifyAndEncodeSettings($library['metadataSettings']) :
         NULL;
 
-      // Save library folder
-      $this->h5pC->fs->saveLibrary($library);
-
+      // MOODLE PATCH: The library needs to be saved in database first before creating the files, because the libraryid is used
+      // as itemid for the files.
       // Update our DB
       $this->h5pF->saveLibraryData($library, $new);
 
+      // Save library folder
+      $this->h5pC->fs->saveLibrary($library);
+
       // Remove cached assets that uses this library
       if ($this->h5pC->aggregateAssets && isset($library['libraryId'])) {
         $removedKeys = $this->h5pF->deleteCachedAssets($library['libraryId']);
@@ -2132,15 +2139,15 @@ class H5PCore {
    *
    * @param H5PFrameworkInterface $H5PFramework
    *  The frameworks implementation of the H5PFrameworkInterface
-   * @param string|\H5PFileStorage $path H5P file storage directory or class.
+   * @param string|H5PFileStorage $path H5P file storage directory or class.
    * @param string $url To file storage directory.
    * @param string $language code. Defaults to english.
    * @param boolean $export enabled?
    */
   public function __construct(H5PFrameworkInterface $H5PFramework, $path, $url, $language = 'en', $export = FALSE) {
     $this->h5pF = $H5PFramework;
 
-    $this->fs = ($path instanceof \H5PFileStorage ? $path : new \H5PDefaultStorage($path));
+    $this->fs = ($path instanceof H5PFileStorage ? $path : new H5PDefaultStorage($path));
 
     $this->url = $url;
     $this->exportEnabled = $export;
@@ -3307,21 +3314,23 @@ private static function getTimeFactor() {
    * @return string
    */
   private static function hashToken($action, $time_factor) {
-    if (!isset($_SESSION['h5p_token'])) {
+    global $SESSION;
+
+    if (!isset($SESSION->h5p_token)) {
       // Create an unique key which is used to create action tokens for this session.
       if (function_exists('random_bytes')) {
-        $_SESSION['h5p_token'] = base64_encode(random_bytes(15));
+        $SESSION->h5p_token = base64_encode(random_bytes(15));
       }
       else if (function_exists('openssl_random_pseudo_bytes')) {
-        $_SESSION['h5p_token'] = base64_encode(openssl_random_pseudo_bytes(15));
+        $SESSION->h5p_token = base64_encode(openssl_random_pseudo_bytes(15));
       }
       else {
-        $_SESSION['h5p_token'] = uniqid('', TRUE);
+        $SESSION->h5p_token = uniqid('', TRUE);
       }
     }
 
     // Create hash and return
-    return substr(hash('md5', $action . $time_factor . $_SESSION['h5p_token']), -16, 13);
+    return substr(hash('md5', $action . $time_factor . $SESSION->h5p_token), -16, 13);
   }
 
   /**