Problem/Motivation

http://api.drupal.org/api/drupal/includes%21file.inc/function/drupal_rea...

Returns the absolute local filesystem path of a stream URI.

This function was originally written to ease the conversion of 6.x code to use 7.x stream wrappers. However, it assumes that every URI may be resolved to an absolute local filesystem path, and this assumption fails when stream wrappers are used to support remote file storage. Remote stream wrappers may implement the realpath method by always returning FALSE. The use of drupal_realpath() is discouraged, and is slowly being removed from core functions where possible.

Only use this function if you know that the stream wrapper in the URI uses the local file system, and you need to pass an absolute path to a function that is incompatible with stream URIs.

@todo This function is deprecated, and should be removed wherever possible.

Proposed resolution

Replace calls to drupal_realpath() with \Drupal\Core\File\FileSystem::realpath()

Remaining tasks

Find all calls of drupal_realpath() in core
Replace calls with \Drupal\Core\File\FileSystem::realpath()

User interface changes

None

API changes

None
Already deprecated in Change Record:https://www.drupal.org/node/2418133

Data model changes

None

CommentFileSizeAuthor
#64 interdiff-1472946-60.txt646 bytesandypost
#62 1472946_realpath-file_system-62.patch70.52 KBandypost
#62 interdiff-1472946-60.patch646 bytesandypost
#60 1472946_realpath-file_system-60.patch70.32 KBandypost
#60 interdiff-1472946-59.txt16.96 KBandypost
#59 1472946_realpath-50-59-array-reroll.patch52.81 KBandypost
#57 1472946_realpath-50-array-reroll.patch52.29 KBgnuget
#50 1472946_realpath-50.patch52.34 KBgnuget
#50 1472946_realpath-interdiff-47-50.txt47.09 KBgnuget
#47 1472946_realpath-interdiff-44-47.txt7.83 KBgnuget
#47 1472946_realpath-47.patch52.63 KBgnuget
#44 1472946_realpath-44.patch58.3 KBgnuget
#42 1472946_test-again-39_42.patch58.3 KBkiamlaluno
#39 1472946-39.patch58.3 KBsidharthap
#36 interdiff-1472946.txt5.6 KBamit.drupal
#32 remove_use_of-1472946-31.patch57.86 KBHimanshu5050
#30 1472946-30.patch58.3 KBdineshkumares
#26 interdiff-1472946-19-24.txt4.92 KBsidharthap
#24 remove_use_of-1472946-24.patch58.22 KBsidharthap
#20 interdiff-1472946-18-19.txt5.71 KBmmrares
#19 remove_use_of-1472946-19.patch57.87 KBmmrares
#18 remove_use_of-1472946-18-reroll.patch52.59 KBmmrares
#10 remove_use_of-1472946-4-reroll.patch52.59 KBgnuget
#4 remove_use_of-1472946-4.patch52.52 KBgnuget
#4 interdiff-1472946-3-4.txt2.28 KBgnuget
#3 interdiff-1472946-1-3.txt8.74 KBgnuget
#3 remove_use_of-1472946-3.patch50.25 KBgnuget
#1 remove_use_of-1472946-1.patch43.68 KBgnuget
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

gnuget’s picture

Assigned: Unassigned » gnuget
Issue summary: View changes
Status: Active » Needs review
FileSize
43.68 KB

Ok, This is my first try.

gnuget’s picture

Status: Needs review » Needs work

I left a few drupal_realpath in some places because i was unsure how access to the service and I wanted to check the majority first.

So I will continue working on this :-)

gnuget’s picture

Assigned: gnuget » Unassigned
Status: Needs work » Needs review
FileSize
50.25 KB
8.74 KB

New patch added and I think this is the good one.

David.

gnuget’s picture

I forgot one file in my last patch, New patch added.

lokapujya’s picture

Issue summary: View changes
lokapujya’s picture

Issue summary: View changes
gnuget’s picture

Version: 8.0.x-dev » 8.1.x-dev

Version updated

gnuget’s picture

Version: 8.1.x-dev » 8.2.x-dev

Status: Needs review » Needs work

The last submitted patch, 4: remove_use_of-1472946-4.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review
FileSize
52.59 KB

Reroll of #8.

kiamlaluno’s picture

