🇦🇺Australia @kim.pepper

I've merged 11.x into this MR. Looks like the Hook Attributes work that got merged is triggering some deprecations that will need looking at.

If we do use Symfony\Contracts\Service\ResetInterface we could collect all services implementing that interface at container build, and provide a simple way to call reset() on them all when needed.

I've added more info to the readme.

This is a duplicate of 🐛 Logging is too noisy by default Closed: won't fix We don't have control over what the opensearch-php lib is logging. You can configure your own logger and use NullLogger if you don't want any logging.

Created a new MR because I don't know how to properly rebase via the gitlab UI. 😞

Rebased on 11.x, simplified the change, and added a test.

Confirmed the test-only pipeline failed:

There was 1 failure:
1) Drupal\KernelTests\Core\File\MimeTypeTest::testFileMimeTypeDetection
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'application/zip'
+'application/octet-stream'

Ah looks like you did test on 10.3.x already. We will need a test to show the before and after with the fix.

This may be resolved with 🐛 ExtensionMimeTypeGuesser::guessMimeType return less accurate MIME type when file extensions have multiple parts Active Are you able to test on 10.3.x?

Added a post update hook and an entity presave as per #43.

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

Adding credits

This seems like a legit response coming from OpenSearch. I'm not sure this module should be doing anything differently. Please comment if you think there is something we can try.

This was fixed in ✨ Ability to disable SSL verification for dev instances Active

quietone → credited kim.pepper → .

Committed to 2.x. Thanks!

I'm a bit late to the party but went looking for an issue like this after a recently being reminded of the poor DX of the current API.

I've tried to go through all 138 comments and it looks to me like they have all been addressed.

The new API in this issue feels like the right level of abstraction to me, and a core API plus experimental module seems like the right approach.

Looks like there is thorough test coverage too, and the IS is up to date.

We still need to get a framework manager to review, but otherwise I would RTBC it.

Looks great. I assume the re-index from one to another is relatively fast?

Confirmed the only changes in behat/mink v1.12.0 are PHP 8.4 compatibility fixes.

I think the right approach is an upgrade hook to set that value.

Reverted ✨ Ability to disable SSL verification for dev instances Active

Reverting this as we don't have an update hook to set the default value.

Committed to 2.x

Committed to 3.x. Thanks!

MR for 2.x needed with BC layer.

Committed to 2.x and 3.x Thanks!

Created a MR for the 3.x branch.

kim.pepper → changed the visibility of the branch 3381020-ability-to-disable to hidden.

kim.pepper → changed the visibility of the branch 3.x to hidden.

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

This might be trickier than I imagined.
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "cron", path: "cron -> automated_cron.subscriber".

Replaced the cron service proxy with a service closure.

I created 📌 Replace lazy service proxy generation with service closures Active to look at the service closure approach. It might be a big change to do in one go, so we could split it into smaller tasks.

kim.pepper → created an issue.

Created an MR for #13

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

Committed to 3.x and 2.x

kim.pepper → created an issue.

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

Added tests and answered question.

Thanks for this. Almost good to go. That ringphp dependency looks like a major PITA.

Yes, the AlterSettingsEvent should probably be called AlterIndexSettingsEvent.

We don't currently have anything to specifically update the server settings.

Thanks. Feedback addressed.

kim.pepper → changed the visibility of the branch 3.x to hidden.

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

I think you can only do this in the 6.1.x to provide a warning that a new param will be added in the next major:

public function getUrl(EntityInterface $entity /*, array $options = [] */);

xjm → credited kim.pepper → .

Yes, I think this is still useful as it reduces boilerplate and makes the definition of listeners simpler. Going to set back to RTBC.

Addressed all feedback. Thanks again.

kim.pepper → created an issue.

Discussed throwing an exception with @mstrelan and agreed that would be a behaviour change that would be out of scope for this issue.

Tests are passing.

kim.pepper → created an issue.

Rebased

Rebased.

Only local images are allowed AFAIK

I agree we could tone it down a bit, however I feel like we need to tell them it has been exposed in data breaches. That's what this module is all about.

I created a POC MR with a FileEye MIME map implementation to check if our interfaces and factory approach works with 3rd party libs.

