@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.
I've created merge request !126 from the patch in #6, and resolved the merge conflicts.
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. For a start, lets see what Testbot thinks of it.
Thank you for updating the tests with the code! This really helps maintainers like myself!
We should 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.
Hi @sokru! I created merge request !125 from the patch in #2.
Marking as "Needs tests", and moving to Needs work, since the patch didn't contain any tests. Let me know if I can help with tests!
mparker17 → made their first commit to this issue’s fork.
I've created merge request !124 from the patch in #5. 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 created merge request !123 from the patch in #2, and fixed the 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".
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 !122 from the patch in #4, and fixed the 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".
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.
(unassining @gcalex5 so other people can work on it if they want)
I've created merge request !121 from the patch in #1, and fixed the 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 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 created merge request !120 from the patch in #3, and fixed the merge conflicts. Then, I hid the patch files, because Testbot no longer tests patch files, and the patch file no longer applies cleanly. If you need an updated patch file, you can download one from the merge request → . For what its worth, the team at Lullabot recommends using local copies of patch files: from personal experience, local patch files often makes composer run faster and more reliably.
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 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". Automated tests ultimately benefit you. They ensure that future changes (i.e.: by other contributors) will not break the functionality that you (or your client) depends on. 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.
Awesome, this is merged now. Note that this change is NOT in the release that went out today — I'd like to give the community a few weeks to update their sites before I push another release with new features like this one. Thank you for understanding! I will update this issue when this change is packaged into a release.
Thanks everyone!
I've created merge request !119 with the changes in patch #6, then I hid the patch files, because Testbot no longer tests patch files, and the patch file no longer applies cleanly. If you need an updated patch file, you can download one from the merge request → . For what its worth, the team at Lullabot recommends using local copies of patch files: from personal experience, local patch files often makes composer run faster and more reliably.
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.
Sounds like @hktang had some issues with the patch in #7, so I'm not sure I can do a code review yet until that is resolved, so I'm moving this to "Needs work".
A very brief glance at the patch suggests there are no tests, and I cannot merge without tests, so I've added the "Needs tests" tag. Automated tests ultimately benefit you. They ensure that future changes (i.e.: by other contributors) will not break the functionality that you (or your client) depends on. 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.
I've created merge request !118 from the patch in #11, then I hid the patch files, because Testbot no longer tests patch files. If you need an updated patch file, you can download one from the merge request → . For what its worth, the team at Lullabot recommends using local copies of patch files: from personal experience, local patch files often makes composer run faster and more reliably.
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'm not able to do a code review.
A glance at the patch file shows no tests, but it will need tests before I can merge it, so I've added the "Needs tests" tag, and moved this back to "Needs work". Automated tests ultimately benefit you. They ensure that future changes (i.e.: by other contributors) will not break the functionality that you (or your client) depends on. 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.
@tannguyenhn thank you for your contribution!
I've created merge request !117 with the changes in patch #2, then unchecked "Display", because Testbot no longer tests patches, and it no longer applies cleanly. If you need an updated patch file, you can download one from the merge request → . For what its worth, the team at Lullabot recommends using local copies of patch files: from personal experience, local patch files often makes composer run faster and more reliably.
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'm only able to do a very quick code review (and use a few copy-pasted responses)! Off the top of my head, I think the approach in this merge request is good!
That being said, the new functionality will need tests, so I've added the "Needs tests" tag, and moved this back to "Needs work". Automated tests ultimately benefit you. They ensure that future changes (i.e.: by other contributors) will not break the functionality that you (or your client) depends on. 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.
Going to add this to the 8.0.x review queue and note that it can be backported to 8.x-7.x.
No code has been submitted here, so I'm moving this back to "Active". Thanks for your patience with me as I try to clean up the backlog.
This was done in ✨ Make 8.x-7.x Drupal 11 compatible Active , which will be released shortly.
I've created merge request !116 with the changes in the patch in #6.
All the tests pass (indicating no regressions to current functionality), and no lints: awesome!
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'm only able to do a very quick code review (and use a few copy-pasted responses)! Off the top of my head, I think the approach in this merge request is good!
That being said, the new functionality will need tests, so I've added the "Needs tests" tag, and moved this back to "Needs work". Automated tests ultimately benefit you. They ensure that future changes (i.e.: by other contributors) will not break the functionality that you (or your client) depends on. 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.
I've unchecked "Display" for the old patch files, because Testbot no longer tests them, and they no longer apply. If you need an updated patch file, you can download one from the merge request → . For what its worth, the team at Lullabot recommends using local copies of patch files: from personal experience, local patch files often makes composer run faster and more reliably.
I've rebased the changes in the merge request so they apply to the latest 8.x-7.x. I've also restored the UI related to index-time boost in elasticsearch_connector.module as requested in #5, repatched in #7, and (implicitly) upvoted in #8 and #9.
All the tests pass (indicating no regressions to current functionality), and no lints: awesome! Thank you very much for updating the tests to account for the changes in this merge request!
I think this is ready to merge, but I'm going to wait until after I've made a 8.x-7.0-alpha6 release, i.e.: so that 8.x-7.0-alpha6 can contain only maintenance-related changes. Thus, I'm going to assign this issue to myself so I don't forget.
Thank you in advance for your patience!
I've unchecked "Display" for the old patch files, because Testbot no longer tests them, and they no longer apply. If you need an updated patch file, you can download one from the merge request → . For what its worth, the team at Lullabot recommends using local copies of patch files: from personal experience, local patch files often makes composer run faster and more reliably.