+++ b/core/includes/file.inc
@@ -541,8 +541,8 @@ function file_unmanaged_prepare($source, &$destination = NULL, $replace = FILE_E
+  $real_source = \Drupal::service('file_system')->realpath($source);
+  $real_destination = \Drupal::service('file_system')->realpath($destination);

Instead of using \Drupal::service('file_system'), would not it be better to use a temporary variable?

gnuget’s picture

Hi! Kiamlaluno.

Thanks for your review.

Instead of using \Drupal::service('file_system'), would not it be better to use a temporary variable?

I think is the same, check: Drupal\Component\DependencyInjection\Container::get() there you will find:

// Re-use shared service instance if it exists.
    if (isset($this->services[$id]) || ($invalid_behavior === ContainerInterface::NULL_ON_INVALID_REFERENCE && array_key_exists($id, $this->services))) {
      return $this->services[$id];
    }

Which basically check if the service has been already instanced and if it is the case then the container just re-use the service instance, so even if the patch is using multiple times \Drupal::service('file_system') it is actually just using the same instance.

kiamlaluno’s picture

Actually, Drupal core uses the following code, in BaseFieldDefinition::create().

  $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
  $default_settings = $field_type_manager->getDefaultStorageSettings($type) + $field_type_manager->getDefaultFieldSettings($type);

If it were calling \Drupal::service('plugin.manager.field.field_type') twice, it would be calling \Drupal\Component\DependencyInjection\Container::get()twice, to get a value it already has.

gnuget’s picture

Status: Needs review » Needs work
Issue tags: +Novice

To me using or not the temporary variable is the same, maybe if the service is used more than two times it would make more sense to create a variable to improve the readability of the code.

But if there is other place in the core where the same scenario happened and in that case a temporary variable was used we should keep the same style and do it in the same way.

I'm not near of my computer so I will add the novice tag in case to someone else want to help to change this.

Thanks for your review.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

valthebald’s picture

Title: Remove use of deprecated function drupal_realpath() throughout core functions » Remove usages of deprecated function drupal_realpath() throughout core functions
Issue summary: View changes
Issue tags: +deprecated, +Dublin2016

Tagging for the Friday mentored sprint

mmrares’s picture

I will try to work on this.

mmrares’s picture

Reroll of #10 applied on 8.3-dev branch.

mmrares’s picture

Status: Needs work » Needs review
FileSize
57.87 KB

There were some new usages that needed to be replaced. This is based on #18 patch.

mmrares’s picture

FileSize
5.71 KB

Added interdiff between #18 and #19 patches.

AndreaD’s picture

Assigned: Unassigned » AndreaD

WOrking on the issue.

AndreaD’s picture

Status: Needs review » Reviewed & tested by the community

I have reviewed the code, before the patch have been applied, the function appeared 50+ times.
After the patch applied there are none.

The code is reviewed.

valthebald’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/file.inc
@@ -452,8 +452,8 @@ function file_unmanaged_copy($source, $destination = NULL, $replace = FILE_EXIST
+  $real_source = \Drupal::service('file_system')->realpath($source) ?: $source;

Here I'd suggest to store \Drupal::service('file_system') result in a variable, and use that variable on the next line.

Same pattern should be applied in all places where we call \Drupal::service() twice

sidharthap’s picture

Assigned: AndreaD » Unassigned
Status: Needs work » Needs review
FileSize
58.22 KB

Thank you @valthebald.
Modified patch as per comment #23

gnuget’s picture

Hi @sidharthap

Can you please provide an interdiff?

sidharthap’s picture

FileSize
4.92 KB

Thank you @gnuget. Here is the diff file.

gnuget’s picture

Status: Needs review » Reviewed & tested by the community

This looks ready to me.

Thanks!

valthebald’s picture

+1 to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: remove_use_of-1472946-24.patch, failed testing.

dineshkumares’s picture

Status: Needs work » Needs review
FileSize
58.3 KB

Rerolling patch in #24

amit.drupal’s picture

I am not able to apply #30 patch .
Please advice.

Himanshu5050’s picture

Patch in #24 is fine and ready, resubmitting it.

Status: Needs review » Needs work

The last submitted patch, 32: remove_use_of-1472946-31.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review

The patch at #30 looks great.

I'm changing the status back to Needs Review.

Thanks!

gnuget’s picture

Status: Needs review » Reviewed & tested by the community
amit.drupal’s picture

FileSize
5.6 KB

diff in patch #30 and #32

gnuget’s picture

diff in patch #30 and #32

Is #32 working? because it seems to it doesn't apply.

I reviewed #30 as the patch ready to be committed.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Please upload the patch to be committed as the last one to this issue. It makes it easier for backports (not relevant here), but also re-tests rely on that.

sidharthap’s picture

#30 patch works. Resubmitting the patch for review.

valthebald’s picture

Status: Needs review » Needs work

Patch applies, but with offsets. Can it be rerolled against the latest 8.3.x please?

Adita’s picture

@valthebald I've applied the patch with git apply --check 1472946-39.patch and it don't need reroll.

kiamlaluno’s picture

Status: Needs work » Needs review
FileSize
58.3 KB

Let's see what the test runner thinks.

Status: Needs review » Needs work

The last submitted patch, 42: 1472946_test-again-39_42.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review
FileSize
58.3 KB

It seems to #42 has a missing new line:

 diff 1472946-39.patch 1472946_test-again-39_42.patch
1089c1089
<    }
---
>    }
\ No newline at end of file

