Remove usages of deprecated function drupal_realpath() throughout core functions

Ok, This is my first try.

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 :-)

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

David.

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

Version updated

Reroll of #8.

+++ 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?

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.

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.

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.

Tagging for the Friday mentored sprint

I will try to work on this.

Reroll of #10 applied on 8.3-dev branch.

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

Added interdiff between #18 and #19 patches.

WOrking on the issue.

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.

+++ 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

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

Hi @sidharthap

Can you please provide an interdiff?

Thank you @gnuget. Here is the diff file.

This looks ready to me.

Thanks!

+1 to RTBC

Rerolling patch in #24

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

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

The patch at #30 looks great.

I'm changing the status back to Needs Review.

Thanks!

diff in patch #30 and #32

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.

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.

#30 patch works. Resubmitting the patch for review.

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

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

Let's see what the test runner thinks.

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 :-)

Yes, it probably was a new line

@catch, @alexpott, and I discussed this issue and had concerns with the use of $this->container being added in tests. Also see #2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests.

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.

Thanks for your review xjm.

Patch attached removing the inyection and using \Drupal.

Also removing the tag novice.

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

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

@xjm I thought #2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests suggests the opposite: to replace `\Drupal::` with `$this->container`? Or the issue summary is not updated?

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!

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

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

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!)

reroll

Fix the rest

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

Fix failed test

proper interdiff

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

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.

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!

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

Thanks!

I can see it now hehe, thanks!

Filed follow-up #2923511: Use proper DI for FileSystem service