🇦🇺Australia @kim.pepper

All of these are available as optional settings. Are you saying we should default to safer settings, or enforce them?

I went through and resolved a load of old feedback threads that had been addressed, and did a final pass.

Looked at:

• Feedback addressed ✔
• BC coverage ✔
• Test coverage ✔
• Upgrade path coverage ✔

IMO this is ready.

I think we should postpone this on 🐛 FileSystem::tempnam() doesn't respect subdirectories for stream wrappers Needs work as that is re-implementing the method without getDirectoryPath() and adding test coverage.

Confirmed the test-only job correctly failed with:

Drupal\KernelTests\Core\File\StreamWrapperTest::testTempnam
Temporary file was created in the default stream wrapper subdirectory
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'public://vxdio1mu'
+'public://'

core/tests/Drupal/KernelTests/Core/File/StreamWrapperTest.php:174

NW for the test location.

I don't believe we have test coverage for this method, so now would be a good time to add it.

OK. I created an issue 📌 FileSystem::tempnam() should use realpath() instead of getDirectoryPath() Active so we can tackle that first.

kim.pepper → created an issue.

Left some feedback. NW for that and coding standard issue.

Created a MR from the patch in #69 and hid patch files.

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

Could we start by looking at how we would change \Drupal\Core\File\FileSystem::tempnam() to not use ::getDirectoryPath()?

Do we need more strict type checks for LocalStream?

There was no public issue that made the change as it was a security issue. Here's the commit https://git.drupalcode.org/project/drupal/-/commit/cf82188fcf317e185716f...

I have checked the MR and confirmed it mentions uris and paths and the english is correct.

Current test fails are unrelated as this are docs only changes. RTBC for when it comes back green.

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

I don't think this is something we will support within core.

Left some feedback and updated the title.

These tests

• Drupal\FunctionalTests\Installer\InstallerTest
• \Drupal\Tests\system\Functional\System\SitesDirectoryHardeningTest

are failing which look related.

Also hid a load of outdated patch files.

Created an MR from #194 so we can run ci pipelines.

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

This was fixed in 9.1.x. It was marked needs work for a backport to D7 however that is no longer supported. Marking fixed.

This was fixed in 9.1.x. and left open for a backport to D7. Marking fixed since D7 is no longer supported.

If there are tests required still we should open a new issue.

This was committed but never closed.

Closing as per #25 looks like this was a hosting provider issue.

Looks like just a few PHPCS errors. Also tagging for Needs Tests. I think it would be good for a test to ensure it all get's wired up correctly.

Released in 2.4.0 →

Committed to 3.x and 2.x. Thanks!

Changes go into 3.x.

Updated title

Marking RTBC. Hoping other maintainers can review as well.

I agree this would be useful. OpenSearch will create an index when the first document is pushed, so this is existing behaviour. We are just making sure our mappings are correct when the index is created.

fubarhouse → credited kim.pepper → .

Currently we default to application/octet-stream if we can't determine the mime type. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/MIME_types#appl...

Any kind of binary data that doesn't fall explicitly into one of the other types;

See also 🐛 Separate MIME type mapping from ExtensionMimeTypeGuesser Needs work Not sure about the automatic extension change tho.

Yeah, not sure of the history of this one. I think we could do away with the check for apache and nginx and just display the default "extension missing" message.

Looks good. Can we add a test?

I'd say this is outdated since we just check if the extension is enabled now.

> I can't create MR from the fork as I might not have access.

Did you click "Get Push Access"?

Also all changes go into 3.x first.

I'm trying an alternative approach in https://git.drupalcode.org/project/drupal/-/merge_requests/11533 however I can't tell if test fails are related or not? 🤔

Instead of adding onto the Time class, I'm creating a dedicated Clock implementation.

All good now.

Reviewed this and checked the property can still be accessed and there is a test to trigger the deprecation.

RTBC once we have a CR.

> I imagine this will need a CR?

Yep.

> How would we want to handle the removal of this code in D12? Do we wait for this to be committed and open a follow up?

I think this would get handled when we make deprecation removal issues when 12.x opens up.

I'm seeing a few changes that look unrelated. You might need to rebase on 11.x?

Yeah I think a sub-classed search backend would work. I think a sub-module in this project would make more sense since there would be minimal differences.

Left one nitpick.

kim.pepper → changed the visibility of the branch 3501375-symfony-clock to hidden.

I created a follow-up 📌 [PP-1] Deprecate TimeInterface::getCurrentTime() and replace with ClockInterface::now()->getTimestamp() Postponed so back to RTBC.

I also squashed the commits and rebased on the latest 11.x. This will help if we need to continually rebase on 11.x until it gets committed.

kim.pepper → created an issue.

We might want to do #17 and deprecate \Drupal::time()->getCurrentTime() in a follow up.

I was creating a follow up issue to replace TimeInterface->getCurrentTime() with ClockInterface->now()->getTimestamp() in the rest of core.

I searched for usages of \Drupal::time()->getCurrentTime() and found 28 instances.

\Drupal::time() returns \Drupal\Component\Datetime\TimeInterface but this doesn't extend Psr\ClockInterface, so it's not a straight method swap.

