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

FIX: localized fields from brick and fieldcollections can be added to index #1872

Merged
merged 16 commits into from
Mar 1, 2022

Conversation

BabovicT
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets

Brick and fieldcollections with localazied fields are now indexable.

@dpfaffenbauer
Copy link
Member

psalm issue is fixed: #1873, can you rebase?

@BabovicT
Copy link
Contributor Author

@dominikager I also added Helper for inheritance to localizedFieldGetter.

@dpfaffenbauer
Copy link
Member

@BabovicT Changed to how I imagine it, can you check and test pls?

}

if ($fd instanceof Localizedfield) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dpfaffenbauer I think it would work but $fd is not instance of LocalizedField. It is instance of field under the localizedfields. Thats why I used
if ($item->getDefinition()->getFieldDefinition('localizedfields')) {

Copy link
Member

Choose a reason for hiding this comment

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

but that only checks if there are localized-fields in the fieldcollection... which is useless basically. We need to know if the selected field is localized or not. But I see that there is an issue here. I will check again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason that checks if that field is inside localized-fields. If you have input and localized input that will be true only for localized input. Not sure why is that the case tho.

Copy link
Member

Choose a reason for hiding this comment

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

changed some more, worked for me now.

@BabovicT
Copy link
Contributor Author

BabovicT commented Mar 1, 2022

Working for me too.

@dpfaffenbauer dpfaffenbauer merged commit 6f0d371 into coreshop:master Mar 1, 2022
@dpfaffenbauer
Copy link
Member

thanks @BabovicT

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.

2 participants