Problem/Motivation

Remove all uses of file_stream_wrapper_get_* and file_get_stream_wrappers

Proposed resolution

Remove
file_stream_wrapper_get_class($scheme)
with
\Drupal::service('stream_wrapper_manager')->getClass($scheme)

Remove
file_stream_wrapper_get_instance_by_uri($uri)
with
\Drupal::service('stream_wrapper_manager')->getViaUri($uri)

Remove
file_stream_wrapper_get_instance_by_scheme($scheme)
with
\Drupal::service('stream_wrapper_manager')->getViaScheme($scheme)

Remove
file_get_stream_wrappers($filter = StreamWrapperInterface::ALL)
with
\Drupal::service('stream_wrapper_manager')->getWrappers($filter)

Remaining tasks

Provide patch

User interface changes

API changes

Remove all the uses of following function:

file_stream_wrapper_get_class
file_stream_wrapper_get_instance_by_uri
file_stream_wrapper_get_instance_by_scheme
file_get_stream_wrappers

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Normal as the patch removes usage of already deprecated functions
Prioritized changes code already marked for removal by 8.0.0
CommentFileSizeAuthor
#69 remove_all_uses_of-2392559-69.patch29.63 KBmondrake
#69 interdiff_66-69.txt822 bytesmondrake
#66 remove_all_uses_of-2392559-66.patch29.18 KBmondrake
#66 interdiff_65-66.txt1.41 KBmondrake
#65 remove_all_uses_of-2392559-65-reroll.patch27.92 KBmondrake
#58 remove_all_uses_of-2392559-58.patch27.88 KBJeroenT
#55 remove_all_uses_of-2392559-55.patch27.89 KBJeroenT
#51 remove_all_uses_of-2392559-51.patch27.91 KBmondrake
#51 interdiff_49-51.txt1.16 KBmondrake
#49 remove_all_uses_of-2392559-49.patch26.75 KBmondrake
#49 interdiff_48-49.txt2.5 KBmondrake
#48 remove_all_uses_of-2392559-48.patch24.57 KBmondrake
#48 interdiff_47-48.txt7.78 KBmondrake
#47 remove_all_uses_of-2392559-47.patch25.98 KBmondrake
#46 remove_all_uses_of-2392559-38.patch26.01 KBmondrake
#38 remove_all_uses_of-2392559-38.patch26.01 KBmondrake
#37 remove_all_uses_of-2392559-37.patch33.13 KBmondrake
#37 interdiff_34-37.txt5.48 KBmondrake
#34 remove_all_uses_of-2392559-34.patch30.55 KBJeroenT
#33 remove_all_uses_of-2392559-33.patch0 bytesJeroenT
#31 interdiff.txt1.75 KBJeroenT
#30 interdiff.txt1.75 KBJeroenT
#30 2392559-30.patch30.45 KBJeroenT
#28 2392559-28.patch30.48 KBrpayanm
#28 2392559-interdiff.txt635 bytesrpayanm
#26 2392559-25-26.txt1.3 KBadci_contributor
#26 remove_all_uses_of-2392559-26.patch30.48 KBadci_contributor
#25 remove_all_uses_of-2392559-25.patch30.1 KBadci_contributor
#21 2392559-21.patch29.91 KBrpayanm
#21 2392559-interdiff.txt7.31 KBrpayanm
#18 remove_all_uses_of-2392559-18.patch25.6 KBmitrpaka
#17 2392559-17.patch25.61 KBrpayanm
#17 2392559-interdiff.txt636 bytesrpayanm
#15 interdiff-2392559-14-15.txt2.07 KBmitrpaka
#15 remove_all_uses_of-2392559-15.patch25.6 KBmitrpaka
#14 interdiff-2392559-11-14.txt4.24 KBmitrpaka
#14 remove_all_uses_of-2392559-14.patch23.73 KBmitrpaka
#11 interdiff-2392559-9-11.txt1.48 KBmitrpaka
#11 remove_all_uses_of-2392559-11.patch20.86 KBmitrpaka
#9 interdiff-4-9.txt1.94 KBianthomas_uk
#9 remove_all_uses_of-2392559-9.patch19.98 KBianthomas_uk
#7 interdiff-7.txt16.62 KBmitrpaka
#7 remove_all_uses_of-2392559-7.patch23.5 KBmitrpaka
#4 remove_all_uses_of-2392559-4.patch18.74 KBmitrpaka
#1 remove_all_uses_of-2392559-1.patch18.47 KBmitrpaka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mitrpaka’s picture