I think we need a new \Drupal::clock() method that returns Psr\ClockInterface. This would make it easier to swap out usage of \Drupal::time()->getCurrentTime() with \Drupal::clock()->now()->getTimestamp().

I know there is an existing issue about this somewhere but I can't find it.

Reviewed and added some feedback.

Added a CR and updated the IS. I think this is ready.

I think this was fixed in 📌 Use \OpenSearch\Aws\SigningClientFactory Active with this code:

    // Set credentials if provided, otherwise fall back to defaults.
    if ('' !== $this->configuration['api_key'] && '' !== $this->configuration['api_secret']) {
      $options['auth_aws']['credentials'] = [
        'access_key' => $this->configuration['api_key'],
        'secret_key' => $this->configuration['api_secret'],
      ];
    }

Because this uses logic in the library, we can only check to see if the 'credentials' array key gets set. See https://github.com/opensearch-project/opensearch-php/blob/main/src/OpenS...

// Check for provided access key and secret.
        if (isset($options['credentials'])) {
            return CredentialProvider::fromCredentials(
                new Credentials(
                    $options['credentials']['access_key'] ?? '',
                    $options['credentials']['secret_key'] ?? '',
                    $options['credentials']['session_token'] ?? null,
                )
            );
        }

        // Fallback to the default provider.
        return CredentialProvider::defaultProvider();

Are you able to manually test this now? If there are issues with the opensearch-php library, feel free to create an issue there too.

📌 Use \OpenSearch\Aws\SigningClientFactory Active is in so this is unblocked.

Committed to 3.x and 2.x.

opensearch-php 2.4.3 has been released

We looked at this at the first-time contributor workshop at DrupalSouth 2025. The following users helped test this manually but could not reproduce:

atowl, lan, yi_jiang, Ronbot, nterbogt, silverham, tarawij, ben_a, cordo1, jimmycann, tyrellblackburn,

We tested by:
1. Deleting the version from the standard.info.yml file (which we installed)
2. Clearing our cache
3. Visiting the Status Report page

Can you please provide more steps to reproduce.

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

If we are adding Psr/Clock as a dependency, then there should be at least one instance where we swap out to use it.

I'm wondering if we don't just add symfony/clock and replace usages of datetime.time service? 🤔

@vijaycs85 would be great if you could test out 📌 Use \OpenSearch\Aws\SigningClientFactory Active and see if it fixes your issues before we commit it.

We should postpone this on a tagged release of the opensearch-php lib.

I think this is due to not checking for empty strings when passing the credentials options.

I'm going to postpone this one on 📌 Use \OpenSearch\Aws\SigningClientFactory Active which changes how we create the signing client.

kim.pepper → created an issue.

\Drupal\Core\File\MimeType\MimeTypeGuesser::guessMimeType() defaults to 'application/octet-stream' if it can't determine a mime type. Would it make sense to default to this rather than empty string?

I think this is the expected behaviour.

1. '_' is a valid separation character and santization only replaces invalid characters.
2. '_-' gets converted to '-' because we de-duplicate separation characters

Reviewed and confirmed the links are correct.

Thanks for reporting @vijaycs85

Can you please confirm the versions of search_api_opensearch and opensearch-project/opensearch-php you are using?

NW for the code style fails

Committed to 3.x and 2.x

kim.pepper → created an issue.

I ran the test-only pipeline and it fails, but not where I expected it to. Any thoughts?

1) Drupal\KernelTests\Core\File\StreamWrapperCustomRootDirectoryTest::testRealPathHandlesCustomRootDirectory
TypeError: stat(): Argument #1 ($filename) must be of type string, false given

The temporary file path is a setting and no longer config since Drupal 8.8.0.

See the changelog: https://www.drupal.org/node/3039255 → for details.

Can we update the IS to mention this only affects root directories (or symlinks to them)?

1. Should we have a config migration path?

you mean add an update hook?

Yep.

Would be great to have a test that covered this.

RTBC +1

I've rebased this on 11.x. In my view this is ready.

@smustgrave it's not as simple as moving the fields around. We need to merge the output from the parent \Drupal\file\Plugin\Field\FieldFormatter\FileMediaFormatterBase::settingsForm(). Trying to splice it into the middle is going to make it pretty fragile imo.

I think we need to handle existing sites a bit better.

• Should we have a config migration path?
• The wording on the form description isn't that helpful to those who don't know what the possible values are.
• Should we have an options list with just OpenSearch and OpenSearch Serverless?
• Can we just default to OpenSearch for existing users.

kim.pepper → changed the visibility of the branch 3479880-allow-aws-signature to hidden.

Created a MR from the patch at #7

Sounds like a good idea.

Committed to 2.x. Thanks!

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

Committed to 3.x and 2.x. Thanks!

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

+++ b/modules/search_api_aws_signature_connector/src/Plugin/OpenSearch/Connector/AwsSignatureConnector.php
@@ -91,6 +93,17 @@ class AwsSignatureConnector extends StandardConnector {
+    ];

Should we default this to 'es'?

Fixed in 📌 Automated Drupal 10 compatibility fixes Fixed

Since we updated the opensearch-php library to 2.4.x you can use the \Drupal\search_api_aws_signature_connector\AwsSigningHttpClientFactory and pass your own Aws\Credentials\CredentialProvider in the service definition.

We might be able to make it an option in the settings form.