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-3116: Made Section identifier nonconvertible #239

Merged
merged 3 commits into from
Jul 19, 2022
Merged

IBX-3116: Made Section identifier nonconvertible #239

merged 3 commits into from
Jul 19, 2022

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Jul 12, 2022 •
edited
Loading

Question Answer
JIRA issue IBX-3116
Type bug
Target Ibexa DXP version v3.3
BC breaks no

As the title suggests.

@barw4 barw4 requested a review from a team July 12, 2022 12:26
@barw4 barw4 self-assigned this Jul 12, 2022
adamwojs
adamwojs requested changes Jul 12, 2022
View reviewed changes
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

Are you sure you know what you are doing?

Instead of changing type to string please use raw flag on identifier field type or change string normalization in a way to that underscores will be accepted.

Please note that similar changes need to be applied for elasticsearch integration and integration test is mandatory in this case.

@barw4
Copy link
Member Author

barw4 commented Jul 12, 2022 •
edited
Loading

@adamwojs yes, I'm sure. I was thinking about changing converter to accept "-", but that drastically changes schema in other places.

Besides, we do have strings here as well:

ezplatform-solr-search-engine/lib/FieldMapper/ContentFieldMapper/BlockDocumentsBaseContentFields.php

Lines 187 to 190 in 9d66536

'content_object_state_identifiers',
$this->getObjectStateIdentifiers($contentInfo->id),
new FieldType\MultipleStringField()
),
- those are also identifiers?

@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

@barw4 barw4 requested review from adamwojs and a team July 14, 2022 15:23
@adamwojs adamwojs requested a review from a team July 14, 2022 19:06
Steveb-p
Steveb-p approved these changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

Please be aware that in cases like this describing what the PR contents are does not describe why are they necessary.

@barw4 barw4 changed the title IBX-3116: Changed content section identifier to string field IBX-3116: Made Section identifier nonconvertible Jul 15, 2022
@bogusez bogusez self-assigned this Jul 18, 2022
@adamwojs adamwojs merged commit 439ae14 into 3.3 Jul 19, 2022
@adamwojs adamwojs deleted the ibx-3116-section-identifier-s branch July 19, 2022 13:50
@adamwojs
Copy link
Member

Could you please merge up changes?

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

@bogusez bogusez bogusez approved these changes

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

@adamwojs adamwojs adamwojs approved these changes

Assignees

@adamwojs adamwojs

@alongosz alongosz

@barw4 barw4

@bogusez bogusez

Labels
Bug QA Approved Ready for MERGE
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@barw4 @adamwojs @Steveb-p @bogusez @alongosz