Here is the patch to remove all uses of file_stream_wrapper_get_*, file_get_mimetype and file_get_stream_wrappers - comments lines has been left intact.

mitrpaka’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: remove_all_uses_of-2392559-1.patch, failed testing.

mitrpaka’s picture

Extension test cases re-run locally.

mitrpaka’s picture

Status: Needs work » Needs review
ianthomas_uk’s picture

Status: Needs review » Needs work

Thanks for your work here.

  1. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeGuesser.php
    @@ -39,7 +39,7 @@ class MimeTypeGuesser implements MimeTypeGuesserInterface {
    -    if ($wrapper = file_stream_wrapper_get_instance_by_uri($path)) {
    +    if ($wrapper = \Drupal::service('stream_wrapper_manager')->getViaUri($path)) {
    

    This is OO code, can we inject stream_wrapper_manager?

  2. +++ b/core/lib/Drupal/Core/Updater/Updater.php
    @@ -343,7 +343,7 @@ public function makeBackup(FileTransferInterface $filetransfer, $from, $to) {
    -    return file_stream_wrapper_get_instance_by_scheme('temporary')->getDirectoryPath();
    +    return \Drupal::service('stream_wrapper_manager')->getViaScheme('temporary')->getDirectoryPath();
    

    OO code, injection?

I've not checked the code base after applying this patch, but if there are still references in comments then they will need to be removed too.

We will also need to link the relevant change record to this issue (or write one if it doesn't exist).

Please include an interdiff when you update a patch https://www.drupal.org/documentation/git/interdiff

mitrpaka’s picture

Status: Needs work » Needs review
FileSize
23.5 KB
16.62 KB

Patch re-rolled. Comments taken into account.

mitrpaka’s picture

ianthomas_uk’s picture

FileSize
19.98 KB
1.94 KB

Please ask for clarification if you don't understand a review comment - you've just used intermediate variables, which is not dependency injection. Drupal DI docs are at https://www.drupal.org/node/2133171, although I appreciate they aren't very good.

Attached is a reroll of #4, using injection in MimeTypeGuesser. I've left Update alone, as that's a bit of a special case and is already using \Drupal elsewhere in the file.

Relevant change records are linked.

Status: Needs review » Needs work

The last submitted patch, 9: remove_all_uses_of-2392559-9.patch, failed testing.

mitrpaka’s picture

Status: Needs work » Needs review
FileSize
20.86 KB
1.48 KB

@ianthomas_uk: Thank you for your help and guidance.

Updated patch to pass tests.

Status: Needs review » Needs work

The last submitted patch, 11: remove_all_uses_of-2392559-11.patch, failed testing.

ianthomas_uk’s picture

  1. +++ b/core/modules/system/src/Tests/File/StreamWrapperTest.php
    @@ -78,7 +78,7 @@
    -    $instance = \Drupal::service('stream_wrapper_manager')->getViaScheme($this->scheme . '://foo');
    +    $instance = \Drupal::service('stream_wrapper_manager')->getViaUri($this->scheme . '://foo');
    

    Oops. Good point (it was late)

  2. +++ b/core/tests/Drupal/Tests/Core/File/MimeTypeGuesserTest.php
    @@ -34,7 +34,7 @@ public function testSymfonyGuesserRegistration() {
    -    $container->set('file.mime_type.guesser', new MimeTypeGuesser());
    +    $container->set('file.mime_type.guesser', new MimeTypeGuesser($container->get('stream_wrapper_manager')));
    

    This is a test, so you can use \Drupal:: instead of $container->get() ($container has only been created on the previous line, it isn't a normal Drupal container)

mitrpaka’s picture

Status: Needs work » Needs review
FileSize
23.73 KB
4.24 KB

Updated patch to pass MimeTypeGuesserTest unit test. Comments taken into account.

mitrpaka’s picture

file_get_mimetype removals were missing since remove_all_uses_of-2392559-9.patch. Now added.

Status: Needs review » Needs work

The last submitted patch, 15: remove_all_uses_of-2392559-15.patch, failed testing.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
636 bytes
25.61 KB

Missing ")".

mitrpaka’s picture

Typo in patch. Actually one "(" too many.

rpayanm’s picture

ohhh that's right!

ianthomas_uk’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -255,8 +255,8 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    -        // file_get_stream_wrappers() needs to re-register Drupal's stream
    -        // wrappers in case a module-provided stream wrapper is used later in
    +        // Drupal's stream wrappers needs to be re-registered
    +        // in case a module-provided stream wrapper is used later in
    

    This comment block needs to be reformatted to wrap at 80 chars

  2. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
    @@ -414,7 +414,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -        $values['favicon_mimetype'] = file_get_mimetype($values['favicon_path']);
    +        $values['favicon_mimetype'] = \Drupal::service('file.mime_type.guesser')->guess($values['favicon_path']);
    

    This should be injected

  3. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
    @@ -172,7 +172,7 @@ public function save($destination) {
    -      $local_wrappers = file_get_stream_wrappers(StreamWrapperInterface::LOCAL);
    +      $local_wrappers = \Drupal::service('stream_wrapper_manager')->getWrappers(StreamWrapperInterface::LOCAL);
    

    This should be injected

  4. +++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
    @@ -279,14 +280,14 @@ function testModuleMetaData() {
    -    $stream_wrappers = file_get_stream_wrappers();
    +    $stream_wrappers = \Drupal::service('stream_wrapper_manager')->getWrappers(StreamWrapperInterface::ALL);
    

    There's no need to add the explicit parameter here - it wasn't in the previous call, and will default to that value anyway.

  5. +++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
    @@ -279,14 +280,14 @@ function testModuleMetaData() {
    -    $stream_wrappers = file_get_stream_wrappers();
    +    $stream_wrappers = \Drupal::service('stream_wrapper_manager')->getWrappers(StreamWrapperInterface::ALL);
    

    Parameter also unnecessary

Looks OK to me other than the above. I couldn't find any remaining references to these functions.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
7.31 KB
29.91 KB

Please review

ianthomas_uk’s picture

Status: Needs review » Needs work

Just the line length in that comment block still to do.

Status: Needs work » Needs review

adci_contributor queued 21: 2392559-21.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 21: 2392559-21.patch, failed testing.

adci_contributor’s picture

Status: Needs work » Needs review
FileSize
30.1 KB

Just reroll.

adci_contributor’s picture

FileSize
30.48 KB
1.3 KB

And updated in according to #20-1.

ianthomas_uk’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/src/Entity/File.php
@@ -185,7 +185,7 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
+      $values['filemime'] = \Drupal::service('file.mime_type.guesser')->guess(($values['uri']));

The double brackets seem to have reappeared on this line:

guess((

The interdiff in #21 must be to #17 not #18.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
635 bytes
30.48 KB
mondrake’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -54,6 +58,36 @@ class GDToolkit extends ImageToolkitBase {
+   * @var \Drupal\Core\StreamWrapper\StreamWrapperManager
...
+   * @param \Symfony\Component\DependencyInjection\ContainerAwareInterface $stream_wrapper_manager
+   *   The StreamWrapper manager.
+   */

this looks weird - shouldn't it be typehinted as \Drupal\Core\StreamWrapper\StreamWrapperManager instead? and in the constructor and the use statement as well.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
30.45 KB
1.75 KB

Replaced ContainerAwareInterface with StreamWrapperManager as suggested by mondrake in #29.

JeroenT’s picture

FileSize
1.75 KB

This is the right interdiff.

Status: Needs review » Needs work

The last submitted patch, 30: 2392559-30.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
0 bytes

Woops, i was working on an older version of 8.0.x. Created re-roll and replaced ContainerAwareInterface with StreamWrapperManager as suggested by mondrake in #29.

JeroenT’s picture

FileSize
30.55 KB

This is the right patch.

ianthomas_uk’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, all comments addressed and change records linked

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
    @@ -69,7 +68,7 @@ class StreamWrapperManager extends ContainerAware {
    -   *   $local_stream_wrappers = file_get_stream_wrappers(StreamWrapperInterface::LOCAL);
    +   *   $local_stream_wrappers = $this->getWrappers(StreamWrapperInterface::LOCAL);
    
    @@ -79,7 +78,7 @@ class StreamWrapperManager extends ContainerAware {
    -   *   $remote_stream_wrappers = array_diff_key(file_get_stream_wrappers(StreamWrapperInterface::ALL), file_get_stream_wrappers(StreamWrapperInterface::LOCAL));
    +   *   $remote_stream_wrappers = array_diff_key($this->getWrappers(StreamWrapperInterface::ALL), $this->getWrappers(StreamWrapperInterface::LOCAL));
    

    This is example code - the use of $this does not look right. \Drupal::service('stream_wrapper_manager') is a little bit better.

  2. +++ b/core/modules/image/src/PathProcessor/PathProcessorImageStyles.php
    @@ -31,7 +31,7 @@ class PathProcessorImageStyles implements InboundPathProcessorInterface {
    -    $directory_path = file_stream_wrapper_get_instance_by_scheme('public')->getDirectoryPath();
    +    $directory_path = \Drupal::service('stream_wrapper_manager')->getViaScheme('public')->getDirectoryPath();
    

    This can be injected in image.service.yml

  3. +++ b/core/modules/image/src/Routing/ImageStyleRoutes.php
    @@ -26,7 +26,7 @@ public function routes() {
    -    $directory_path = file_stream_wrapper_get_instance_by_scheme('public')->getDirectoryPath();
    +    $directory_path = \Drupal::service('stream_wrapper_manager')->getViaScheme('public')->getDirectoryPath();
    

    This can be injected by implementing ContainerInjectionInterface.

  4. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
    @@ -12,6 +12,7 @@
    +use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface;
    
    @@ -38,6 +39,13 @@ class ThemeSettingsForm extends ConfigFormBase {
    +   * The MIME type guesser.
    +   *
    +   * @var \Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface
    +   */
    +  protected $mimeTypeGuesser;
    +
    +  /**
    
    @@ -49,16 +57,19 @@ class ThemeSettingsForm extends ConfigFormBase {
    -   * @param \Drupal\Core\Extension\ModuleHandlerInterface
    +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    ...
    -   * The theme handler.
    +   *   The theme handler.
    +   * @param \Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface $mime_type_guesser
    +   *   The MIME type guesser instance to use.
    ...
    -  public function __construct(ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler) {
    +  public function __construct(ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, MimeTypeGuesserInterface $mime_type_guesser) {
    ...
    +    $this->mimeTypeGuesser = $mime_type_guesser;
    
    @@ -68,7 +79,8 @@ public static function create(ContainerInterface $container) {
    -      $container->get('theme_handler')
    +      $container->get('theme_handler'),
    +      $container->get('file.mime_type.guesser')
    
    @@ -446,7 +458,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -        $values['favicon_mimetype'] = file_get_mimetype($values['favicon_path']);
    +        $values['favicon_mimetype'] = $this->mimeTypeGuesser->guess($values['favicon_path']);
    

    Unrelated?

mondrake’s picture

Status: Needs work » Needs review
FileSize
5.48 KB
33.13 KB

#36

1,2,3 - Done
4 - Why? It's all there to allow removal of usage of file_get_mimetype in

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -446,7 +458,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
-        $values['favicon_mimetype'] = file_get_mimetype($values['favicon_path']);
+        $values['favicon_mimetype'] = $this->mimeTypeGuesser->guess($values['favicon_path']);

which is in the scope. Or I miss something?

mondrake’s picture

Issue summary: View changes
FileSize
26.01 KB

file_get_mimetype() has just been removed in #2388749: Register symfony's mime guessers if they are supported, so that piece is no longer relevant here.

Rerolled, the patch is smaller now. Sorry no interdiff.

mondrake’s picture

Title: Remove all uses of file_stream_wrapper_get_*, file_get_mimetype and file_get_stream_wrappers » Remove all uses of file_stream_wrapper_get_* and file_get_stream_wrappers

mondrake’s picture

Title: Remove all uses of file_stream_wrapper_get_* and file_get_stream_wrappers » Remove all uses of file_stream_wrapper_get_*, file_get_mimetype and file_get_stream_wrappers
Issue summary: View changes

#2388749: Register symfony's mime guessers if they are supported was rolled back and postponed to later than 8.0.

Reverting changes since #38, patch in #37 is the one to review now.

ianthomas_uk’s picture

As shown by alexpott's question in #36 there isn't really a particularly strong link between file_get_mimetype and the other functions that we're removing. They were also covered by different CRs.

Given that we've now got a patch that just does the other functions, does it make sense to split off file_get_mimetype into its own issue?

ianthomas_uk’s picture

removed - double post

mondrake’s picture

#42 fair point, let's see if #38 still runs and then we can split.

mondrake’s picture

Title: Remove all uses of file_stream_wrapper_get_*, file_get_mimetype and file_get_stream_wrappers » Remove all uses of file_stream_wrapper_get_* and file_get_stream_wrappers
Issue summary: View changes
FileSize
26.01 KB

Created spin-off #2415757: Remove all uses of file_get_mimetype as suggested in #42.

#38 is still good, re-adding here to keep things clean.

mondrake’s picture

FileSize
25.98 KB

Reroll after #2364157: Replace most existing _url calls with Url objects, only patch context changes

mondrake’s picture

Reroll after #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class, and changed to StreamWrapperManagerInterface where appropriate.

mondrake’s picture

Found another one that got lost

Status: Needs review » Needs work

The last submitted patch, 49: remove_all_uses_of-2392559-49.patch, failed testing.

mondrake’s picture

Fixed PHPUnit test

mondrake’s picture

Status: Needs work » Needs review
andypost’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
@@ -12,7 +12,6 @@
- * @see file_get_stream_wrappers()

+++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManagerInterface.php
@@ -10,7 +10,6 @@
- * @see file_get_stream_wrappers()

should be replaced not removed

rpayanm’s picture

@andypost By what should be replaced?
Because on the next line said:

@see \Drupal\Core\StreamWrapper\StreamWrapperInterface

IMHO I think that with that is enough.

JeroenT’s picture

Status: Needs work » Needs review
Issue tags: +drupaldevdays
FileSize
27.89 KB

Patch no longer applied. Created re-roll.

JeroenT’s picture

Issue tags: +Needs reroll
JeroenT’s picture

Issue tags: -Needs reroll
FileSize
27.88 KB

Created straight reroll. Patch attached.

JeroenT’s picture

I just realised this patch didn't need a reroll...

Status: Needs review » Needs work

The last submitted patch, 58: remove_all_uses_of-2392559-58.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review

Patch in #55 still applies.

The last submitted patch, 55: remove_all_uses_of-2392559-55.patch, failed testing.

JeroenT’s picture

Issue tags: +Needs reroll
mondrake’s picture

Issue tags: -Needs reroll
FileSize
27.92 KB

Plain reroll.

mondrake’s picture

Fixed a comment and a typo in the @deprecation of file_stream_wrapper_get_instance_by_uri, not sure if because of that the deprecation should now be postponed.

Also, what is the preference for services injection in tests? i.e. call \Drupal::service or $this->container->get?

The current patch does this

+++ b/core/modules/system/src/Tests/Session/SessionTest.php
@@ -290,7 +290,7 @@ function sessionReset($uid = 0) {
-    $this->cookieFile = file_stream_wrapper_get_instance_by_scheme('temporary')->getDirectoryPath() . '/cookie.' . $uid . '.txt';
+    $this->cookieFile = \Drupal::service('stream_wrapper_manager')->getViaScheme('temporary')->getDirectoryPath() . '/cookie.' . $uid . '.txt';

should it do

+++ b/core/modules/system/src/Tests/Session/SessionTest.php
@@ -290,7 +290,7 @@ function sessionReset($uid = 0) {
-    $this->cookieFile = file_stream_wrapper_get_instance_by_scheme('temporary')->getDirectoryPath() . '/cookie.' . $uid . '.txt';
+    $this->cookieFile = $this->container->get('stream_wrapper_manager')->getViaScheme('temporary')->getDirectoryPath() . '/cookie.' . $uid . '.txt';

instead?

The last submitted patch, 65: remove_all_uses_of-2392559-65-reroll.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 66: remove_all_uses_of-2392559-66.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
822 bytes
29.63 KB

Fixed unit test.

mondrake’s picture

Issue summary: View changes

Added beta evaluation.

JeroenT’s picture

Issue tags: +@deprecated
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good and my IDE says it does, indeed, remove file_stream_wrapper_get_* and file_get_stream_wrappers.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 73585ec and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 73585ec on 8.0.x
    Issue #2392559 by mondrake, mitrpaka, JeroenT, rpayanm, adci_contributor...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.