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

Add option to index child attributes or not #2443

Open
wants to merge 6 commits into
base: 2.10.x
Choose a base branch
from

Conversation

fredden
Copy link
Contributor

@fredden fredden commented Feb 9, 2022

While working with bundle products on a website, the merchant wants to only include attributes of the parent/bundle product not any of its options/children. This configuration option adds the ability for website administrators to make this decision. By default, the existing behaviour is preserved.

@romainruaud
Copy link
Collaborator

Hi @fredden,

having such a config globally is not enough for me, I'd like to be able to also control which attributes are mergeable, and which are not, as it's done "hardcoded" actually, but with the ability to inject the list through DI.

Can you rework this PR ? Anyway, thank you for proposing it.

Best regards

@fredden
Copy link
Contributor Author

fredden commented Feb 15, 2022

Hi @romainruaud. Thanks for the feedback.

Making the list of forbidenChildrenAttributeCodes configurable sounds like a good idea. If that feature existed, we may have been able to use that instead of the approach here.

I think having the setting exposed in the admin makes more sense than via dependency injection though. This way, if a website administrator adds a new attribute, they'll be able to select its index status right away, and not require a deployment to set this.

With an admin configuration option per attribute, I can see it being a chore marking each and every attribute individually as indexable or not; having a global toggle would help then - which is what's being introduced here.

What do you think about putting this global toggle in for now, and working on getting the list configurable as a separate task?

@robaimes
Copy link

robaimes commented May 6, 2022

@fredden Leaving this here for notes, but following the merge of #2465, this file requires it's constructor modifying to also accommodate for the new Config parameter.

@fredden
Copy link
Contributor Author

fredden commented May 6, 2022

Thanks @robaimes. I've updated this pull request to be compatible with the changes introduced in #2465.

@romainruaud I still think that this should be merged, and the list of attributes should be set in admin configuration (not via dependency injection) as a separate task / feature.

@fredden
Copy link
Contributor Author

fredden commented Aug 22, 2022

Hi @rbayet. What can I do to help get this merged in?

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.

3 participants