Thanks for the detailed review @mondrake. Much appreciated. I've responded to all your comments and made changes where I agree. I resolved a few where I did not agree, but feel free to re-open those threads if you disagree.

Found the source of the test fails was a deprecation warning on the legacy hook_file_mimetype_mapping_alter() in file_test.module. We already test this in file_deprecated_test.module so I removed it.

Ready for reviews again!

the hook was triggered when the service was created

Fair enough.

As for the test fail, I have no idea why \Drupal\Tests\block\Functional\BlockCacheTest::testCachePerPage() is failing. It's passing locally for me. Could it be a random fail?

I've done a fair bit of rework to replace the new hook with tagged services. I think triggering hooks during container build is a bit of an antipattern, and this approach uses a more symfony native approach.

#134 Yeah I guess I was talking about the container cache. I misunderstood what you meant by 'Both the old and new alter hooks run whenever the map is used.' thinking you meant when we call guessMimeType() but it's when the service is created.

I did some more work removing setMapping(). I don't think that's going to cause too many issues.

Simple removal and change. RTBC on green.

Changes are inline with the core change record https://www.drupal.org/node/3305664 → so RTBC for me.

It would be impossible to call these without passing a param value so RTBC for me.

Changes are straightforward and pipeline time is down from ~1 hour to ~10 mins.

#131 I don't think that's right.

  file.mime_type.mapper:
    class: Drupal\Core\File\MimeType\MimeTypeMapper
    calls:
      - [alterMapping, ['@module_handler']]

The hook gets fired when this service is created.

#128 Thinking about this further, we should probably remove setMapping() to hide the underlying data structure. Projects like https://www.drupal.org/project/sophron → that use the https://github.com/FileEye/MimeMap library have a different data structure.

#129 hmm. Shouldn't that hook only get fired on container build? It should be cached after that.

function hook_mimetype_alter(?MimeTypeMapperInterface $mime_type_mapper = NULL) gives you the ability to add/remove/update mappings by using the API interface rather than the raw data structure, which is better IMO.

Also different mapping implementations could use different data structures, so what gets returned from getMapping() could be different.

Can't already alter the mapping with the alter hook?

Also created a follow-up 📌 Generate file extension mime type map with fileeye/mimemap update utility Active

Did a fair amount of cleanup on this issue.

• ExtensionMimeTypeGuesser is now using the MimeTypeMapper
• Removed the getMapping() method which was temporary anyway and refactored tests to not call it
• Split up tests and moved them to the Drupal\KernelTests\Core\File\MimeType namespace to make the namespace under lib/
• Handle NULL returned from ExtensionMimeTypeGuesser::guessMimeType()
• Updated the changes to the default map that went in since this was last worked on a year ago
• Added typehints
• Added more dependency injection
• Fixed linting errors
• Updated the deprecation message to refer to 11.1.0 and 12.0.0

Had a quick play with this. It would require adding the library as a production composer dependency and a bridge for our MimeTypeMapperInterface being added in 🐛 Separate MIME type mapping from ExtensionMimeTypeGuesser Needs work

Going to postpone on that for now.

#17 sounds like a great idea. Created 📌 Generate file extension mime type map with fileeye/mimemap update utility Active

kim.pepper → created an issue.

Rule of thumb is convert PSR level to RfcLogLevel if the logging implementation requires it. e.g. syslog.

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

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

kim.pepper → changed the visibility of the branch 11.x to hidden.

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

Ah ok. Let's close this as a duplicate of 🐛 Separate MIME type mapping from ExtensionMimeTypeGuesser Needs work

Hmm. I didn't realise how this was being used.

I think we should use a deprecation helper to introduce usage of the new method ::getMimeType() based on its existence in the implementation class.

I'm not sure about that approach.

It might be better to add a new interface and service and switch that out with a BC layer.

We don't need a re-roll @peter_serfozo as we are using the MR !9114

I created a task 📌 Create a method for getting the MIME type for a file extension from a map Active that should make looking up the MIME type for an extension less hacky.

I created a task 📌 Create a method for getting the MIME type for a file extension from a map Active that should make looking up the MIME type for an extension less hacky.