Remove vendor directory and composer.lock from project repository

Bumping this to major because it means that fully updating to H5P 1.25 requires an update to the vendor directory. I've committed an issue that allows update via composer, but that makes the app vendor directory out of sync with the module one #3411479: Allow update to H5P editor/core 1.25.

H5P uses some css/js from this directory though, so it's not as easy as deleting it, those would have to be included another way. A possible solution would be a build step for the module to extract those files from h5p core and add them in a css/js vendor directory without any of the PHP code though. Maybe just be deleting the PHP files from /vendor?

The /h5p/vendor/ directory can't really be deleted since not only are so many CSS and JS files referenced in code, but it's also in the resize JS link in embeds, so removing it would somewhat break other sites that have embedded an H5P. Also, the composer.json needs to lock down a specific version of the dependencies in order to have a match between the dependencies included in the module's /vendor and the dependencies installed by composer in the project's root /vendor. The merge request in #3420268: Support h5p/h5p-core:1.26 and h5p/h5p-editor:1.25 deletes the autoloader and all php files included in the module, and keeps the composer.lock file in order for the version installed in the project /vendor to match the CSS and JS shipped with the module's /vendor directory.

Merge request 27 removes the vendor directory and adds a build step using a custom Drush command drush h5p:copy-assets to copy assets, such as JS and CSS, to ./assets/h5p-core and ./modules/h5peditor/assets/h5p-editor. The Drush command outputs every source/destination copied. The Readme.md is updated to document the Drush command.

This MR doesn't deal with updating the version of the h5p dependencies. It focuses exclusively on removing the vendor directory and creating a post-install build step using Drush.

The issue of "resize JS link in embeds" was ignored. Sites with that feature can create redirects to fix it.

@shaundychko

So were on the latest alpha, I then require dev so I can test your patch, but I also need the patch from #3497789 but no matter what order I can't apply both patches. What am I doing wrong, since https://www.drupal.org/project/h5p/issues/3497789#comment-15941539 makes it sound like it should work.

Here is my patch in composer, I can't get both to work

"drupal/h5p": {
                "https://www.drupal.org/project/h5p/issues/3194707": "https://www.drupal.org/files/issues/2021-01-26/h5p-fix-keeping-h5p-content-id-3194707-0.patch",
                "https://www.drupal.org/project/h5p/issues/3309446": "https://git.drupalcode.org/project/h5p/-/merge_requests/27.diff",
                "https://www.drupal.org/project/h5p/issues/3497789": "https://git.drupalcode.org/project/h5p/-/merge_requests/26.diff"
            },

Ok I think I found my answer https://www.drupal.org/project/h5p/issues/3499799

Don't really think the module can require a separate drush build step after every composer update/install.

What about a script that deletes all files from the vendor/ directory except the CSS/JS?

That way the runtime code doesn't have to change, but the PHP would no longer be in the folders. This would then have to be run when releases are made by the maintainers, but not by individual users of the module.

It might also be worth filing an upstream issue against h5p itself to ask them to split the PHP and css/js assets into two separate repositories, then the CSS/JS could be brought in with libraries module are similar standard approaches.

drupal/core-vendor-hardening might have ideas to borrow for removing all the php files.

Could we use getInstallPath to move the files to the public directory at runtime. We might need to use hook_library_info_alter to set the drupal library to the correct public file path.

I was recently told about another module that provides functionality for exactly this. The Vendor stream wrapper module provides an API to allow modules to include css/etc through a drupal route. Looks promising, but, if we want to use Drupal's library system we might still need to use hook_library_info_alter.

First pass was a bust. The css included in the package has some relative urls that didn't play well with the Vendor Stream Wrapper module. I am shifting to another approach based on the same mechanism as the asset injector module. Essentially, I will copy the files into the default assets stream wrapper and serve them directly from there.

I have gone ahead and rebased the branch and rebuilt it to automatically copy the js/css/etc into the assets://h5p files location. Things are working pretty well. I have run into one issue with the language support. It doesn't seem to find the language js files.

Haven't done a full review yet but +1 to copying the assets across. Does the MR so far handle deleting the assets and copying them again so they can be updated?

They should be overwritten when recopied so deletion shouldn't be necessary. That should handle any updates to the h5p packages. The files themselves are already tightly coupled, so if an update to h5p-core or h5p-editor happens that changes the files we would need to deal with that update specifically anyway.

Something that this brought to light are some optimizations that aught to be done, in another issue, regarding the embedding. There is also a php file that was being included embed.php. I is basically just a template. It would be nice if, up stream, this file got turned into a callback or, maybe, we turn it into a twig template.

The change I made is to use this embed.php file directly from the vendor.

    // Render the page and add to the response
    ob_start();
    include InstalledVersions::getInstallPath("h5p/h5p-core") . '/embed.php';
    $response['#markup'] = ob_get_clean();

I created #3546025: Use a twig template instead of capturing the output of embed.php to handle the embed issue.

This issue is now ready for review.

Some issues that came up during development that bear special scrutiny:

• H5P Editor widget behaviors.
• Content/H5P Libraries admin page.
• Running library upgrades (hard to test since this happens automatically and old H5Ps are difficult to source).
• At least one core module, locale, presented issues that needed to be resolved. Other core modules may have similar issues, and these issues may only present themselves after configuration. We're relying on the community to help us sort these out.
• With a translated site, some errors were reported occasionally.

This looks good, marking RTBC. Things that have been tested in both Drupal 10 and Drupal 11:

Check asset mirroring works:

• Make sure the vendor assets haven't been mirrored to /sites/default/files/h5p, clear cache, then view a node with an H5P field.
• Same as above, but visit the H5P editor for a node with an H5P field.
• Same as above, but view an H5P embed route

Check paths have been updated:

• For each of: H5P node view, H5P edit view, and H5P embed views
• Open dev console network tab
• Ensure the only CSS/JS files loaded with "vendor" in them are part of Drupal's core vendor folder
• Ensure H5P CSS/JS files load with the "sites/default/files/h5p/h5p-core/" and "sites/default/files/h5p/h5p-editor/" paths
• Ensure no H5P CSS/JS files return a 404 status (with the exception of the file mentioned in #3546467: Content Hub Metadata Cache Route Not Defined

Aggregation

• Turn Drupal CSS/JS aggregation on and check that H5Ps load
• Turn Drupal CSS/JS aggregation off and check thatn H5Ps load

Thanks everyone for all your work on this big MR. Merged and marking as fixed.