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!
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 merge request !40 onto the latest 8.x-7.x, and fixed the merge conflicts.
A brief glance at the code shows that there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag, and moved it back 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.
mparker17 → made their first commit to this issue’s fork.
I rebased the changes in the merge request onto the latest 8.x-7.x, and fixed 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 the new release comes out. This means I haven't had a chance to properly review the code.
A brief glance at the code shows that there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag, and moved it back 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. An issue summary helps a maintainer understand why a change was necessary ("Problem/Motivation"), why you chose a particular solution ("Proposed resolution"), and how to test the change manually ("Steps to reproduce"). After an issue is fixed, a good issue summary documents what changed and what could be impacted ("User interface changes", "API changes", "Data model changes"), for people upgrading.
mparker17 → made their first commit to this issue’s fork.
Thank you very much for the contribution, @dubois!
Now that Drupal.org no longer automatically tests patch files, and has updated tooling for merging and credit, issue forks (and merge requests) are SUPER helpful for me as a maintainer!
I've applied your patch in #3 to the issue fork, and created merge request !135 with your changes.
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 the new release comes out. If you need an updated patch file, you can download one from the merge request → .
Question, is there any Elasticsearch-specific code in the processor? If not, perhaps if we should move this issue to the Search API issue queue (i.e.: by changing the Project field in the issue metadata)! As a part of Search API, more people could benefit from the code!
mparker17 → made their first commit to this issue’s fork.
I've created merge request !134 from the patch in #3, and resolved the merge conflicts. If anyone needs an updated patch file, you can download one from the merge request → .
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 the new release comes out. This means I haven't had a chance to properly review the code.
However, a brief glance at the code shows that there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag, and moved it back 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. An issue summary helps a maintainer understand why a change was necessary ("Problem/Motivation"), why you chose a particular solution ("Proposed resolution"), and how to test the change manually ("Steps to reproduce"). After an issue is fixed, a good issue summary documents what changed and what could be impacted ("User interface changes", "API changes", "Data model changes"), for people upgrading.
mparker17 → made their first commit to this issue’s fork.
@edurenye, thanks for improving my patch earlier!
I've now created merge request !133 from the patch in #3. If you need an updated patch file, you can download one from the merge request → now.
I had re-done a bunch of the documentation in 🐛 Coding Standards Updates Needs work , so the patch is much smaller now, and now really only contains the typehints added in #3.
I'm not sure how good the tests are for these events, so I'm going to mark the patch with the "Needs tests" tag, and move the status back to "Needs work", because I can't merge changes without making sure that test coverage is sound. It's my fault for not doing that earlier.
Thanks for your patience!
(oops, I said I was going to change this issue to "Needs work" and then forgot to do that 🤦♂️)
Thank you very much for the contribution, @Dimitris.T!
I'm a maintainer of the module, and I just released elasticsearch_connector-8.x-7.0-alpha6 → , which had some changes in it that could make it hard to apply older patches, so I'm going through the issues with patches in the queue, and trying to resolve merge conflicts, hopefully saving people time when the new release comes out.
In the past, Drupal.org used to automatically review patch files uploaded to issues; but this recently went away, replaced with GitLab CI jobs that run on merge requests. I've created merge request !132 with the changes in your patch, and credited you! 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 have the following feedback:
- I think your approach is sound, i.e.: I think you're approaching the problem in the right way,
- The automated static analysis tool is recommending that you replace the
\Drupal::config('system.date')call in the patch with dependency injection → , and, - Your new code doesn't have any automated tests, and I cannot merge changes without corresponding tests
Because of points #2 and #3, I'm changing the Status back to "Needs work".
Because of the missing tests, I'm adding the tag "Needs tests" so that the community knows what needs to be done. Automated tests ensure that future changes to the module (i.e.: by other contributors) will not break the functionality that you (or your client) depends on! If I can help you with writing tests, please ask: I am happy to help!
I'm also tagging the issue with "Needs issue summary update". An issue summary helps a maintainer understand why a change was necessary ("Problem/Motivation"), why you chose a particular solution ("Proposed resolution"), and how to test the change manually ("Steps to reproduce"). After an issue is fixed, a good issue summary documents what changed and what could be impacted ("User interface changes", "API changes", "Data model changes"), for people upgrading. I can help with this as well!
If you have any questions, please ask, and I'll do my best to answer them!
Thank you again!
mparker17 → made their first commit to this issue’s fork.
Thank you very much again, @wotts! I've created merge request !131 from the patch in #6. If anyone needs an updated patch file, you can download one from the merge request → .
I don't have time to properly review the code right now. However, a brief glance at the code shows that there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag, and moved it back 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)!
mparker17 → made their first commit to this issue’s fork.
Thanks @wotts! You got to it before I did! I've created merge request !130 from the patch in #24. If anyone needs an updated patch file, you can download one from the merge request → .
I don't have time to properly review the code right now. However, a brief glance at the code shows that there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag, and moved it back to "Needs work".
mparker17 → made their first commit to this issue’s fork.
I've created merge request !129 from the patch in #2. If you need an updated patch file, you can download one from the merge request → .
I'm in the middle of preparing for a release, more specifically, ensuring that as many patches as possible will apply on the new release, i.e.: by resolving merge conflicts, i.e.: hopefully saving people time when the new release comes out. This means I haven't had a chance to properly review the code.
It sounds like it didn't solve the issue that @emudojo was having. Would it be possible to report back to see if the new patch works?
A brief glance at the code shows that there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag, and moved it back to "Needs work".
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've created merge request !128 from the patch in #2. If you need an updated patch file, you can download one from the merge request → .
I'm in the middle of preparing for a release, more specifically, ensuring that as many patches as possible will apply on the new release, i.e.: by resolving merge conflicts, i.e.: hopefully saving people time when the new release comes out. This means I haven't had a chance to properly review the code.
However, a brief glance at the code shows that there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag, and moved it back to "Needs work".
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've rebased the merge request onto the latest 8.x-7.x, and fixed merge conflicts. If you need an updated patch file, you can download one from the merge request → .
I'm in the middle of preparing for a release, more specifically, ensuring that as many patches as possible will apply on the new release, i.e.: by resolving merge conflicts, i.e.: hopefully saving people time when the new release comes out. This means I haven't had a chance to properly review the code.
However, a brief glance at the code shows that there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag, and moved it back to "Needs work".
mparker17 → made their first commit to this issue’s fork.
I've created merge request !127 from the patch in #4. If you need an updated patch file, you can download one from the merge request → .
I'm in the middle of preparing for a release, more specifically, ensuring that as many patches as possible will apply on the new release, i.e.: by resolving merge conflicts, i.e.: hopefully saving people time when the new release comes out. This means I haven't had a chance to properly review the code.
However, a brief glance at the code shows that there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag, and moved it back to "Needs work".
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.