- Issue created by @a.dmitriiev
- 🇺🇸United States phenaproxima Massachusetts
I am a fairly strong -1 on this.
It's just...too fiddly. It's trying to make a generic action to do something that is inherently not generic. Generic actions should only be added to do things that work the same way on every config entity type (or nearly every type). To me, this is 1000% a case to create a custom action in Search API.
If we were going to pursue something like this, we'd have to have think through it very carefully (all the use cases and implications thereof). An action like what you're proposing, to me, feels like treating all config as though it's composed of dumb arrays. Which is true on one level (and it's certainly true of simple config)...but very dangerous to generalize. Config entities have setter methods and APIs for a reason.
So my preference is to close this issue as a wontfix, and pursue a more specific action in Search API to configure indexes the way we want.
- 🇩🇪Germany a.dmitriiev
Ok, thank you for the feedback. Let's continue in corresponding projects.
- 🇦🇹Austria drunken monkey Vienna, Austria
Thanks for creating this issue!
It’s indeed not ideal UX that you have to configure the view mode for each bundle separately, especially if you add bundles after configuring the row style or field. (The field will at least post warnings to the log to point the user to this problem.)For field-based processor (like Tokenizer, Ignore Case, etc.) we already have something similar, the “Enable on all supported fields” option added in #2867809: Add new fulltext fields to enabled fields processors → . That solved a very similar problem, namely the need to reconfigure all these processors when adding a new fulltext field. The solution here would be a bit more complicated, as it’s a select instead of a checkbox and we still need separate settings for different datasources (entity types), but seems something similar could work here, too.
Maybe like this: For each datasource, we have a select box with all the different view modes for that datasource, plus an option that says something like “Select per bundle”. When that latter option is selected, we display the additional select boxes for each bundle, otherwise the datasource setting juts applies to all bundles and the bundle-specific settings get hidden.
Alternatively, the datasource-level setting could just have the view modes as options, the bundle-specific select boxes could always be visible but we add a new default option to them, “Use datasource-level default”. (All labels mentioned here up for debate.)If you want to work on an MR implementing something like this, that would be great!
- Merge request !187Issue #3483366: Add global setting for 'rendered_item' field plugin → (Merged) created by a.dmitriiev
- 🇩🇪Germany a.dmitriiev
I have started a draft MR. At the moment I made the changes only to the "Rendered Item" plugin for index field. Not fully tested yet. I will continue tomorrow, as this feature is really needed for Drupal CMS.
- 🇩🇪Germany a.dmitriiev
Please disregard patches and review MR. The patches are just needed to be used with stable 1.35 version.
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
So this change does not need a new config action, it only needs this change and the config action/recipe will set it to the new :all selection?
I think this is a good solution and I can't find anything to complain about really, apart from there not being any test coverage.
At the least I think we should add to the RenderedItemTest.
- 🇦🇹Austria drunken monkey Vienna, Austria
Thanks for your work on this!
I went over the code and found a few places where a) the wording of UI text could be improved or b) the code wasn’t working 100% correctly in all cases anymore. All of this should now be fixed in the MR. Please test/review my changes and tell me what you think.In any case, I agree with @borisson_, this definitely needs some test coverage. The number of potential code paths in
\Drupal\search_api\Plugin\search_api\processor\RenderedItem::addFieldValues()
borders on disconcerting.
I did, however, now already adapt all the views included in the module (mainly test views) that use theSearchApiRow
plugin to utilize the new:default
option, so we should at least get some very rough test coverage for the Views part that way. As our test coverage for the Views integration is generally rather shoddy, I’m not sure whether doing more is necessary, though it would of course be great. - 🇩🇪Germany a.dmitriiev
Thank you all for the feedback and review. I will try to add tests and go over all suggestions asap.
- 🇩🇪Germany a.dmitriiev
I have added the test for global default view mode set. The test also includes the override for one bundle. Please review.
- 🇩🇪Germany a.dmitriiev
Updated patch for 1.35 to use in Drupal CMS search track recipe
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
I think this is all the test coverage we need. Looks good to me.
- 🇦🇹Austria drunken monkey Vienna, Austria
Great job. Merged!
Thanks again! -
drunken monkey →
committed a7b566b0 on 8.x-1.x authored by
a.dmitriiev →
Issue #3483366 by a.dmitriiev, drunken monkey, borisson_: Added a "...
-
drunken monkey →
committed a7b566b0 on 8.x-1.x authored by
a.dmitriiev →