🏄‍♂️🇦🇺Sydney, Australia
Account created on 24 September 2008, over 16 years ago
  • Co-Founder and Technical Director at PreviousNext 
  • Co-Founder and Technical Director at Skpr 
#

Merge Requests

More

Recent comments

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Released in 2.4.0

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Changes go into 3.x.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Updated title

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Marking RTBC. Hoping other maintainers can review as well.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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;

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Looks good. Can we add a test?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

> 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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

All good now.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Left one nitpick.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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().

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Reviewed and added some feedback.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 3.x and 2.x.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

opensearch-php 2.4.3 has been released

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

@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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

\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?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Reviewed and confirmed the links are correct.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Thanks for reporting @vijaycs85

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

NW for the code style fails

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 3.x and 2.x

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

1. Should we have a config migration path?

you mean add an update hook?

Yep.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Would be great to have a test that covered this.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

RTBC +1

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

@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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Created a MR from the patch at #7

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Sounds like a good idea.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 2.x. Thanks!

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
+++ 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'?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Thanks @webflo Can you please create an MR against 3.x as that's the active development branch. We can backport to 2.x.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

The event is for altering validation constraints. If you are creating a new one, you probably want to implement your own ConstraintValidator plug-in that extends \Drupal\file\Plugin\Validation\Constraint\BaseFileConstraintValidator.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 8.x-1.x. Thanks!

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 8.x-1.x

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 3.x and 2.x

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Commited to 2.x

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 2.x

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia
🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 2.x

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

kim.pepper created an issue.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

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

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Made it configurable and added an update hook. @yobottehg it would be great if you could test this out manually.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

@yobottehg I've rebased this on 3.x.

Can you explain why you chose a value of 20 rather than any other arbitrary number? Also, should this be configurable?

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

New commits go into 3.x

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Committed to 3.x.

🇦🇺Australia kim.pepper 🏄‍♂️🇦🇺Sydney, Australia

Need to add the composer dependency.

Production build 0.71.5 2024