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.

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

IBX-7653: Add IsContainer criterion support #63

Merged
merged 10 commits into from
May 16, 2024

Conversation

tischsoic
Copy link
Contributor

@tischsoic tischsoic commented Mar 13, 2024

Question Answer
JIRA issue https://issues.ibexa.co/browse/IBX-7653
Type improvement
Target Ibexa version v4.6
BC breaks yes/no

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping for example @ibexa/php-dev for back-end changes and/or @ibexa/javascript-dev for front-end changes).

@tischsoic tischsoic self-assigned this Mar 13, 2024
@tischsoic tischsoic changed the base branch from main to 4.6 March 25, 2024 10:18
@tischsoic tischsoic force-pushed the IBX-7653-images-visible-in-tree-browser-in-image-picker branch from 8a2a19f to f8d8ed6 Compare March 25, 2024 10:53
@tischsoic tischsoic marked this pull request as ready for review March 25, 2024 10:53
@tischsoic tischsoic requested a review from a team March 25, 2024 10:53
@tischsoic tischsoic force-pushed the IBX-7653-images-visible-in-tree-browser-in-image-picker branch from 0e8149f to 01555b4 Compare April 4, 2024 11:13
src/lib/Query/Content/CriterionVisitor/IsContainer.php Outdated Show resolved Hide resolved
src/lib/Query/Location/CriterionVisitor/IsContainer.php Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This is almost the same code as the Content\CriterionVisitor. Consider extracting common code.

Copy link
Contributor Author

@tischsoic tischsoic Apr 5, 2024

Choose a reason for hiding this comment

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

From what I have checked, we do not extract the common part (e.g. LocationIdIn, LocationRemoteIdIn etc.) so I didn't want to introduce another abstraction just for IsContainer.

Copy link
Member

Choose a reason for hiding this comment

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

From what I have checked, we do not extract the common part (e.g. LocationIdIn, LocationRemoteIdIn etc.) so I didn't want to introduce another abstraction just for IsContainer.

This is a technical debt that can be easily mitigated. Sonar Cloud analysis is valid.
My question here would be if this can be refactored into a common base for all visitors or just for IsContainer? The latter approach is a minimum requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say that extracting this code into an abstract is debatable in this case. Those classes share code, but are for different domains.

I would argue that a trait is a candidate, especially since there are valid cases where code related to checking value is different for a Criterion. We basically need to inject a validation method to ensure that argument passed is correct. This is a specific behavior that we need for a particular set of Criterions.

Thoughts?

@Steveb-p Steveb-p requested a review from a team April 4, 2024 12:31
@tischsoic tischsoic requested a review from Steveb-p April 5, 2024 07:51
{
$value = $criterion->value;

if (!is_array($value) || !is_bool($value[0])) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really accept array of values ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, every scalar value is wrapped in an array here:
https://github.com/ibexa/core/blob/86d44648ace5e4ab48b9deae9e327145130113c9/src/contracts/Repository/Values/Content/Query/Criterion.php#L94
it was discussed with PHP and AFAICR we plan to change it in 5.0.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

@alongosz alongosz requested a review from a team April 19, 2024 12:09
@tischsoic tischsoic force-pushed the IBX-7653-images-visible-in-tree-browser-in-image-picker branch from bb387fe to 242b40f Compare May 10, 2024 07:44
@tischsoic tischsoic force-pushed the IBX-7653-images-visible-in-tree-browser-in-image-picker branch from 242b40f to 98eb4b6 Compare May 16, 2024 07:05
Copy link

sonarcloud bot commented May 16, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@tischsoic
Copy link
Contributor Author

CI is failing because [TMP] commits have been removed.

@dew326 dew326 merged commit d8e2746 into 4.6 May 16, 2024
16 of 19 checks passed
@dew326 dew326 deleted the IBX-7653-images-visible-in-tree-browser-in-image-picker branch May 16, 2024 07:20
@tischsoic
Copy link
Contributor Author

Merged up:

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

Successfully merging this pull request may close these issues.

7 participants