Skip to content

Refactor VideoSerializationHandler for asset properties - #481

Open
dpfaffenbauer wants to merge 5 commits into
pimcore:2026.xfrom
dpfaffenbauer:patch-1
Open

Refactor VideoSerializationHandler for asset properties#481
dpfaffenbauer wants to merge 5 commits into
pimcore:2026.xfrom
dpfaffenbauer:patch-1

Conversation

@dpfaffenbauer

@dpfaffenbauer dpfaffenbauer commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Refactored video asset serialization to use private methods for duration, width, and height retrieval with error handling.

Changes in this pull request

Resolves pimcore/platform-version#245

Additional info

Refactored video asset serialization to use private methods for duration, width, and height retrieval with error handling.
Copilot AI review requested due to automatic review settings July 23, 2026 12:52
@pimcore-deployments
pimcore-deployments marked this pull request as draft July 23, 2026 12:53
@dpfaffenbauer
dpfaffenbauer marked this pull request as ready for review July 23, 2026 12:55
@pimcore-deployments
pimcore-deployments marked this pull request as draft July 23, 2026 12:55

Copilot AI 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.

Pull request overview

This PR refactors VideoSerializationHandler so that the duration, width, and height of a video asset are read through dedicated private methods that wrap the getter calls in try/catch and log errors instead of letting exceptions propagate during serialization. It follows the existing pattern already used by getImageThumbnail() in the same class.

Changes:

  • Replace direct $asset->getDuration()/getWidth()/getHeight() calls in getAdditionalSystemFields() with calls to new private helpers.
  • Add getDuration(), getWidth(), and getHeight() helpers that catch Exception and log a failure message.
Comments suppressed due to low confidence (1)

src/Service/Serializer/AssetTypeSerializationHandler/VideoSerializationHandler.php:109

  • $video is undefined here — the parameter of this method is named $asset. In the catch path this accesses an undefined variable and then calls getId() on null, which throws an Error that is not caught by catch (Exception), so the error-logging path itself crashes. Use $asset->getId().
                $video->getId() .

Updated error logging to use asset ID instead of video ID for consistency.
@dpfaffenbauer
dpfaffenbauer marked this pull request as ready for review July 23, 2026 12:58
@pimcore-deployments
pimcore-deployments marked this pull request as draft July 23, 2026 12:58
@dpfaffenbauer
dpfaffenbauer marked this pull request as ready for review July 23, 2026 13:08
@pimcore-deployments
pimcore-deployments marked this pull request as draft July 29, 2026 12:43
@pimcore-deployments

Copy link
Copy Markdown
Contributor

👋 Friendly reminder — this pull request is still a draft.

A guardrail converted it to draft because something needed fixing, and there has been no activity since. If nothing changes, it will be closed in 14 days.

To keep it open, just take one of these steps:

  • Address the guardrail comment above, then press Ready for review.
  • Or leave a comment explaining that it is still in progress — any activity resets this timer.

Closing is not rejection: the branch and the discussion stay, and the PR can be reopened at any time.

When fixed, press Ready for review to re-run the checks.

@dpfaffenbauer
dpfaffenbauer marked this pull request as ready for review August 13, 2026 06:05
@sonarqubecloud

Copy link
Copy Markdown

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.

[Generic Data Index] Bug in VideoSerializationHandler with missing asset file

4 participants