🇦🇺Australia @kim.pepper

bbrala → credited kim.pepper → .

larowlan → credited kim.pepper → .

Left some feedback.

Thanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.

As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" more than 1 year ago.

Since we need more information to move forward with this issue, I am closing it.

Please feel free to reopen with more information.

Committed to 2.x

kim.pepper → created an issue.

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

I think we can safely close this one as outdated. file_create_url() was deprecated in drupal 9.3.0 and template_preprocess_file_link() now calls \Drupal\Core\File\FileUrlGeneratorInterface::generate() which returns a \Drupal\Core\Url object (not FALSE or NULL).

Also file_unmanaged_prepare() was deprecated in drupal 8.7.0 and replaced with \Drupal\Core\File\FileSystem::prepareDestination() so we need an IS update.

The first step should be to update the documentation to clarify the URI only supports a local streamwrapper. We can then create a follow up to discuss adding remote streams.

The list of allowed extensions comes directory from the extension validator. Can we confirm this bug still exists?

file_url_transform_relative() was deprecated in 9.3.0 and was converted to \Drupal\Core\File\FileUrlGeneratorInterface::transformRelative(). At the very least this needs an IS update. Can we check if this is still an issue in the new code?

Closing as a duplicate as per #12

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

Left some feedback in the MR

griffynh → credited kim.pepper → .

OK thanks for clarifying @FeyP. Going to mark this RTBC.

Adding credit

I found a lot of other examples in file module where we are using uid => 1 Are we sure we can't remove these special behaviours either?

❯ grep -nri "'uid' => 1"
tests/src/Kernel/Migrate/d6/MigrateUploadTest.php:43:        'uid' => 1,
tests/src/Kernel/Validation/FileValidatorTestBase.php:48:      'uid' => 1,
tests/src/Kernel/FileManagedUnitTestBase.php:40:    $user = User::create(['uid' => 1, 'name' => $this->randomMachineName()]);
tests/src/Kernel/FileManagedUnitTestBase.php:170:      'uid' => 1,
tests/src/Kernel/FileManagedAccessTest.php:50:      'uid' => 1,
tests/src/Kernel/FileManagedAccessTest.php:65:      'uid' => 1,
tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:33:        'uid' => 1,
tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:43:        'uid' => 1,
tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:53:        'uid' => 1,
tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:68:        'uid' => 1,
tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:78:        'uid' => 1,
tests/src/Kernel/FileUriItemTest.php:27:      'uid' => 1,
tests/src/Functional/FileManagedTestBase.php:157:      'uid' => 1,
tests/src/Functional/FileListingTest.php:235:      'uid' => 1,
tests/src/Functional/FileListingTest.php:268:      'uid' => 1,

Committed to 3.x and 2.x

kim.pepper → created an issue.

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

Saving credit

kim.pepper → created an issue.

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

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

Updated title and IS

Made the fix a bit simpler and added test coverage.

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

Agree there is no need for a FallbackMimeTypeGuesser if we can return 'application/octet-stream' by default from \Drupal\Core\File\MimeType\MimeTypeGuesser::guessMimeType()

griffynh → credited kim.pepper → .

Should probably be Closed outdated

As per #62

Once 10.3 is out and 10.2 is security only, this is obsolete.

I assume we can now mark this as closed won't fix.

Duplicate of 🐛 StreamWrapperInterface return types for realpath() and dirname() are inconsistent with documented use and intention Needs work

Sorry for all the noise. I think I went back and forth with this too many times, and made changes I shouldn't have.

Hmm. I'm having second thoughts about reverting. I think we need to return NULL.

@longwave I thought we were doing this issue to fix the problem of guessers that come after.

Thus, any mime type guesser that would otherwise come afterwards is ignored.

However, I think we can use priority to allow workarounds.

Reverted.

kim.pepper → created an issue.

Hiding patch files

Tried a new approach to add a constructor param to trigger a deprecation.

Spent some time on looking at the patch and how this could be implemented here. The code that checks for whether a field is a list or not is quite complex and seems to indicate there is a lack of trust in the TypeData definition isList() method.

If we were to proceed, I would expect we would need pretty decent Kernel test coverage to ensure indexing and querying work as expected.

See the fix in Search API OpenSearch 🐛 Handle missing return type in __wakeup() Fixed

Noticed that writing test.bbb to the filesystem was causing issues here so created a follow up 📌 FileSaveUploadTest should not write to the app root Needs review

kim.pepper → created an issue.

This was fixed in Search API OpenSearch by overriding \Drupal\Core\DependencyInjection\DependencySerializationTrait::__wakeup() to add a return type. See 🐛 Handle missing return type in __wakeup() Fixed

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

This is an issue because \Drupal\Core\DependencyInjection\DependencySerializationTrait::__sleep() and __wakeup() do not have support types in 10.2.x/10.3.x but they have been added in 11.x.

As PHP supports covariance, we can ensure our class implements the return types by overriding these methods and we can remove them when 10.x versions are no longer supported.

https://www.php.net/manual/en/language.oop5.variance.php

Adding return types should be done on the implementing classes first.

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

For example:

   * @return string|null
   *   The mime type or NULL, if none could be guessed.
   *   For BC, this currently returns 'application/octet-stream' when none could
   *   be guessed but will return NULL in drupal:12.0.0.

and // @todo https://www.drupal.org/node/3156672 Return NULL in Drupal 12.0.0.

As per https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... → we can't really trigger a deprecation warning or use the @deprecated annotation. Instead we need to document the behaviour change in the docblock, and add a @todo to switch to returning NULL in the next major.

Committed to 3.x. Thanks!

Just to clarify, BC is for 2.x branch. An MR without BC is ok for the 3.x branch.

NW for BC support

This looks good to me. I'll leave it as RTBC in case anyone else has feedback over the next 24hrs.

Committed to 2.x. Thanks!

NW for feedback and test fails

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

Can you please rebase this onto the latest 2.x branch so we get gitlab ci codesniffer validation running? Thanks.

This MR will need to be rebased to go onto 3.x branch. It's currently on 1.x which is security fixes only.

bbrala → credited kim.pepper → .

We need to fix the next major (11.x) test fail. This is what this issue is about.

Committed to 2.x and 3.x thanks!

I fixed a few codesniff issues in the 3.x branch so we can focus on just the 11.x compatibility here.

Committed to 3.x and 2.x

kim.pepper → created an issue.

Left some feedback.

Hiding some files

I've toned this down to only add autoconfigure: true to non-test modules. I had to exclude the mysql.services.yml too for reasons I haven't looked into.

kim.pepper → created an issue.

Note 📌 Add support for !service_closure custom tag in YamlFileLoader Fixed went in recently.

NW for #86

Left some feedback.

I also added autowire: true everywhere too, which I think we should remove and just do autoconfigure: true first.

kim.pepper → created an issue.

@ankitv18 go for it!

Thanks for working on this. FYI this is blocking Search API Opensearch Drupal 11.x support. 📌 Drupal 11 readiness Active

kim.pepper → created an issue.

Given we are held up with concerns in ✨ Turn autowire & autoconfigure ON/OFF globally Needs review should we go ahead and do it the manual way here first?

I was curious so I added autoconfigure: true and autowire: true to every services.yml file to see what happens.

@liquidcms I suggest you create a new issue to add this feature.

Drop 9.x support

kim.pepper → created an issue.

Added a change record.

Updated IS with more background

Thanks @lhridley for your detailed investigation. I'm happy with the changes in the MR.