Uploading a new patch. (I rerolled it just to be sure and it hasn't conflicts as expected)

Thanks for keeping this issue alive :-)

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

Yes, it probably was a new line

xjm’s picture

Status: Reviewed & tested by the community » Needs work

@catch, @alexpott, and I discussed this issue and had concerns with the use of $this->container being added in tests. Also see #2066993: Use the testing fixture container in tests instead of \Drupal.

We would be best off to start by using \Drupal for the initial patch, to remove the dependency on the global function and present a single scope for the issue. See https://www.drupal.org/core/scope. Then, we can have a followup meta issue to inject the service properly where appropriate. The correct fix in tests can be discussed there as needed. See #2729597: [meta] Replace \Drupal with injected services where appropriate in core.

gnuget’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
52.63 KB
7.83 KB

Thanks for your review xjm.

Patch attached removing the inyection and using \Drupal.

Also removing the tag novice.

Status: Needs review » Needs work

The last submitted patch, 47: 1472946_realpath-47.patch, failed testing.

gnuget’s picture

eh.. ignore my last patch will upload a new version. :-) I made a mistake while I was doing it.

gnuget’s picture

Status: Needs work » Needs review
FileSize
47.09 KB
52.34 KB

ok round 2, let's see what the bot says.

valthebald’s picture

@xjm I thought #2066993: Use the testing fixture container in tests instead of \Drupal suggests the opposite: to replace `\Drupal::` with `$this->container`? Or the issue summary is not updated?

gnuget’s picture

In anycase if we prefer use the fixture container we can use #44 and if we prefer to use the static method can use #50. :-)

In is a matter to decide which is the good one.

Thanks!

20th’s picture

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gnuget’s picture

Did we decide already if we are going to use the fixture container or the static method?

lokapujya’s picture

The way I read it: Use \Drupal for now. Let the meta issue replace it after.

gnuget’s picture

Stright reroll of #50. (this patch use the static \Drupal as expected).

(Big thanks to Lendude for https://www.dx-experts.nl/converting-patches-use-correct-array-notation, the script did 156 replacements!)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

andypost’s picture

Fix the rest

core8$ git grep drupal_realpath
core/includes/file.inc:1149:function drupal_realpath($uri) {

Status: Needs review » Needs work

The last submitted patch, 60: 1472946_realpath-file_system-60.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
646 bytes
70.52 KB

Fix failed test

The last submitted patch, 62: interdiff-1472946-60.patch, failed testing. View results

gnuget’s picture

Thanks for keeping this alive, to me this is RTBC!

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This does replace all the drupal_realpath calls, after applying the path, the only leftover I can find is the function declaration. There's a couple of places where this could be injected instead of using \Drupal::service(). However that might be too much to do in this issue.

catch’s picture

Status: Reviewed & tested by the community » Fixed

In #1472946-46: Remove usages of deprecated function drupal_realpath() throughout core functions we decided to keep the scope limited to a find and replace here and handle injection in follow-ups, so happy with the patch as is.

Committed/pushed to 8.5.x, thanks!

gnuget’s picture

I can't see the commit, To which branch was committed?

Thanks!

  • catch committed 700a72f on 8.5.x
    Issue #1472946 by gnuget, andypost, mmrares, sidharthap, kiamlaluno,...
gnuget’s picture

I can see it now hehe, thanks!

andypost’s picture

Status: Fixed » Closed (fixed)

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