🇨🇦Canada @mparker17

On the plus side, though, it seems like tests are passing in the merge request as it is now.

Re: #113 you could apply the patch from that other issue to core using composer-patches and then use it in this issue, yes.

@bradjones1 Hmm... could you explain how to do this, or point me towards some documentation?

Aside: in order to change this for !8101, I'd have to drop all the commits in the 📌 Adding GIN and GIST indexes to PostgreSQL databases RTBC branch (or re-create the branch), update composer.json to apply the patch, then re-apply the commits specific to this branch... which is fine, but I don't want to force-push until I know it's going to work, and local testing doesn't seem to work. For simplicity below, the logs will assume I'm re-creating the branch.

I've set up a dev environment using justafish/ddev-drupal-core-dev...

$ git clone --branch 11.x https://git.drupalcode.org/project/drupal.git d10-core
$ cd d10-core
$ ddev config --project-type=drupal10
$ ddev start
$ ddev corepack enable
$ ddev get justafish/ddev-drupal-core-dev
$ ddev restart
$ ddev composer install

... as I expected, it didn't seem like cweagans/composer-patches was required by anything...

$ ddev composer depends 'cweagans/composer-patches'

In BaseDependencyCommand.php line 100:

  Could not find package "cweagans/composer-patches" in your project  

... so I tried installing cweagans/composer-patches, but that failed because, I guess, core references itself in its own composer.json?

$ ddev composer require 'cweagans/composer-patches:^1'
./composer.json has been updated
Running composer update cweagans/composer-patches
> Drupal\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires drupal/core 1.0.0 (exact version match: 1.0.0 or 1.0.0.0), found drupal/core[dev-main] but it does not match the constraint.
  Problem 2
    - Root composer.json requires drupal/core-project-message 1.0.0 (exact version match: 1.0.0 or 1.0.0.0), found drupal/core-project-message[dev-main] but it does not match the constraint.
  Problem 3
    - Root composer.json requires drupal/core-vendor-hardening 1.0.0 (exact version match: 1.0.0 or 1.0.0.0), found drupal/core-vendor-hardening[dev-main] but it does not match the constraint.

Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.

Installation failed, reverting ./composer.json and ./composer.lock to their original content.
Composer [require cweagans/composer-patches:^1] failed, composer command failed: exit status 2. stderr=

So I tried simply adding the patch to composer.json...

$ ddev composer config --json extra.patches.'drupal/core' '{"#3397622, !5839 Adding GIN and GIST indexes to PostgreSQL databases": "https://git.drupalcode.org/project/drupal/-/merge_requests/5839.diff"}'
$ git diff
diff --git a/composer.json b/composer.json
index b56c8cec3e..e30ad42bdf 100644
--- a/composer.json
+++ b/composer.json
@@ -96,6 +96,11 @@
                 "               and not intended to be used for production sites.",
                 "               See: https://www.drupal.org/node/3082474"
             ]
