- Issue created by @useernamee
- Merge request !112#3493780: Support for Search API view as configured through drupal_cms_search reciple. → (Merged) created by useernamee
- 🇸🇮Slovenia useernamee Ljubljana
Ugh, I came full circle and ended with implementing changes from 📌 Use CE Generator to build View rows Needs work .
- 🇦🇹Austria fago Vienna
so this is about supporting search api views - let's make this clearer.
- 🇦🇹Austria fago Vienna
Quick review:
* I don't think the module README is the place for drupal-CMS documentation. The README should stay general and document how to use it with Search API. But concrete drupal-CMS instructions should go elsewhere imo (not sure where), or better be not necessary since automated, but that'S a different story/issue.
* Cannot judge about the details, but let's make sure things work via automated tests. Can we also add some search-api & views config to tests that we run as part of automated tests? I think we could simply copy things over from the recipe, but I'd copy to document what is supported on the long-time and not depend on the recipe since the recipe can change over time. - 🇸🇮Slovenia useernamee Ljubljana
- I did some restructuring and moved search api integration into its own module so views is independent of search api.
- I removed recipe data from README
- Added tests - 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
Beidres the comment on the MR:
The changes to CustomElementsPage.php and CustomElements.php seem good, and I'm confident they are well enough tested by the tests included. They can be committed.
However, the whole lupus_decoupled_search_api module... contains zero code. It's only dependencies on lupus_decoupled_views + search_api_db, and a README.
- Isn't this better off being a recipe, instead of introducing a new 'empty' module?
- Then we can move tests/modules and tests/src in the 'module root' tests/ folder. I see no issue with that.
Needs work for answering that + the above MR comment.
Note I haven't reviewed point 2 of the proposed resolution yet.
- 🇸🇮Slovenia useernamee Ljubljana
> However, the whole lupus_decoupled_search_api module... contains zero code. It's only dependencies on lupus_decoupled_views + search_api_db, and a README.
It contains tests. And I wouldn't want to move tests to lupus_decoupled_views (or any other place) since modules have different purpose.
> Isn't this better off being a recipe, instead of introducing a new 'empty' module?
It is a recipe (https://git.drupalcode.org/project/drupal_cms/-/blob/1.x/recipes/drupal_...) - we test that recipe works with lupus decoupled.
> Then we can move tests/modules and tests/src in the 'module root' tests/ folder. I see no issue with that.
There are other ways to add search functionality to lupus decoupled (with external service). This only tests that Drupal CMS Search recipe works.
- 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
Discussed in person, IMHO no need for me to reiterate here at this moment
- 🇸🇮Slovenia useernamee Ljubljana
- removed the module and moved the tests to the base module.
- updated tests - 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
Point 1: this works. (So, in the end, it's just small changes to get/derive the view mode from the right place. Plus tests/etc.)
Point 2: already done, on https://lupus-decoupled.org/advanced-topics/searches
so, RTBC.
- 🇸🇮Slovenia useernamee Ljubljana
I added a sentence to the docs: https://github.com/drunomics/lupus-decoupled-website/pull/94
- 🇦🇹Austria fago Vienna
- The MR cannot be merged, because it conflicts. Please update and let's merge then
- I added a remark to the docs update, please check. https://github.com/drunomics/lupus-decoupled-website/pull/94#pullrequest... - 🇸🇮Slovenia useernamee Ljubljana
- reRolled the PR https://git.drupalcode.org/project/lupus_decoupled/-/merge_requests/112#...
- added PR remarks https://github.com/drunomics/lupus-decoupled-website/pull/94#pullrequest... -
fago →
committed 55f86552 on 1.x authored by
useernamee →
Issue #3493780 by useernamee, fago, roderik: Support for SearchAPI based...
-
fago →
committed 55f86552 on 1.x authored by
useernamee →
Automatically closed - issue fixed for 2 weeks with no activity.