🇦🇺Australia @kim.pepper

Rebased on 11.x.

I think this might get more attention if it were under theme system.

Looks like permissions aren't set correctly in your file system. Did you follow the setup instructions here? https://www.drupal.org/docs/administering-a-drupal-site/security-in-drup... →

Changes should be against 3.x and need to be in a MR to trigger the CI pipeline.

Looks like some legit test fails.

Rebased on 11.x

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

Updated title.

Updated IS to reflect latest proposed resolution.

Added a default mime type constant of \Drupal\Core\File\MimeType\MimeTypeGuesser::DEFAULT_MIME_TYPE with value 'application/octet-stream' and use that instead of empty string.

Hiding some older patches and MRs.

kim.pepper → changed the visibility of the branch 3469280-drupalfileiconmimetypes-doesnt-handle to hidden.

kim.pepper → changed the visibility of the branch 3469280-File-mime-type-null to hidden.

Changing from a bug to a feature request.

I think this is a performance improvement rather than a bug so changing to a task.

Tagging as needs issue summary update to add in the template, and rename title to replace drupal_move_uploaded_file() which no longer exists.

According to https://www.drupal.org/about/core/policies/core-dependency-policies/depe... → we need framework and release manager signoff on adding a new dependency on psr/clock. 🤷

Rebased on 11.x Tests green again.

TBH I don't have a strong opinion on which characters should be included or excluded. I'll happily defer to those with a stronger security background. I _do_ think removing whitespace will be disruptive to site owners.

Based on #10 I think we can RTBC this now.

Left some suggestions.

I think we agreed on stripping special characters. It was more about which characters to strip. I have a feeling stripping whitespace might be contentious.

I had to look into why unset($transaction) works and it's because we have a \Drupal\Core\Database\Transaction::__destruct() method that calls
$this->transactionManager()->unpile()

Looking at the solution were all options taken?

Do you mean are all the special characters included?

Test fail looks unrelated

Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest::testPlaceholderHtmlEdgeCases
WebDriver\Exception\UnknownError: unknown error: session deleted because of page crash
from unknown error: cannot determine loading status
from tab crashed
  (Session info: chrome-headless-shell=123.0.6312.58)
  (Driver info: chromedriver=123.0.6312.58 (6b4b19e9dfbb93aa414dc045bd445287977d8d7a-refs/branch-heads/6312_46@{#3}),platform=Linux 5.10.234-225.921.amzn2.x86_64 x86_64)

kim.pepper → changed the visibility of the branch 3519726-fix-incorrect-mimetype-service to hidden.

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

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

Created a MR from the commit in #2

This bug only exists in 10.4.x as it triggers a deprecation that was removed in 11.0.x.

Updated the title and IS.

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

Reviewed the code changes and agree with moving legacy tests. Unfortunately a test-only run doesn't not fail to confirm the bug is fixed as the code that would fail has been moved to a new test class. Not sure if it's worth it pursuing?

Addressed feedback and updated the IS.

I think we can create a new requirements check for the config setting. However, there was some pushback (#44_) on showing warnings on a standard install, so we need to be sure this is considered.

@mondrake created https://www.drupal.org/project/sophron/issues/3519605 📌 SophronMimeTypeMapFactory should just override MimeTypeMapFactory::doCreateMap() Active

kim.pepper → created an issue.

Tests back to green.

Rebased on 11.x

I re-ran the pipeline a couple of times to see if it was a random failure, but still getting the fails.

Rebased on 11.x

Back to NR for the feedback from @acbramley

Rebased on 11.x and added docs to default.settings.php

This feature request was posted against D7 which is no longer supported. If there is a feature request that would apply to Drupal 11+ then please feel free to update the Issue Summary and describe the details of that request.

Attempted to write a test for this, but couldn't work out why the invalid operation was not being passed through to \Drupal\Core\StreamWrapper\LocalStream::stream_lock() correctly. 🤔

Updated the IS saying we propose to return FALSE for unknown operations.

Review the code changes and it makes sense to me. Unfortunately we don't have any test coverage. It would be helpful to add tests to demonstrate the bug is fixed. NW for that.

kim.pepper → changed the visibility of the branch 3493632-flock-operations to hidden.

Removed config option, and updated tests now we replace special chars.

Updated title and issue summary to match current MR implementation approach.

Rebased on 11.x

Here's a basic implementation. I haven't looked at any tests yet.

        'FileImageDimensions' => ['maxDimensions' => 50 * 50],

Do you mean ['maxDimensions' => '50x50'] here?

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.