Problem/Motivation

The Views RSS: Media module currently has a dependency on the Libraries module to load getID3. However, getID3 can be loaded through the composer class loader instead.

Furthermore, getID3 provides additional metadata for multimedia files that might not be necessary for all installations. I suggest we implement a submodule views_rss_media_getid3 that provides getID3 integration.

Proposed resolution

Remove the dependency on Libraries module and getID3, use class loader instead. Provide getID3 integration through a new views_rss_media_getid3 submodule.

Remaining tasks

  1. Write a patch
  2. Review
  3. Commit
  4. Document how to use the composer class loader to load getID3
  5. Update Drupal.org documentation to reflect the new method for loading getID3

User interface changes

The getID3 library used to provide additional metadata for multimedia file formats is now installed through composer. Execute the following command to add this library to your installation:
composer require james-heinrich/getid3

API changes

  1. views_rss_media no longer depends on getID3.
  2. views_rss_media no longer depends on the Libraries module.
  3. getID3 integration is now provided through a new views_rss_media_getid3 submodule.

Data model changes

None.

Comments

idebr created an issue. See original summary.

idebr’s picture

Status: Active » Needs review
StatusFileSize
new15.08 KB

Attached patch removes the dependency on Libraries module and getID3, and uses the class loader instead. It provides getID3 integration through a new views_rss_media_getid3 submodule.

idebr’s picture

StatusFileSize
new15.91 KB
new1.87 KB

Fixed two occurrences where an URL attribute referred to a Drupal URI instead of a public URL, and a copy/paste error.

lendude’s picture

StatusFileSize
new335 bytes
new15.94 KB

This makes the new module compatible with D9.

idebr’s picture

StatusFileSize
new2.46 KB
new17.45 KB

Attached patch replaces methods that were removed in Drupal 9.

idebr’s picture

StatusFileSize
new497 bytes
new17.46 KB

Attached patch fixed the file URL generation.

lendude’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the code and tested locally for client and works great.

gisle’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

The patch does not apply cleanly to the latest dev-snapshot (it needs to be rerolled becuse of changes to views_rss_media.install). The rest apply.

However, enabling the new views_rss_media_getid3 submodule fails with the message:

Views RSS: Media getID3 requires the getID3 library.

I am fine with proposed solution (remove the dependency on Libraries module and getID3, use composerclass loader instead, provide getID3 integration through a new views_rss_media_getid3 submodule). However, there seems to be some bits missing. To use composer to load an external library, I belive the library needs to be declared as a requirement in the module's composer.json, and there is none.

Also, the documention for the module needs to be updated to reflect the altered method for getting the library. Don't go ahead and edit the documentation wiki on Drupal.org, instead add it to the issue summary, in the section "User interface changes". The new documentation will be reviewed along with the code changes.

idebr’s picture

Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new17.42 KB

#8 A Drupal submodule cannot list its own requirements, for example in views_rss_media_getid3/composer.json so you either have add a requirement in the root of the repository or install it separately.

Attached patch is a reroll against the latest dev
I also updated the 'User interface changes' in the issue summary with the new installation instructions for the getID3 library.

  • idebr authored df6b0ff on 8.x-1.x
    Issue #3068934 by idebr: Fixed libraries dependency, use class loader
    
gisle’s picture

Status: Needs review » Fixed

Reviewed and found to be ready. Added a README.md with instructions about installing the submodule.

Pushed to the latest snapshot of 8.x-1.x-dev. Thanks for the patch.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.