Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IBX-2264: Handled empty Thumbnail ProxyObject #91

Merged
merged 1 commit into from
Mar 16, 2022
Merged

IBX-2264: Handled empty Thumbnail ProxyObject #91

merged 1 commit into from
Mar 16, 2022

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Feb 21, 2022 •
edited
Loading

Question Answer
JIRA issue IBX-2264
Type bug
Target version v3.3
BC breaks no
Tests pass yes
Doc needed no

Right now getThumbnail can never be null (but this is what we have in the interface so this is an inconsistency) due to the implementation of ProxyObject, this can cause problems like the one described in the ticket. Potentially later on we could implement some decoy thumbnail or allow Thumbnail with null as a resource and change the interface.

Related ezplatform-kernel PR: ezsystems/ezplatform-kernel#288

TODO:

  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@barw4 barw4 self-assigned this Feb 21, 2022
@barw4 barw4 mentioned this pull request Feb 21, 2022
Merged
6 tasks
@barw4 barw4 requested review from adamwojs, ViniTou and a team February 21, 2022 15:34
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ViniTou
Copy link
Contributor

ViniTou commented Mar 4, 2022

@adamwojs
Is there no better way to check if underlying object in proxy is null than accessing one of its properties?

@micszo micszo self-assigned this Mar 16, 2022
@micszo micszo mentioned this pull request Mar 16, 2022
Closed
micszo
micszo approved these changes Mar 16, 2022
View reviewed changes
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

QA Approved on Ibexa Commerce 3.3.15 with diff.

Also tested on NFS.

@micszo micszo removed their assignment Mar 16, 2022
@Steveb-p Steveb-p merged commit 397e700 into 1.3 Mar 16, 2022
@Steveb-p Steveb-p deleted the ibx-2264-handle-missing-thumbnail branch March 16, 2022 15:23
@micszo
Copy link
Member

micszo commented Mar 17, 2022

btw, cross merge workflow for bot it missing in this repo. PR in new org will have to be opened manually.

@barw4 barw4 mentioned this pull request Mar 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konradoboza konradoboza konradoboza approved these changes

@micszo micszo micszo approved these changes

@alongosz alongosz alongosz approved these changes

@damianz5 damianz5 damianz5 approved these changes

@Steveb-p Steveb-p Steveb-p approved these changes

@mikadamczyk mikadamczyk mikadamczyk approved these changes

@adamwojs adamwojs Awaiting requested review from adamwojs

@ViniTou ViniTou Awaiting requested review from ViniTou

Assignees

@barw4 barw4

Labels
Bug QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@barw4 @ViniTou @micszo @mikadamczyk @Steveb-p @damianz5 @alongosz @konradoboza @lserwatka