Skip to content

[service.subtitles.opensubtitles-com] 1.0.9#2802

Open
opensubtitlesdev wants to merge 6 commits intoxbmc:matrixfrom
opensubtitlesdev:matrix
Open

[service.subtitles.opensubtitles-com] 1.0.9#2802
opensubtitlesdev wants to merge 6 commits intoxbmc:matrixfrom
opensubtitlesdev:matrix

Conversation

@opensubtitlesdev
Copy link

Description

Updated addon with recent pull requests and a new feature.

  • added Test Connection button in settings to verify credentials and view account info
  • show specific error message when search or download fails
  • added search results caching with configurable duration
  • fixed trailing slash on subdirectory folder path

Checklist:

  • [ X] My code follows the add-on rules and piracy stance of this project.
  • [ X] I have read the CONTRIBUTING document
  • [ X] Each add-on submission should be a single commit with using the following style: [script.foo.bar] 1.0.0

@kodiai
Copy link

kodiai bot commented Mar 1, 2026

Kodiai Review Summary

What Changed

Updated addon with test connection feature, improved error handling, search results caching, and trailing slash fix for subdirectory paths.

Reviewed: core logic, docs

Strengths

  • ✅ Added specific error messages (32007, 32008, 32009) for TooManyRequests, ServiceUnavailable, and ProviderError exceptions in both search and download flows, improving user experience
  • ✅ Fixed trailing slash inconsistency on subdirectory folder path (special://temp/oss/)

Observations

Impact

[MAJOR] service.subtitles.opensubtitles-com/resources/lib/os/provider.py (212): MD5 used for non-cryptographic purpose without clarifying comment
MD5 is deprecated for security purposes. While this usage (cache key generation) is non-cryptographic, the code should include a comment clarifying this is for cache keying only, not security.

[MAJOR] service.subtitles.opensubtitles-com/test_connection.py (44): Bare except clause catches all exceptions including broad Exception type
Catches generic Exception which can hide SystemExit, KeyboardInterrupt, and other critical exceptions. Should catch specific exception types or at minimum use BaseException filtering.

[MEDIUM] service.subtitles.opensubtitles-com/resources/lib/subtitle_downloader.py (139-140): Bare except clause without exception type
Missing exception type specification when handling translatePath fallback for older Kodi versions. Should catch specific exceptions (e.g., AttributeError) to avoid silencing unexpected errors.

[MEDIUM] service.subtitles.opensubtitles-com/resources/lib/os/provider.py (197): Potential ValueError on invalid cache_setting conversion
Converts cache_setting using int(float(cache_setting)) which can raise ValueError if setting contains non-numeric string. While wrapped in try-except, the broad Exception catch may hide other issues.

Suggestions

  • Optional: Consider using SHA256 instead of MD5 for cache keys to follow modern hashing best practices, even for non-cryptographic use
  • Optional: Add validation in settings.xml to ensure search_cache_duration is numeric-only to prevent ValueError

Verdict

🔴 Address before merging -- 2 blocking issue(s) found (CRITICAL/MAJOR)

Review Details
  • Files reviewed: 13
  • Lines changed: +257 -24
  • Profile: balanced (auto, lines changed: 281)
  • Author: regular (default)
  • Findings: 0 critical, 2 major, 2 medium, 0 minor (includes 4 from summary observations)
  • Review completed: 2026-03-01T05:18:08.556Z
  • Keyword parsing:
    • focus hints: [SERVICE.SUBTITLES.OPENSUBTITLES-COM]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants