Skip to content

44965: Failed test: Player - Video: show - #11107

Open
alex40724 wants to merge 5 commits into
ILIAS-eLearning:release_10from
leifos-gmbh:fix_44965
Open

44965: Failed test: Player - Video: show#11107
alex40724 wants to merge 5 commits into
ILIAS-eLearning:release_10from
leifos-gmbh:fix_44965

Conversation

@alex40724

@alex40724 alex40724 commented Feb 11, 2026

Copy link
Copy Markdown
Member

See https://mantis.ilias.de/view.php?id=44965

The PR also includes the text changes requested here:

https://mantis.ilias.de/view.php?id=44964

@alex40724

Copy link
Copy Markdown
Member Author

The label seem to be obligatory for some browsers (e.g. Firefox), so I would not make it an optional parameter even if it breaks backwards compatibility - hence the Jour Fixe label.

@thibsy thibsy removed the jour fixe label Feb 22, 2026
@thibsy

thibsy commented Feb 22, 2026

Copy link
Copy Markdown
Contributor

Sry @alex40724, I didn't have time to review this yet. I therefore removed the JF label again. Please let the UI coordinators add this in the future.

@thibsy thibsy added the css/html Pull requests that propose changes to CSS/SCSS or HTML files. label Feb 24, 2026

@thibsy thibsy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alex40724,

Thx a lot for your contribution to the UI framework!

Please consider the following suggestions. You do not need to follow them, but please indicate shortly why you prefer to do otherwise:

  • Player\Video::withAdditionalSubtitleFile(): to provide JF with an alternative approach, we could add a default assignment of an empty string along with a deprecation notice for 10. This would require some additional logic inside the component and renderer, but it would prevent possible fatals and allows gradual fixing of plugins.

Please implement the following changes:

  • Player\Video::getSubtitleLabels(): please remove this method from the public interface.

I have added the JF label so we can decide on the direction of this PR.

Kind regards,
@thibsy (as UI coordinator)

@thibsy thibsy added jour fixe php Pull requests that update Php code labels Feb 24, 2026
@matthiaskunkel

Copy link
Copy Markdown
Member

Jour Fixe, 09 MAR 2026: We highly appreciate this PR and accept it and the breaking change for 11 and trunk. For 10 we would like to have an additional PR that sets an empty string for the label to prevent problems with already existing plugins.

@thibsy

thibsy commented Mar 13, 2026

Copy link
Copy Markdown
Contributor

@alex40724 I assign this to you for the time being. Thx for working on this.

@thibsy thibsy assigned alex40724 and unassigned thibsy Mar 13, 2026
@alex40724 alex40724 assigned thibsy and unassigned alex40724 Aug 22, 2026
@alex40724

Copy link
Copy Markdown
Member Author

@thibsy The requested changes are applied to the branch.

@thibsy thibsy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alex40724,

Thx for working on this again.

JF only accept the solution with a default value (string $label = '') for ILIAS 10 (see @matthiaskunkel comment above). Since this has been approved before the initial release of ILIAS 11, I would suggest to apply the same fix for ILIAS 11 as well now and only make this parameter mandatory for trunk.

Therefore, please implement the following changes:

  • Optional parameter: please make the new parameter optional so we do not break existing usages.
  • Conflicts: I cannot merge the PR due to conflicts, please rebase the branch onto latest release_10.

Afterwards, this should be good to go. Let me know how you want to proceed with the bugfix and usages for trunk.

Kind regards,
@thibsy (as UI coordinator)

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

Labels

bugfix css/html Pull requests that propose changes to CSS/SCSS or HTML files. kitchen sink php Pull requests that update Php code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants