@filsterjisah, This sounds like a useful addition! Let me know if there's anything I can do to help!
@fathershawn, thank you!
I'm happy to accept a patch with tests!
I prefer to manually verify every bug/feature as part of my review process, so including a very brief overview of how to demonstrate the changed behavior would be appreciated (I can often figure it out myself, but if I cannot then I sometimes need to delay merging the patch and ask for clarification).
If there's a similar feature in search_api_opensearch → , following their approach to the problem would make my co-maintainers more comfortable with the change as well.
For patches that are "pretty close", I have no issues with adding a few extra tests, fixing small lints, adding a bit of documentation, etc. to help get it over the finish line (within reason).
Merged! Thanks everyone! I will update this issue when the change is released.
I've reviewed the code and it looks good.
I can easily reproduce the error in manual tests on the 8.x-7.x branch (and I think it's common enough that I've bumped the Priority). I can also confirm that the code in the merge request fixes the error.
There were no automated tests in the merge request, so I've added some.
I found that the Cluster::load() call in IndexFactory::getIndexName() makes it hard to test. That code was there before, though, so I think it's out-of-scope to change that. However, I can test some of the code around it.
I also wrote some tests for the getClusterId(), getElasticClient(), getFuzziness(), and isAvailable() functions in SearchApiElasticsearchBackend. I'm assuming that ElasticsearchTest (which extends \Drupal\Tests\search_api\Kernel\BackendTestBase) tests the other functions that were modified in this merge request (i.e.: testAddFacets(), testDeleteItems(), testFieldsUpdated(), testIndexItems(), testRemoveIndex(), testSearch(), testUpdateIndex()).
Given that this seems to be a common error, all of my changes (except for removing the t() call) were to comments or tests, and I have manually tested this, I think this would be okay to merge.
I've fixed @sokru's review comments: thank you very much!
For documentation / search-ability purposes, the error thrown at /admin/config/search/search-api/add-server is...
Drupal\search_api\SearchApiException: Cannot load the Elasticsearch cluster for your index. in Drupal\elasticsearch_connector\Plugin\search_api\backend\SearchApiElasticsearchBackend->__construct() (line 205 of /var/www/html/src/Plugin/search_api/backend/SearchApiElasticsearchBackend.php).
Moving this to 8.x-7.x, and I've fixed the merge conflicts.
(update the issue title to distinguish this from ✨ LIKE and EXACT filter for searching fulltext Needs review )
(update the issue title to distinguish this from its parent issue)
I've updated the issue summary as best I can.
(going to remove one of the tags that has recently become "undefined" — I don't remember what it was though)
Awesome, merged. Thanks @robert-os: I've credited you!
I will update this ticket when this issue gets released.
(quick update, I just needed to update the test in #3550775, and it worked, so I've merged it to the 8.0.x branch)
Other than rebasing the issue onto 8.0.x, my only changes since @robert-os' RTBC in #6 were changes to the tests.
For additional context, @robert-os had encountered a closely-related error in #3498378-4: Document how to add a custom stemmer when creating or updating an index → , and I'd requested he test this issue to see if it solved the error.
Since I'm a maintainer, I wrote the code, I feel this is likely a blocker for new users, and it solved @robert-os' issues (as well as solving issues in my own manual tests), I feel confident merging this.
@robert-os, I think there is value in adding a new event that triggers on addIndex() too! However, I suggest that we file a separate issue for that (i.e.: instead of repurposing this ticket). Would you be willing to create an issue to add a new event that triggers on addIndex(), and link to it in a comment here?
Regarding 🐛 Server tasks can fail if index is not closed before putSettings() and re-opened after Needs review : that error wasn't happening to me on my client site (i.e.: with an existing Elasticsearch index), but it seemed to be happening regularly enough in my test site that I felt like it should get fixed soon! And since you had come up with the same solution in #4, I wanted to make you aware of it, and hopefully, get some feedback on whether it worked for your use case as well. Your feedback gives me more confidence that merging 3550775 is more likely to fix problems than create new ones! :) I had forgotten about the failing test, though, so I'm going to look into that. I'll leave a comment there when I've fixed the test so you know that there's a new change to test. If the fix ends up simply being a change to test code, I'm likely going to merge 3550775.
Thank you very much!
As for this issue, I think that — once the issue to add a new event that triggers on addIndex() is merged — maybe all there is left to do in this issue is make sure that the documentation includes information on how to add a custom stemmer. (I've updated the issue's title and component to reflect that)
@robert-os, the issue in
🐛
Server tasks can fail if index is not closed before putSettings() and re-opened after
Needs review
introduces close() and open() calls around the putSettings() method (i.e.: your approach #2). Would you be able to try out the code in that issue and see if it works? If it does, can you mark the issue as "Reviewed & tested by the community"? Thank you very much!
Updating the issue summary to document this change for future readers of the release notes.
Agreed; I was having trouble creating the merge request for some reason: thanks @sokru!
Maintainers, would you like me to try to add a test? I could add a Functional test that saves the form, and checks that there is no token_help in the config afterwards.
I think that Testbot checks the config schema during tests, so any test that saves the form should trigger an error on the unpatched code... but writing a full test for the other elements of the form seems out-of-scope. I suppose I could make a very simple stub test, and file a follow-up ticket to fill in the rest of the test.
Glad to hear!
I'll create a merge request to make it easier for the maintainers.
elasticsearch_test module was failing to install because of a missing index on the local cluster.
@sweetchuck, thank you for the contribution!
I recently wrote some documentation on how to set up a local environment for developing elasticsearch_connector-8.x-7.x → with ddev/ddev-drupal-contrib... will this help with what you're trying to do?
(since I've asked a question, I'm moving this to "Postponed (maintainer needs more info)". When you answer, please change it back to "Active" (or some other status if you'd like).
Merged! I'll update the issue summary when I release this change.
Testbot is happy. Because this is a change that only affects development environments, I'm going to merge this.
I've also sorted require, require-dev to provide more stability ( see this change record and associated links for more on why sorting provides more stability → )
@osopolar or @dineshkumarbollu, may I trouble you to provide steps to reproduce this issue on the 8.x-7.0-alpha6? Can you also give the version of Drupal that you're running, and any other patches you're applying to Elasticsearch Connector?
I'm moving this to "Postponed (maintainer needs more info)" for now, but when you reply, please set it back to "Needs work".
(I want to merge this, but I can't merge a change without verifying that the change does something, and since I can't reproduce the error you're seeing, I can't tell if it does anything)
The current steps to reproduce say "Go to /admin/config/search/elasticsearch-connector/cluster/{name}/edit" but that doesn't seem to work for me... I don't see that error in the console when I go there.
Investigating further...
- As far as I can tell,
js/ec-index.jsis supposed to be attached to the page when a form element of'#type' => 'ec_index'is added to a page (via_elasticsearch_ec_index_attached()) - The form element of
'#type' => 'ec_index'is supposed to be defined inelasticsearch_connector_element_info()but hook_element_info() was removed before Drupal 8.0.0 → , and I can't find any form elements of'#type' => 'ec_index'anywhere in the 8.x-7.x code
Looking at it from another direction...
- The code in
js/ec-index.jsis supposed to take action on DOM elements that match the jQuery (CSS) selectora.ec-index-dialog - The only place in the code that I can find anything being added with the class
ec-index-dialogis in_elasticsearch_ec_index_process(), which is a process callback for the the definition of the form element whose'#type' => 'ec_index', which is again, defined inelasticsearch_connector_element_info()which isn't used in Drupal 8.0.0 or higher
Also, given that the error message you're seeing starts with Uncaught TypeError, that seems to tell me that this isn't just a lint that you're seeing, it must be running somewhere (but maybe I'm mistaken).
Looking at the history of this issue, it was never filed against a Drupal 7 version of the module, so I'm really confused!
If this is "dead code" (i.e.: code that still exists but isn't called by anything anymore), then I think we should just delete it — but I'd rather delete dead code in a separate issue, because changing the scope of an issue makes things convoluted.
I think this issue was already fixed in 📌 Nullable types must be explicit Needs review .
May I trouble you to test out the code in that issue by applying the patch → from that issue's merge request, and reporting back here if it works for you?
I will be releasing a new version of the module with the change from that issue soon! (I wanted to wait at least 2 weeks from the previous release to ensure that everyone had a chance to upgrade).
I believe that nowadays, Elastic Cloud...
- only runs Elasticsearch 8 or 9, and,
- uses API keys for authentication
(see the Elastic Cloud Hosted docs and Elastic Cloud Serverless docs for more details)
Regarding the first point, we haven't been testing the 8.x-7.x release series of this module with Elasticsearch 8 or 9. However, the 8.0.x release series is tested against Elasticsearch 8. If you haven't already, it might be worth upgrading the 8.0.x release series.
Regarding using API keys for authentication, there's an issue for 8.x-7.x to do that in ✨ Use ApiKey as authentication for elasticsearch Active . The issue to make that work in the 8.0.x release series was in ✨ Connect to an Elastic Cloud Hosted Deployment acting as a Search API Server by entering a Cloud ID and API key Active and has already been released.
I also wrote some documentation for the 8.0.x release series about setting up Elastic Cloud → , and connecting to Elastic Cloud with an Elastic Cloud ID or Elastic Cloud Endpoint →
Do you think that ✨ Use ApiKey as authentication for elasticsearch Active or ✨ Connect to an Elastic Cloud Hosted Deployment acting as a Search API Server by entering a Cloud ID and API key Active addresses the issue(s) you're experiencing? If so, please reply here.
If one or both of those issues does NOT address the issue here, then I'll leave this issue open so we can incorporate the things you need.
If one or both of those issues does address the issue here, I'll mark this issue as a duplicate of 3507220 and 3522107.
Thank you in advance for your patience and understanding with me as I try to find duplicates in this module's issue queue!
Tagging ✨ Connect to an Elastic Cloud Hosted Deployment acting as a Search API Server by entering a Cloud ID and API key Active (for the 8.0.x release series) as a related issue.
@rmasoad@gmail.com, may I trouble you to try adding the "Published on" field after upgrading to the latest version of this module ( 8.x-7.0-alpha6 → at time-of-writing), and let me know if it still doesn't make it to the Elasticsearch instance?
If the field still doesn't make it to the Elasticsearch instance, then I'll leave this issue open for further investigation and a fix.
If you can no longer reproduce the behavior you reported, then I'd like to close this issue as Outdated.
Thank you in advance for your patience with me as I try to address open issues in the issue queue!
@ajay547, may I trouble you to try out the latest version of this module ( 8.x-7.0-alpha6 → at time-of-writing), and let me know if you still get the error that you reported here?
If you still get the error, then I'll leave this issue open.
If you can no longer reproduce the error, then I'd like to close this issue as Outdated.
Thank you in advance for your patience with me as I try to address open issues in the issue queue!
@ajay547, @liam morland, and @sja112, may I trouble you to try out the latest version of this module ( 8.x-7.0-alpha6 → at time-of-writing), and run cron as suggested in #5, and let me know if you still get the error?
If you still get the error, then I'll leave this issue open.
If you can no longer reproduce the error, I'd like to close this issue as Outdated.
Thank you in advance for your patience with me as I try to address open issues in the issue queue!
@shiv_yadav, a quick search of this module's code at version
8.x-7.0-alpha6 →
finds 0 matches for ArrayUtils, Stdlib, and Laminas...
$ git checkout 8.x-7.0-alpha6
HEAD is now at 1e3dabd
📌
Replace README.txt with README.md and update file format
RTBC
feat: Replace README.txt with README.md and update file format
$ rg --no-ignore 'ArrayUtils' .
$ rg --no-ignore 'Stdlib' .
$ rg --no-ignore 'Laminas' .
May I trouble you to upgrade to at least 8.x-7.0-alpha6 → and see if you still get the error?
If you still get the error, then I'll leave this issue open.
If you can no longer reproduce the error, I'm going to close this issue as Outdated.
Thank you in advance for your patience with me as I try to address open issues in the issue queue!
I'm also going to unassign the issue so that anyone can contribute a fix.
@lyalyuk, may I trouble you to check if this issue is related to #3109361: Preserve index setting when re-creating it → (and/or 🐛 Indexing after deleting index leads to problematic index definition Active )?
If it is related, I'd like to close this issue as a duplicate of #3109361 so that we can focus our efforts in one place.
If it is not related, then I'll move this back to "Active".
Thank you very much in advance for your patience with me as I try make this module more maintainable by tagging duplicate issues!
@cweiske, may I trouble you to check if this issue is related to #3109361: Preserve index setting when re-creating it → (and/or 🐛 Errors during clearing index on wodby environment Postponed: needs info )?
If it is related, I'd like to close this issue as a duplicate of #3109361 so that we can focus our efforts in one place.
If it is not related, then I'll move this back to "Active".
Thank you very much in advance for your patience with me as I try make this module more maintainable by tagging duplicate issues!
@msielski, and @fra_ore_90, may I trouble you to take a look at ✨ Add Search API data source implementation Needs work to see if it addresses this issue, and then get back to me here whether it does or does not?
If it does, then I'd like to mark this as a duplicate, so that people can focus their efforts on that issue.
If it does not, then I'm happy to leave this open.
Thank you very much in advance for your patience with me as I try to make this module (and its issue queue) more maintainable!
Adding ✨ Add geo_distance proximity filter Needs review and #3193259: Bug in indexing geofield → as related issues.
It sounds like @sokru fixed the issue, and this issue has had the status "Postponed (maintainer needs more info)" for ~4 years 10 months, so I'm going to close this issue as Outdated.
Please re-open this issue if it still doesn't work even after upgrading to the latest 8.x-7.x version → ( 8.x-7.0-alpha6 → at time-of-writing)!
Thank you very much in advance for your patience with me as I try to get a handle on open issues in this module's queue!
@mennovdheuvel, @mehuls, and @kumkum29, may I trouble you to try this on the latest 8.x-7.x version → ( 8.x-7.0-alpha6 → at time-of-writing) and report if it's working now, or still seems to be a problem?
Thank you very much in advance for your patience with me as I try to get a handle on open issues in this module's queue!
Given that @jerrac filed the issue, and recommended the patch in 🐛 Partial Search Issue RTBC , and @ushma reported that the patch works for them, I'm going to close this issue as a duplicate of 2912308.
Thanks everyone!
This issue was already fixed in 📌 Nullable types must be explicit Needs review . If you could test out the code in that issue by applying the patch → from that issue's merge request, it would be a big help! I will be releasing a new version of the module with the change from that issue soon!
I have tested this with both a local Elasticsearch cluster, and with an Elastic Cloud backend, and the changes seem to work.
That being said, I would appreciate hearing from other members of the community whether testing this change works before merging locally. Note you can download a patch file (i.e.: for composer) from the merge request. →
Updating issue summary to match the scope of changes in this merge request.
I think that I've been running into this issue occasionally while working on the Elasticsearch Connector module's 8.0.x release series but I hadn't been able to track down a cause. Elasticsearch Connector is calling clear() in a function that gets called by BackendClient::updateIndex()!. @achap and @drunken monkey, thank you for tracking it down and fixing it!
@alt.dev, that's okay!
Thank you for updating the merge request, and thank you for pulling some of the changes from the 8.x-7.x merge request as well - the more similar the two issues are, the easier it will be to merge the changes to both branches!
Regarding the 2. Facets Fail on "Text" Fields problem - it sounds similar to a problem I ran into while preparing the release - are you seeing errors similar to what's described in 🐛 "Fielddata is disabled on [field] in [index]" error when trying to add a facet on a String field in an index managed by Search API Active ?
Regarding the 3. Views require the "_id_ Field problem - I think it's safe to assume that we'd always want to add the _id field, so I created
#3550052: Always add the _id field to Elasticsearch indexes →
to figure out how to do that.
The 1. ElasticSearch Indexes are Invisible problem is difficult, and I'm very much open to ideas.
For historical context in 📌 Investigate search_api_opensearch as base for elasticsearch_connector Fixed , we realized that elasticsearch_connector had become fairly large, unmaintainable, and the code was pretty unique (i.e.: unlike any other search_api modules). So @sokru and I made an effort to break away from the old elasticsearch_connector code, and follow the code in search_api_opensearch → in the hope that both projects could share code, or at least ideas. For us, this meant removing Elasticsearch Connector's unique UI to manage clusters and indexes (i.e.: a separate UI from Search API), and by extension, removing ClusterManager and ClientManager. It looks like neither search_api_opensearch-2.x nor search_api_opensearch-3.x have something like this datasource plugin (nor any issues to add one).
Maybe we can take inspiration from how Search API Solr → does this without a unique UI?
Maybe there's a way to break this problem into smaller pieces? Sometimes I find that when I do this, the path forward becomes clearer.
(As it is, this merge request is very large, has no tests, and it is relatively difficult for me as a reviewer to understand the purpose of each piece, so breaking it down into smaller pieces might be worth doing anyway!)
(I've updated the issue summary with the relevant information about Drupal versions and compatibility)
For what it's worth...
- Elasti
csearch Connector says the minimum version of Drupal core that it supports is 9.2. - Drupal 9.2 defines an entity_type.manager service, which has a getStorage() method that follows the signature used in the code
The fact that an automated test didn't pick this up means that this code path in ElasticsearchViewsFulltextSearch.php is untested, which is concerning. That being said, I think this particular bug is important enough that we should file a follow-up issue to add tests: I've added the tag "Needs followup" so we don't forget to create one.
I still need to manually test this change, so I'm going to leave this as "Needs review" until I've done so. Since there are no "Steps to reproduce", and I'm going to have to figure how to reproduce the error myself so I can test the change, I may as well update the issue summary at the same time, so I'm leaving the "Needs issue summary update" tag in place for now.
CSpell noticed a misspelled word, getindex in src/Event/PrepareIndexEvent.php line 99, which looks like public function getindexId() {
This error will go away if we change the function name to getIndexId()
However, getindexId isn't used in the patch. I assume @darshanchoudhary has a custom module or patch that uses this function? If so, then I'll let @darshanchoudhary fix this lint, so that they can update their custom module or patch at the same time.
Note that PHPCS is showing the following lints...
FILE: src/Event/PrepareSearchQueryEvent.php
------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------
125 | ERROR | Public method name "PrepareSearchQueryEvent::getSearchAPIQuery" is not in lowerCamel format
| | (Drupal.NamingConventions.ValidFunctionName.ScopeNotCamelCaps)
------------------------------------------------------------------------------------------------------------------------
FILE: src/Event/BuildSearchParamsEvent.php
------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------
99 | ERROR | Public method name "BuildSearchParamsEvent::getSearchAPIQuery" is not in lowerCamel format
| | (Drupal.NamingConventions.ValidFunctionName.ScopeNotCamelCaps)
------------------------------------------------------------------------------------------------------------------------
PHPCS wants these functions to be named getSearchApiQuery.
These functions aren't used in the patch. I assume @andyg5000 has a custom module or patch that uses these functions? If so, then I'll let @andyg5000 fix this lint, so that they can update their custom module or patch at the same time.
I've merged this change and credited both @beloglazov91 and @sprouse_moose.
(I assumed that, because @sprouse_moose needed the patch, they had reviewed the changes and were successfully using it on their site)
Thank you everyone!
I will update this issue when the change gets released.
@sprouse_moose, thank you; and sorry for the churn.
I wanted to get the most-breaking-changes out in their own release, so that the module can finally become maintainable again (this release of most-breaking-changes became 8.x-7.0-alpha6).
While I'd love to merge everything that I can right away, I also need to give the community a bit of time to adjust to the changes... besides patches in the public queue, many people maintain internal patches as well, and they need time to adjust those. This particular release was A LOT of work, because I wanted to make it as easy as possible for people to upgrade after all the breaking changes, so I fixed merge conflicts in ~57 issues on my own time (this took me more than 20 hours of work, excluding meals, breaks, much-needed sleep).
Anyway, I plan to make another release in ~2 weeks time with bugfixes and new features.
If you want to help, you could test out the change that has already been merged in 📌 Index time boost is deprecated Needs review .
If you're using the change in this issue, it is actually pretty helpful for maintainers to know that! We're aware that any change that we make to the module could break ~4000 sites using this module at time-of-writing → , which is a lot of pressure! When people report that they've reviewed the change → , and/or they're using it and it hasn't broken their site, it gives us more confidence to merge it without worrying about 4000 people yelling at us! 😅
Taking a look at the changes in this merge request...
- Elasticsearch Connector says the minimum version of Drupal core that it supports is 9.2.
- Drupal 9.2 says the minimum version of PHP that it supports is 7.3.0.
- Nullable types were introduced in PHP 7.1.0.
- There are other instances of nullable type declarations in the module already.
- I don't see any other instances of type declarations that could be nullable but are not yet marked as such after this change.
- It's not feasible to write automated tests for this change.
I've updated the issue summary with this information (this helps maintainers make a decision about whether it's safe to merge).
The code in this issue fixed a phpstan lint that had previously been ignored. I've deleted the override in phpstan-baseline.neon. And, testbot is now green across the board.
Testing myself locally, I can confirm that it works, so I'm marking this as RTBC.
I will merge this change shortly.
Thank you everyone!
Change wording slightly to distinguish from the 8.x-7.x version of this page.
Slight wording changes to distinguish from separate 8.x-7.x guide.
I went to try to rebase this onto the latest changes to 8.x-7.x, and found that there's a bunch of unrelated changes in the patch in #17. Both branches also appear to have some unrelated changes in them.
For now, I'm going to move this back to "Needs work".
I went to try to reapply this patch to the latest changes in the 8.x-7.x branch, but I couldn't find the code that this patch applied to.
When I switched back to the version that this ticket was filed against, 8.x-7.0-alpha3, I still could not find the code in this patch's context in src/Plugin/search_api/backend/SearchApiElasticsearchBackend.php
I'm not sure exactly what happened here: my best guess is that the issue was accidentally filed under the wrong version of the project.
I'm going to move this back to "Needs work".
Thanks for your understanding and patience!
I'm not sure what happened here, but there don't appear to be any changes in the merge request... I'm going to move this back to "Needs work".
mparker17 → changed the visibility of the branch 3358159-indexing-errors-with to hidden.
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I rebased the merge request onto the latest 8.x-7.x... If you need an updated patch file, you can download one from the merge request → .
Thank you!
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I rebased the changes in the merge request and resolved the merge conflict. If you need an updated patch file, you can download one from the merge request → .
Thank you!
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I created merge request !150 from the patch. If you need an updated patch file, you can download one from the merge request → .
Thank you!
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I rebased the code in the merge request onto the latest 8.x-7.x.
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I created merge request !149 from the patch in #2. If you need an updated patch file, you can download one from the merge request → .
Thank you!
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I created merge request !148 from the patch in #3. If you need an updated patch file, you can download one from the merge request → .
Thanks!
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I created merge request !147 from the patch in #7, and fixed the merge conflicts. If you need an updated patch file, you can download one from the merge request → .
Thank you!
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I created merge request !146 fromt he patch in #3, and resolved the merge conflicts. If you need an updated patch file, you can download one from the merge request → .
Thank you!
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I created merge request !145 from the patch in #4. If you need an updated patch file, you can download one from the merge request → .
Thank you!
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I turned the patch in #4 into merge request !144 and resolved the merge conflict. If you need an updated patch file, you can download one from the merge request → .
Thank you!
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I turned the patch into merge request !143. If you need an updated patch file, you can download one from the merge request → .
Thank you!
Hi @alberto56, thank you for the contribution!
Actually, this got resolved in 🐛 Coding Standards Updates Needs work — but I didn't notice until I went to rebase the patch.
I'm going to mark this issue as a duplciate of #3346008, which was released in elasticsearch_connector-8.x-7.0-alpha6 → .
mparker17 → made their first commit to this issue’s fork.
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I rebased the code in the issue fork, applied the patch from #52 to it, and created merge request !142. If you need an updated patch file, you can download one from the merge request → .
mparker17 → changed the visibility of the branch 8.x-7.x to hidden.
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I committed the patch in #8 to merge request !141. If you need an updated patch file, you can download one from the merge request → .
mparker17 → made their first commit to this issue’s fork.
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I turned the patch in #24 into merge request !140, and fixed the merge conflicts. If you need an updated patch file, you can download one from the merge request → .
mparker17 → made their first commit to this issue’s fork.
I just rebased this issue onto the latest 8.x-7.x, and resolved the merge conflicts!
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I converted the patch in #11 into merge request !139. If you need an updated patch file, you can download one from the merge request → .
mparker17 → made their first commit to this issue’s fork.
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I created merge request !138 from the patch in #17. If you need an updated patch file, you can download one from the merge request → .
Since I'm busy fixing merge requests in many issues in this project, I'm unable to review your code in as much detail as I would like. But I was able to briefly look at your code, and I think the approach is sound (i.e.: I think you're approaching the problem in the right way). But I need automated tests before I can merge (in order to prevent future changes from causing the code in this issue to regress), so I've added the "Needs tests" tag, and changed the issue status to "Needs work". If you need help writing tests, please ask (although I have a large backlog of tickets to work on, and I volunteer my time on the 8.x-7.x branch, so I cannot guarantee a speedy response - thanks in advance for understanding)!
We should also update the issue summary before release, so I've added the "Needs issue summary update" tag.
Thank you!
mparker17 → made their first commit to this issue’s fork.
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I rebased the code in the issue fork onto the latest 8.x-7.x, and fixed the merge conflicts. If you need an updated patch file, you can download one from the merge request → .
I didn't have much time to review, but briefly looking at the code, I think your approach is sound (i.e.: I think you're approaching the problem in the right way). But, I don't see any automated tests, and I cannot merge changes without corresponding tests, so I'm adding the "Needs tests" tag, and changing the status to "Needs work". Thank you in advance for your patience and understanding.
We should also update the issue summary before release, so I've added the "Needs issue summary update" tag.
mparker17 → made their first commit to this issue’s fork.
I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes that could break older patches, so I've been proactively fixing merge conflicts, hopefully saving people time when they need to upgrade.
I created merge request !136 from the patch in #3, and fixed the merge conflict with 🐛 Coding Standards Updates Needs work . I also applied the fix that eslint was suggesting, to help this ticket move along faster. If you need an updated patch file, you can download one from the merge request → .
I reviewed the changes, and I find this more readable than the call I had written in 3346008, so I'm inclined to go with it.
I don't think this is easy to test, so I'm not going to ask for tests for this issue.
We should update the issue summary before release though, so I've added the "Needs issue summary update".
I'm going to leave it in the "Needs review" status until I have had a chance to test it on my test site. If it works, then I'll move this to RTBC and merge it.
Thank you very much for your contribution, @dineshkumarbollu!