+        },
+        "patches": {
+            "drupal/core": {
+                "#3397622, !5839 Adding GIN and GIST indexes to PostgreSQL databases": "https://git.drupalcode.org/project/drupal/-/merge_requests/5839.diff"
+            }
         }
     },
     "autoload": {

...seems fine so far, so lets apply the patch...

$ ddev composer update drupal/core
> Drupal\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires drupal/core 1.0.0 (exact version match: 1.0.0 or 1.0.0.0), found drupal/core[dev-main] but it does not match the constraint.
  Problem 2
    - Root composer.json requires drupal/core-project-message 1.0.0 (exact version match: 1.0.0 or 1.0.0.0), found drupal/core-project-message[dev-main] but it does not match the constraint.
  Problem 3
    - Root composer.json requires drupal/core-vendor-hardening 1.0.0 (exact version match: 1.0.0 or 1.0.0.0), found drupal/core-vendor-hardening[dev-main] but it does not match the constraint.

Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.
Composer [update drupal/core] failed, composer command failed: exit status 2. stderr=

... same as before.

What about just updating the lock file?

$ ddev composer update --lock            
> Drupal\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires drupal/core == 11.9999999.9999999.9999999-dev (exact version match), it is satisfiable by drupal/core[11.x-dev] from composer repo (https://repo.packagist.org) but drupal/core[dev-main] from path repo (core) has higher repository priority. The packages from the higher priority repository do not match your constraint and are therefore not installable. That repository is canonical so the lower priority repo's packages are not installable. See https://getcomposer.org/repoprio for details and assistance.
  Problem 2
    - Root composer.json requires drupal/core-project-message == 11.9999999.9999999.9999999-dev (exact version match), it is satisfiable by drupal/core-project-message[11.x-dev] from composer repo (https://repo.packagist.org) but drupal/core-project-message[dev-main] from path repo (composer/Plugin/ProjectMessage) has higher repository priority. The packages from the higher priority repository do not match your constraint and are therefore not installable. That repository is canonical so the lower priority repo's packages are not installable. See https://getcomposer.org/repoprio for details and assistance.
  Problem 3
    - Root composer.json requires drupal/core-vendor-hardening == 11.9999999.9999999.9999999-dev (exact version match), it is satisfiable by drupal/core-vendor-hardening[11.x-dev] from composer repo (https://repo.packagist.org) but drupal/core-vendor-hardening[dev-main] from path repo (composer/Plugin/VendorHardening) has higher repository priority. The packages from the higher priority repository do not match your constraint and are therefore not installable. That repository is canonical so the lower priority repo's packages are not installable. See https://getcomposer.org/repoprio for details and assistance.

Composer [update --lock] failed, composer command failed: exit status 2. stderr=

... apparently not!

What if I try running GitLab CI tests locally → ?

$ alias drupal-ci-local='gitlab-ci-local --remote-variables git@git.drupal.org:project/gitlab_templates=includes/include.drupalci.variables.yml=main --variable="_GITLAB_TEMPLATES_REPO=project/gitlab_templates"'
$ drupal-ci-local
...
 FAIL  📦️ Composer                                   
  >   Invalid version string "HEAD-dev"  
  >                                      
  > validate [--no-check-all] [--check-lock] [--no-check-lock] [--no-check-publish] [--no-check-version] [-A|--with-dependencies] [--strict] [--] [<file>]

... so I'm not sure what to do next.

> Re: #113 you could apply the patch from that other issue to core using composer-patches and then use it in this issue, yes.

I merged in the commits, but that idea sounds be a lot easier to review, so I'm going to try that shortly.

@bradjones1 would it be helpful to update this patch to use the API in 📌 Adding GIN and GIST indexes to PostgreSQL databases RTBC , given that it is RTBC?

Anything else I could help with?

I've attempted to re-roll the patch from #93 onto 11.x at commit f560c83a1c (from 2024-05-15).

Added an update hook. Feedback welcome!

Tagging with "Performance" because it affects performance. Tagging with "PostgreSQL" because it particularly affects sites running on the PostgreSQL database. See the Issue tags -- special tags documentation → for more information.

I should add an update hook for existing sites...

I don't think it's possible to write tests for this, but if you have ideas, I would very much appreciate some mentorship on how to write them!

Going to leave this as "Needs review". Feedback is welcome!

mparker17 → created an issue.

Hmm... let me think about it.

In the meantime, I've pushed some code linting fixes, but tests are still failing, because I think composer.json is taking precedence over sitemap.info.yml.

Once I get it working, would you be willing to write a test for the new functionality?

@matthieuscarset, thank you for the contribution!

I think that the test (and some, but not all of the lints) is failing because the domain_menus module is not present in the test environment, so it cannot resolve the \Drupal\domain_menus\DomainMenusConstants import in the DomainMenuSitemapDeriver.

There are two ways that we could proceed:

1. We could modify sitemap.info.yml to include domain_menus as one of the test_dependencies as part of this patch.
   If we go this route, we will also need to write automated tests.
2. We could create a new patch to the domain_menus module, and implement the Sitemap plugin there.
   This may sound like a lot of work, but I believe it would mostly consist of opening a new issue, creating a new issue fork, copying the plugin you wrote here and modifying the namespace lines, and updating domain_menus.info.yml
   This is possible because the Sitemap module uses Drupal's Plugin API → , which allows plugins for one module (e.g.: Sitemap plugins) to exist inside another module (e.g.: domain menus). The Plugin code is only run if both modules are installed. I can't think of a module that defines a Sitemap plugin at the moment, but, for example, Drupal core's Field module allows plugins of type Field to be created, and the contrib Address module → defines its own Field plugin to for a [post] address field.

As a Sitemap module maintainer, I would prefer the second option (i.e.: patch domain_menus), because the Sitemap module maintainers' current policy is only to maintain Sitemap plugins for Drupal core (e.g.: front page (system module), book, menu, taxonomy, etc.), and let other module maintainers use the Plugin API to add Sitemap plugins in their own module. (this policy exists because the Sitemap module maintainers are all volunteers, and we don't have very much time to maintain this module if other modules change).

That being said...

1. If you would like some help writing a patch to the domain_menus module, I would be happy to be a mentor!
2. If the domain_menus module maintainer — @drpldrp — is unwilling to accept a Sitemap plugin, and you are willing to become a Sitemap module maintainer, then I would be open to revisiting our policy to only maintain Sitemap plugins for Drupal core.

@matthieuscarset, what do you think?

@artemboiko, thank you! (and apologies: I only have expertise in the 8.0.x branch of the module, so I'm unable to help move the 8.x-7.x branch forward!)

I've created a merge request: lets see if tests pass.

Offhand, the code looks fine thus far. May I request that you update the test at tests/src/Unit/SearchAPI/Query/FilterBuilderTest.php to test the new code? (this will ensure that future changes don't accidentally break the LIKE and EXACT functionality you need!)

Thank you very much!

mparker17 → made their first commit to this issue’s fork.

Here's some code that seems to do what my use-case requires, in Drupal 10, running on PostgreSQL, based on my understanding of how the node translation system works (which might be wrong)...

  /**
   * Load the latest revision for a translation.
   *
   * @param int|string $nodeId
   *   The node ID to load.
   * @param string $langcode
   *   The language code to load.
   *
   * @return \Drupal\node\NodeInterface|null
   *   The node revision for the given node in the given language.
   */
  public function loadLatestRevisionTranslation(int|string $nodeId, string $langcode): ?NodeInterface {
    $answer = NULL;

    // Select rows from the node_field_revision table, which contains all
    // revisions from all languages (node_revision only contains revisions for
    // the language that the node was created in).
    $query = $this->database->select('node_field_revision', 'nfr');

    // Limit the selection to rows that match the given node and language code.
    $query->condition('nfr.nid', $nodeId);
    $query->condition('nfr.langcode', $langcode);

    // When you save a revision for a node, revision_translation_affected is set
    // to 1 for the language that you were editing when you clicked "Save". If
    // the node happens to have other translations, all translations get a new
    // revision with the same vid. However, in this case,
    // revision_translation_affected is set to 1 for the language you were
    // editing when you clicked "Save" as before; but the revisions for the
    // other languages get set to NULL.
    // We only care about the revision that the user clicked "Save" on, so we
    // limit the selection to rows where revision_translation_affected is set to
    // 1.
    $query->condition('nfr.revision_translation_affected', 1);

    // Tell the query engine to group rows where the nid and langcode match,
    // i.e.: group rows by what we think of as a node-translation. Inside that
    // grouping of rows (a.k.a. window, sliding window, subgroup, etc.), find
    // the maximum revision ID (vid). Because vids are assigned sequentially,
    // this effectively finds the most-recent revision for that translation.
    //
    // Note that if this is the ONLY field that we are selecting, we could just
    // SELECT MAX(vid) here, without the sliding window... however, if we
    // decided to SELECT any other fields (i.e.: other use-cases, future
    // changes, debugging purposes, etc.), we would get different results
    // (unless we are very careful about how we prepare the GROUP BY and ORDER
    // BY clauses). Using a window function allows us to SELECT other fields
    // without affecting the result in this column.
    $query->addExpression('MAX(nfr.vid) OVER (PARTITION BY nfr.nid, nfr.langcode)');

    // Limits/ranges are applied at the end: and in this case, we would have one
    // row for each revision created for the given translation, with the same
    // MAX(vid) value in each row, so we only need to return the first row.
    $query->range(0, 1);

    // Now execute the query, get the first value in the first column, try to
    // load the node with that revision, and if we are successful, assign the
    // variable that we return.
    try {
      $statement = $query->execute();
      $result = $statement->fetchField(0);
      if (\is_numeric($result)) {
        $candidate = $this->getNodeStorage()->loadRevision($result);
        if ($candidate instanceof NodeInterface) {
          $answer = $candidate->getTranslation($langcode);
        }
      }
    }
    catch (\Throwable $t) {
      // No-op: if an exception occurs, return NULL.
    }

    return $answer;
  }

Code looks good to me, but I don't officially maintain this branch, so I'm going to mark it as RTBC.

The nit was fixed, back to RTBC

@undertext, thank you for the patch. Sorry I didn't respond earlier, I had my notifications set up incorrectly for this project.

@michael.roosz, have you tested @undertext's patch to see if it fixes the problem for you?

Sounds good. Let's do that after a 4.0.0 release!

You were credited in the commit message; but I've credited you in the issue node as well! Thanks very much!

Great, thanks! Merging!

@bobi-mel, looks great! There are a few PHPCS notices; I can fix those before merging. Thank you very much!

(I'm editing the issue so d.o rebuilds caches for this page and updates the issue link backgrounds to indicate the status)

I think @boromino is more knowledgeable than myself about whether this is the correct solution (and whether the JS got built correctly).

The code looks like Drupal Core's MissingAlternativeTextView custom CKEditor plugin, and I know from experience that Drupal.t() is the correct way to use Drupal's translation system in Drupal-JavaScript → , so it seems good to me.

Here's a proposal for a new project page... feedback welcome!

Elasticsearch is a powerful, distributed, RESTful search and analytics engine based on Apache Lucene that supports full-text search, vector search, retrieval augmented generation (RAG), facets → , spellchecking → , hit highlighting, auto-completion → , location-based searching → , and more.

This modules provides a Search API → backend for Elasticsearch, using the official Elasticsearch PHP Client.

Note that, in January 2021, Amazon forked Elasticsearch (then at version 7.10.2)
to create OpenSearch, and the two projects have diverged over time. If you are using OpenSearch, please consider using the Search API OpenSearch module → instead.

Requirements

Elasticsearch Connector requires the Search API → module to function.

Recommended modules

The following modules are optional, but can extend the functionality of Elasticsearch Connector:

• The Facets module → , which allows site builders to easily create and manage faceted search interfaces.
• The Geofield module → , which allows storing, managing and representing dynamic geographic data in Drupal.
• The Search API Autocomplete module → , which provides autocomplete functionality for searches.
• The Search API Location module → , which allows location-based searching.
• The Search API Spellcheck module → , which suggests corrections to misspelled words in the search query ("Did you mean ...?").

Roadmap

The ElasticSearch Connector maintainers intend to support ElasticSearch only, i.e.: we do not intend to support OpenSearch, because the Search API OpenSearch module → does that already.

The maintainers intend to support versions of this module that are compatible with the currently-supported versions of Elasticsearch. For more information, please see Elasticsearch's documentation on Elastic Product End of Life Dates.

Known problems

If you find a problem, please let us know by adding an issue → !

In the 8.0.* release series, changing the mapping of an existing field or deleting a field will cause the search index to be cleared, and all items queued for re-indexing. This is a limitation of Elasticsearch: see the Elasticsearch documentation on their Update mapping API for more details. We plan to mitigate this by using Elasticsearch's Aliases API to automatically creating a new index and reindexing to it in ✨ Support Aliases API and zero downtime mapping updates Active .

Credits

NodeSpark → , Google Summer of Code (GSoC) → 2014, FFW → , and Utilis.io sponsored initial development of Elasticsearch Connector.

Fame Helsinki → , Ontario Digital Service → , and Consensus Enterprises → sponsored maintenance and support for Elasticsearch 8.

This module was created by Nikolay Ignatov (skek) → who maintains the list of maintainers. If you would like to become a maintainer yourself, please reach out to him directly.

Similar projects and how they are different

• Search API OpenSearch → provides a Search API backend for OpenSearch. OpenSearch was created as a fork from Elasticsearch version 7.10.2, but the two projects have diverged, and there are breaking changes in the API for indexing and querying.
• Azure search → provides a Search API backend for Microsoft Azure AI Search. Microsoft Azure AI Search has a different set of features, and a different API for indexing and querying than Elasticsearch.
• Search API Solr → provides a Search API backend for Apache Solr, another Lucene-based search engine. Solr has a different set of features, and a different API for indexing and querying than Elasticsearch.

Note that there are several modules that extend Elasticsearch Connector → :

• Elasticsearch - Search API → provides a framework to set up custom Elasticsearch based search pages.
• Search API Elasticsearch Synonym → provides a way to enter synonyms for Elasticsearch in Solr format.
• Elastic Enterprise Search → provides a way to connect to Elastic Enterprise Search, an additional Elastic service that adds APIs and UIs to those already provided by Elasticsearch. A previous (now deprecated) version of Elastic Enterprise Search was named Elastic AppSearch → .

Dependencies

In order to function, this module requires a connection to an Elasticsearch cluster:

• Elasticsearch B.V., the company that created Elasticsearch and sponsors its development, offers Elasticsearch as a Service through a product called Elastic Cloud (which can be hosted on Amazon Web Services (AWS), Google Cloud Platform (GCP), and/or Microsoft Azure, and has a 14-day free trial).
• Some hosting providers (e.g.: Platform.sh, Lagoon) offer Elasticsearch plugins.
• You can also self-host Elasticsearch with Docker or Kubernetes.
• To test with a single-node cluster in CI, see this project's .gitlab-ci.yml on the 8.0.x branch.
• For local development, both ddev and lando provide officially-supported plugins; or you can run it locally.

That makes sense to me! Thanks @drunken monkey!

@budalokko, thank you!

I'll change the ticket to Version 8.x-7.x-dev, and Status back to Active.

Reviews welcome!

Now that I've created the follow-up issues (commit 2af064d, ✨ FilterBuilder: change how we determine if a condition group is empty Fixed , ✨ Add isEmpty() function to ConditionGroupInterface Active , ✨ Declare that TextTokenInterface and TextValueInterface implement \Stringable Needs review ) I'm going to close this issue as "Won't fix"

Thanks again, @drunken monkey!

mparker17 → created an issue.

Awesome, thanks for the quick review!

FWIW, this change was also made in elasticsearch_connector in commit 2af064d.

Here's a merge request!

(aside: "hi!" from the elasticsearch_connector maintainers! Our 8.0.x branch took a lot of inspiration from this project (see 📌 Investigate search_api_opensearch as base for elasticsearch_connector Fixed ), and we noticed this issue in our code when we moved to PHPStan level 2 — I filed ✨ Declare that ConditionGroupInterface or ConditionGroup implements \Stringable Needs review upstream; but @drunken monkey pointed out empty($condition_group->getConditions()) is a better approach than empty($condition_group->__toString()))

mparker17 → created an issue.

Tests pass, so I'm going to mark this as "Needs review".

Note that \Stringable was added in PHP 8.0, so referencing it will be an implicit dependency on PHP 8.0 or higher.

However, that should be fine, as the .info.yml files in the 8.x-1.x branch dropped support for Drupal 9 in 📌 Drop support for Drupal 9 Fixed (committed 2023-11-11), and Drupal core dropped support for PHP 7 starting with 9.4 → (i.e.: Drupal ^9.4, ^10, and ^11 all require at least PHP 8.0).

mparker17 → created an issue.

@drunken monkey, I apologize for the delay in replying: thank you for your patience!

Taking a fresh look at it, I think your reasoning for not wanting to add \Stringable to ConditionGroupInterface or ConditionGroup makes a lot of sense, and I agree with you.

I will adjust my code as you suggested...

-if (!empty($condition_group->__toString())) {
+if (!empty($condition_group->getConditions())) {

I agree that a isEmpty() function would be ideal: it makes the code much easier to understand at-a-glance...

if (!empty($condition_group->__toString())) {
if (!empty($condition_group->getConditions())) {
if (!$condition_group->isEmpty()) {

... I think this should be done in a separate ticket though.

I support adding \Stringable to TextTokenInterface and TextValueInterface, and to add the string return typehints in a separate ticket.

Thank you very much!

@sokru, Thanks for your patience with me!

I have added support for an index suffix to the module in the latest changes, and updated the migration and migration tests accordingly!

(I'm editing the issue so d.o rebuilds caches for this page and updates the issue link backgrounds to indicate the status)

After turning off the computer yesterday, I thought of an easier way to write a test to edit an abbreviation, and happily, it works.

All the tests and lints are passing, so I'm going to merge it!

Was trying to add a test for editing an abbreviation, but that turned out to be more difficult than it appeared, because the API for selecting text in JavaScript acts differently depending on whether you happen to be looking at a Document Node containing only text, or a Document Node containing text mixed with HTML elements. CKEditor appears to have its own way to select text, but there isn't any good documentation on how to use it.

Anyway, I think tests for editing an abbreviation can come later: they shouldn't block merging this.

Awesome, new tests pass with no linter notices, so this is ready for review again.

(Marking off the task to look through the open issues)

I've moved the D11-compatibility issue to be a non-blocker: see #3429173-9: Automated Drupal 11 compatibility fixes for ckeditor_abbreviation → for more information about why.

Hmm...

1. If I leave the dependency on drupal/ckeditor, then the D11 tests won't run; but that's important for the future.
2. If I drop the dependency on drupal/ckeditor, then the D10 and D11 phpstan will fail because they can't find the
   

            </li>
     <li>\Drupal\ckeditor_abbreviation\Plugin\CKEditorPlugin\AbbreviationCKEditorButton

   class

3. If ignore the PHPStan failures with a phpstan-baseline.neon, then phpstan on D9 will fail because it's expecting to get errors and won't get them.

I daresay the best solution here would be to split off a ckeditor_abbreviation-5.0.x branch to add D11 support and drop D9 support. (Aside: I'm fine with supporting two stable branches, as long as we have automated tests (i.e.: ✨ Add automated regression tests Needs work ).

So I'm going to mark this issue as postponed for now. Once the automated tests are merged, then we can re-evaluate.

mparker17 → changed the visibility of the branch project-update-bot-only to hidden.

Created my own merge request to upgrade the 4.0.x branch: previously this filed against the 3.0.0 branch and patched that way too.

I have run manual tests on a D11 site with the changes in this merge request, and it works well.

I should add a test to check that the title attribute is left out if it is empty, i.e.: functionality added in commit 27c25c99.

mparker17 → created an issue.

@jannakha, thank you for filing this issue. I've moved it from the "Task" category to the "Feature request" category → , because it is asking for "new functionality to be added to the project". I have also changed the Priority → from "Normal" to "Major" because changing the UI is both very valuable to the project, and also a large scope of work to complete.

I didn't see your proposed resolution ("Make adding/editing

I am marking this as "Postponed (maintainer needs more info)" because we need some more information from you before we can proceed...

1. Apologies (you probably know a lot more about the inner workings of CKEditor plugins than I do), but when you say "abbr UI can be improved as it's currently not a CKEditor5 object", what do you mean by "not a CKEditor5 object"?
   My first thought was that you're referring to how CKEditor core upcasts anchor tags to CKEditor "model" objects; but the code in our js/ckeditor5_plugins/abbreviation/src/abbreviationediting.js does the same thing... so perhaps you mean something else? Can you explain what you mean specifically?
2. Could you clarify whether you're requesting that we change the "Add Abbreviation" UI if the toolbar button is clicked without selecting text? (currently, both fields in the balloon are editable in this case) If so, what should it look like and how should it work?
3. Could you clarify whether you're requesting that we change the "Add Abbreviation" UI if the toolbar button is clicked after selecting text? (currently, only the second "Add title" field in the balloon is editable in this case) If so, what should it look like and how should it work?
4. Could you clarify whether you're requesting that we change the "Edit Abbreviation" UI at all? If so, what should it look like and how should it work?
5. Could you clarify which buttons you are requesting to appear in the balloon if you click an abbreviation? (CKEditor core's anchor tag UI shows an "Fragment" button which doesn't make sense in the context of an abbreviation, an "Edit" button that launches an Edit balloon, and a "Delete" link that removes the link/abbreviation)

Thank you!

Version 2.0.1 is no longer supported nor recommended; and version 4.0.0-alpha2 does include CKEditor 5 support.

I would recommend upgrading from version 2.0.1 to 4.0.0-alpha2 or higher.

The tests in this merge request work on Drupal 9 and 10, testing CKEditor5.

More specifically, it covers the CKEditor Abbreviation JS library, and Drupal's integration with it through the text format, toolbar, and custom integration CSS/JS.

I consider this ready for review! Feedback is welcome.

Given that Drupal 9 has reached end of life on November 1st, 2023 → , I haven't bothered to write tests for Drupal 9 testing CKEditor4, i.e.: the file at web/modules/contrib/ckeditor_abbreviation/src/Plugin/CKEditorPlugin/AbbreviationCKEditorButton.php remains untested. Note that we can't drop support for D9, delete the CKEditor4 plugin without breaking backwards compatibility, so deleting this will have to wait for major version 5.

Also, regarding the ckeditor_abbreviation.abbreviation_dialog route, and the form it points to (\Drupal\ckeditor_abbreviation\Form\CkeditorAbbreviationDialog)... I can't figure out where these are used, so I'm not sure how to test them, and they remain untested. I suspect this is "dead code", i.e.: no longer used... but if that's wrong, please let me know how I could test this code!

Merged!

Awesome, thanks @firewaller!

I've created a merge request and fixed some lints. If everything works, I will merge tomorrow.

This looks great, but I don't want to merge new functionality without tests.

I found that there are docs for a README template → , and a Project Page template → .

And, there's now a button on Project Edit pgaes to Copy README.md or README.txt file to the description

I don't see any issues reported by PHPCS in the pipeline run yesterday: https://git.drupalcode.org/project/structure_sync/-/jobs/1160448 - I suspect you're checking against an older version of the coding standards.

I don't see any issues reported by PHPCS in the pipeline run yesterday: https://git.drupalcode.org/project/views_mark_current/-/jobs/1161057 - I suspect you're checking against an older version of the coding standards.

Still working on this; but I've made some good progress.

I apologize, but I don't understand what the problem is.

I am not sure if this was intentional, but this issue was filed as a problem with the Feedback module → (a plugin that administrators can install on a Drupal site to gather feedback from users of their site; i.e.: not intended for feedback on Drupal core, other Drupal modules, themes, or distributions; nor Drupal.org itself — although I can move this issue to one of those projects if it turns out it was filed against the Feedback module in error).

Thank you for your patience with me.

Moved 🐛 Field not focusable when required on source language change. Postponed: needs info to "needs more information"

I've created a merge request, and happily, it seems like everything is working.

Can I trouble someone experiencing this issue to see if they still experience this problem on multiselect-2.0.0-beta4 → , and if so, does the code in the merge request fix the problem?

It would be super-helpful if we could add a regression test, so we can detect if this regresses again in the future.

Thank you in advance for your patience with me! I'm reviewing issues in the queue as part of 🌱 [Plan] Stable 2.0.0 release Active .

@jksloan2947, may I trouble you to see if you still experience this problem on multiselect-2.0.0-beta4 → ?

Thank you in advance for your patience with me! I'm reviewing issues in the queue as part of 🌱 [Plan] Stable 2.0.0 release Active .

I tried applying the patch from #3 to the latest code in the 2.x branch, but it didn't apply, so I've tried rewriting it.

Can I trouble someone experiencing this issue to see if they still experience this problem on multiselect-2.0.0-beta4 → , and if so, does the code in the merge request fix the problem?

Thank you in advance for your patience with me! I'm reviewing issues in the queue as part of 🌱 [Plan] Stable 2.0.0 release Active .

mparker17 → made their first commit to this issue’s fork.

mparker17 → made their first commit to this issue’s fork.

mparker17 → created an issue.

I was able to test this as follows:

1. Install Drupal with the Standard install profile, and enable the Multiselect module
2. Add a field of type List (text) to the Article content type, give it 3 allowed values ("Lorem", "Ipsum", "Dolor"). Set its cardinality to "Unlimited".
3. Manage form display for the Article content type; set the widget for the new List (text) field to "Multiselect"
4. Add an article; fill in Title with a random value, select values "Lorem" and "Dolor" in the new List (text) field.
5. Save the article.
6. Edit the article, and ensure that the values "Lorem" and "Dolor" in the new List (text) field are still selected.

When the jQuery.fn.selectAll functionality was broken, there were no values selected in step 6.

... hopefully this helps whomever ends up writing the test.

mparker17 → created an issue.

The follow-up has been merged; thanks @rgpublic and @bobi-mel!

Merged.

Hmm... keep getting merge errors. Moving back to RTBC, might have to rebase.

Merged!

All the tests pass, including on D11.

mparker17 → created an issue.

This looks good to me; merging.

This looks good to me; merging.

We should probably add CI/CD config to this module so we can better test patches.

This looks good to me; merging.

We should probably drop support for D8, as that's long been unsupported and we're not testing against it.

Hmm... I'm seeing test failures, so I think we need to update the tests to account for the new functionality.

Apologies; I didn't get a notification for this issue, so I didn't realize this ticket existed or that there was a patch ready.

I've created an issue fork, so that I can see if there are any test failures related to the changes in this ticket. I don't see any tests in the patch - but I'm not sure how one could write tests for this particular change - so I want to do some manual testing before I merge this.

mparker17 → made their first commit to this issue’s fork.

This issue is now outdated: D10 support has been there since 2.0.0-apha2, and I just merged D11 support in 📌 Automated Drupal 11 compatibility fixes for views_delimited_list Fixed .

Looks good to me. Merged!

This looks fine to me. Moving to RTBC.

Merged!

Out of curiosity, I tried deleting the deprecated config schema definitions, but tests failed: https://git.drupalcode.org/project/elasticsearch_connector/-/jobs/1138070 - so I've put back the deprecated config schema definitions.

Sounds great! Thank you, @sokru!

I have started writing tests, but they're still in the early stages of implementation. I will push them to this issue soon, then start on implementing phase 1.

Thinking about how to implement Phase 2 (i.e.: rebuild the index)...

In #3285438-9: The whole index gets cleared/deleted when any change in the search index configuration is imported/synced → , @longwave suggests using a blue/green deployment method. That is to say, for each Search API Index defined in Drupal's configuration (e.g.: machine name foo), we would need to work with (at least) 2 ElasticSearch Indexes (e.g.: foo_blue and foo_green). We'd initially pick one of them to be "active" (e.g.: foo_green), create it, and work with it normally. Later, if there was a configuration change that required us to re-index, then we would...

1. create the other index (e.g.: foo_blue) with the changed configuration
2. reindex the old index (foo_green) to the new one (foo_blue)
3. set the new index (foo_blue) as the "active" index
4. delete the old index (foo_green)

To signify which is the "active" index, we should create at least 1 Index Alias, that points to the currently-"active" index.

Aside: ElasticSearch allows you to create readable Index Aliases that point to 1..* Indexes; but only allows you to create writeable Index Aliases that point to 1 Index. The simplest approach for elasticsearch_connector might be to create 1 Index Alias (e.g.: foo, named after the Search API Index) that points to the active Index; but if that doesn't work, we might have to create 2 (foo_read, and foo_write).

While I think a blue/green method is a reasonable approach, I can see it being a source of confusion: admins/DevOps might (reasonably) ask questions like...

1. "When I created 1 Search API Index, I expected 1 ElasticSearch Index, so why did I get 2-3 things: 1-2 ElasticSearch Index Aliases, and an ElasticSearch Index; and why don?"
2. "Why the sudden spike in CPU / disk usage when I change a text field to an integer in the index settings?"
3. "Why were there 2 indexes for a short time?" (especially if the reindex operation resulted in a spike in the disk/memory/CPU monitoring logs),
4. "Why did the old index that I was referring to by name get deleted?"

... so if we take this approach, then we need to be pretty clear about what to expect, and how to interact with it (e.g.: if you need to read/write to ElasticSeach directly, read/write to the Index Alias, not the Index directly) both in the Search API Index UI, in the module's README, and/or in other documentation.

I'd be interested in hearing feedback from other maintainers about whether they think this is the right approach, and whether we should create 1 or 2 aliases.

According to the ElasticSearch 8.12 Guide, in the REST APIs section, under Index API -> Update mapping API, it sounds like:

1. You can add new fields to an index anytime
2. You can add new properties to an existing field anytime
3. You can add multi-fields (i.e.: index the same field in different ways) anytime
4. You can change mapping parameters for an existing field anytime
5. You have to reindex to change the mapping of an existing field
6. To rename a field, the suggested way to do so is to create an alias field, which you can do anytime
7. (the docs don't describe how to delete/remove a field, but checking with our client's search team lead, that also requires reindexing)

To quote from that page in the section about chang[ing] the mapping of an existing field, "If you need to change the mapping of a field in other indices, create a new index with the correct mapping and reindex your data into that index." (similar to what's described for OpenSearch in #3285438-15: The whole index gets cleared/deleted when any change in the search index configuration is imported/synced → ).

So it looks like we're in a similar situation to the Search API OpenSearch maintainers.

This looks good to me as well. Excellent work, @hexaki: thank you very much!

I think this is ready for review.

@drunken monkey, thanks for the feedback and input!

I'm still new to maintaining modules in the Search API ecosystem, so I'm not sure I'm qualified enough to have an opinion on whether or not FieldInterface, ItemInterface, TextTokenInterface, TextValueInterface, ConditionInterface, QueryInterface, and/or ResultSetInterface should implement \Stringable. In elasticsearch_connector, we only explicitly try to convert ConditionGroups to strings.

It seems logical that all of these things should implement \Stringable — and from what I know of both ElasticSearch and Solr, all these things get converted to strings in some way, because both engines use JSON as an data interchange format — but I can also imagine a search backend that uses a binary data interchange format, where that would make less sense (in fact, I worked with one briefly in my pre-Drupal career) — but I dunno how useful that would be as a Drupal search backend, and I suppose if you really had to conform to the \Stringable interface in that case, then you could base64-encode the binary data, or return an empty string, etc.

I do like adding the : string return typehint to function __toString().

Made all the requested changes, and fixed the PHPStan errors, so moving back to "RTBC"

This is an interesting idea!

That being said, I can't see it being useful for everyone using the sitemap module, so I think it would make the most sense as either a sub-module (i.e.: in Sitemap's codebase), or as a separate contrib module in Sitemap's ecosystem.

Putting on my maintainer hat for a moment, I would be willing to accept it as a submodule, but I would want a good suite of automated tests in that case.

Whether it becomes part of the Sitemap codebase or not, I would welcome some input on how Sitemap could improve its public API (and - if possible - how we can test the Sitemap module's public API, i.e.: so that we can detect BC-breaks).

LGTM

@sokru, thanks for the review!

@sukhi.chuhan, the UX design lead that I consulted with for the setting's text, advocated for using "temporary" to indicate that the setting should only be used for the short term, but she also okay with removing it if the word temporary feels duplicative for the audience (i.e.: admins changing ElasticSearch server settings).

I will push a commit shortly to remove it

Adding 🌱 Plan for 8.0.0-alpha release Active as a parent issue.

Adding 🌱 Plan for 8.0.0-alpha release Active as a parent issue.

Also going to move this to RTBC for another maintainer to take a look at.