Problem/Motivation

Coming from #1308152-202: Add stream wrappers to access extension files there are many uses of drupal_get_path() in where the simple use of __DIR__ would have been sufficient and more performant.

Proposed resolution

Replace occurrences of drupal_get_path() with __DIR__ wherever the targeted file is known to share the same module directory with the calling code (and whose relative position will not change dynamically), and thus can be hard-coded.

Remaining tasks

Patch
Reviews (patch to review is #29)
Commit

User interface changes

None

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fietserwin’s picture

Issue summary: View changes
catch’s picture

Title: Replace uses of drupal_get_path() with __DIR__ where possible » Replace uses of drupal_get_path() with __DIR__ or Extension::getPath() where possible
almaudoh’s picture

Issue summary: View changes
almaudoh’s picture

First patch with replacements in Tests that read from the filesystem. NR for testbot.

Status: Needs review » Needs work

The last submitted patch, 4: replace_uses_of-2351919-4.patch, failed testing.

almaudoh’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review

Patch was rebased to 8.1.x

Status: Needs review » Needs work

The last submitted patch, 4: replace_uses_of-2351919-4.patch, failed testing.

The last submitted patch, 4: replace_uses_of-2351919-4.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
10.18 KB
4.55 KB
+++ b/core/modules/filter/src/Tests/FilterUnitTest.php
@@ -933,7 +933,7 @@ function testUrlFilterContent() {
     $input = file_get_contents($path . '/filter.url-input.txt');

+++ b/core/modules/locale/locale.bulk.inc
@@ -154,7 +154,7 @@ function locale_translate_batch_build(array $files, array $options) {
-      'file'          => drupal_get_path('module', 'locale') . '/locale.bulk.inc',
+      'file'          => __DIR__ . '/locale.bulk.inc',

@@ -580,7 +580,7 @@ function locale_config_batch_build(array $names, array $langcodes, array $option
-    'file'          => drupal_get_path('module', 'locale') . '/locale.bulk.inc',
+    'file'          => __DIR__ . '/locale.bulk.inc',

+++ b/core/modules/locale/locale.compare.inc
@@ -245,7 +245,7 @@ function locale_translation_batch_status_build($projects = array(), $langcodes =
-    'file' => drupal_get_path('module', 'locale') . '/locale.batch.inc',
+    'file' => __DIR__ . '/locale.batch.inc',

+++ b/core/modules/locale/locale.fetch.inc
@@ -44,7 +44,7 @@ function locale_translation_batch_update_build($projects = array(), $langcodes =
-    'file' => drupal_get_path('module', 'locale') . '/locale.batch.inc',
+    'file' => __DIR__ . '/locale.batch.inc',

@@ -73,7 +73,7 @@ function locale_translation_batch_fetch_build($projects = array(), $langcodes =
-    'file' => drupal_get_path('module', 'locale') . '/locale.batch.inc',
+    'file' => __DIR__ . '/locale.batch.inc',

These cannot be replaced. In fact __DIR__ is not the same as drupal_get_path(). The last returns a relative (system) path (e.g. core/modules/node) while __DIR__ a full qualified path (e.g. /var/www/html/core/modules/node). In this case, the batch API requires the relative path.

Status: Needs review » Needs work

The last submitted patch, 9: 2351919-8.patch, failed testing.

almaudoh’s picture

In fact __DIR__ is not the same as drupal_get_path(). The last returns a relative (system) path (e.g. core/modules/node) while __DIR__ a full qualified path (e.g. /var/www/html/core/modules/node)

This I discovered too. A solution I have thought about is a helper method like determineRelativeFilePath() or getRelativeFilePath() in WebTestBase that would turn __DIR__ into a drupal-root-relative path:


function getRelativeFilePath($path, $root_path = DRUPAL_ROOT) {
  if (strpos($path, $root_path) === 0) {
    return substr($path, strlen($root_path));
  }
  return $path;
}

Then in webtests we can call $this->getRelativeFilePath(__DIR__) or $this->getRelativeFilePath(realpath(__DIR__))

claudiu.cristea’s picture

@almaudoh, agree with the idea but...

This I discovered too. A solution I have thought about is a helper method like determineRelativeFilePath() or getRelativeFilePath() in WebTestBase that would turn __DIR__ into a drupal-root-relative path:

Well, it shouldn't be in WebTestBase but in Drupal/Component somewhere because this can be used in many places.

claudiu.cristea’s picture

Wim Leers’s picture

Assigned: Unassigned » catch

Assigning to catch for feedback.

Berdir’s picture

I think adding more code to deal with include files seems somewhat backwards.

For batch specifically, I think it would be great if we'd introduce support for calling a service method (we could also already now use static methods I think) directly. Then we could just move that code to a service/class and call it there.

For simplenews, I added a simple helper function that does that for me:

/**
 * Batch callback to dispatch batch operations to a service.
 */
function _simplenews_batch_dispatcher() {
  $args = func_get_args();
  list($service, $method) = explode(':', array_shift($args));
  call_user_func_array(array(\Drupal::service($service), $method), $args);
}

We could support that syntax natively, which we already do in many other places where we use callbacks.

almaudoh’s picture

Well, it shouldn't be in WebTestBase but in Drupal/Component somewhere because this can be used in many places.

+1. That makes a lot of sense.

I think adding more code to deal with include files seems somewhat backwards.

I don't really understand the comment? Are you referring to #11/ #12? Are you suggesting we should add a service to convert absolute paths to relative paths (and such similar functionality)?

Berdir’s picture

No, I'm not suggesting to convert paths. I'm suggesting to add support to batch to call service methods. And convert the batch code examples above to either move it to static methods (which already works) or even better, a service. We can easily do that in 8.1.x as long a we leave BC functions that call the new code.

Then the need to call drupal_get_path() simply vanishes, as class loading takes care of it.

almaudoh’s picture

#17: Ok, understood now. So that needs a new issue.

Wim Leers’s picture

Assigned: catch » Unassigned

Berdir++

almaudoh’s picture

Wim Leers’s picture

Doing this would also allow us to close #2442383: Add the option to cache drupal_get_filename() most likely.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
8.74 KB

Reverted the batch stuff and fixed the test fail in NodeImportCreateTest. Still more to come...

Edit: Some of the batch stuff had already been reverted in #9, I just wasn't paying attention :\

Status: Needs review » Needs work

The last submitted patch, 22: replace_uses_of-2351919-22.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
9.57 KB

Sorry, wrong patch :( Interdiff is the same.

almaudoh’s picture

FileSize
1.43 KB

Here's the *correct* interdiff...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Went through the patch and these instances are fine.

I couldn't fine an instance which could be replaced in a similar way as this patch does so far.

almaudoh’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.32 KB
16.89 KB

Still found some more, let's see if these ones pass...

Status: Needs review » Needs work

The last submitted patch, 27: replace_uses_of-2351919-27.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
2.3 KB
14.59 KB

Reverted the test fails.

Wim Leers’s picture

I think we can do much more? Any place that expects Drupal root-relative URLs and where we are referencing a core module is always going to have the same relative path: core/modules/<MODULE>/path/to/file.

tstoeckler’s picture

Status: Needs review » Needs work

Re #30: Drupal supports overriding core modules from /modules or /sites/foo.com/modules. I'm not advocating that that is a good feature, but it is a feature that we support and always have supported so we shouldn't break it here.

We can of course rip that support out, but that's D9 material at this point.

Wim Leers’s picture

Drupal supports overriding core modules from /modules or /sites/foo.com/modules

:O :O :O WHAT!? Really?

I've never heard about that before.

In that case, you're absolutely right of course.

fietserwin’s picture

The IS states "... to replace drupal_get_path() with __DIR__ wherever the targeted file is known to be within the same directory (or a subdirectory) ...".

Going through the replacements, I see:

  • Hardcoded paths starting at the Drupal root (core/modules/...). Do we want to support the case where someone wants to play with a core module and starts by moving it to another location?
  • Relative paths going up in the directory structure (__DIR__ . '/../../), mainly:
    • from the src dir to the module dir (and back down, mainly from {module}/src/Tests to '{module}/tests'):
    • from submodules (mainly test sub modules) to their parent module

Is this what we really want? Tying modules and sub modules to a fixed location and sacrificing readability for minimal performance gains in tests? I'm not really for or against this, but we should align the IS and the patch.

and then some really awkward changes (separate issues?) and 1 bug:

  1. +++ b/core/modules/color/color.module
    @@ -299,7 +299,7 @@ function template_preprocess_color_scheme_form(&$variables) {
    -  $preview_html_path = \Drupal::root() . '/' . (isset($info['preview_html']) ? drupal_get_path('theme', $theme) . '/' . $info['preview_html'] : drupal_get_path('module', 'color') . '/preview.html');
    +  $preview_html_path = \Drupal::root() . '/' . (isset($info['preview_html']) ? drupal_get_path('theme', $theme) . '/' . $info['preview_html'] : 'core/modules/core/preview.html');
       $variables['html_preview']['#markup'] = file_get_contents($preview_html_path);
    

    core/modules/color/preview.html?

  2. +++ b/core/modules/system/src/Tests/Image/ToolkitGdTest.php
    @@ -259,7 +259,7 @@ function testManipulations() {
    -        $image = $this->imageFactory->get(drupal_get_path('module', 'simpletest') . '/files/' . $file);
    +        $image = $this->imageFactory->get('core/modules/simpletest/files/' . $file);
             $toolkit = $image->getToolkit();
    

    Isn't it about time that the Image module takes over these images, so it doesn't depend on simpletest any longer (6 occurrences in Image module).

  3. +++ b/core/modules/update/update.authorize.inc
    @@ -53,7 +53,7 @@ function update_authorize_run_update($filetransfer, $projects) {
    -    'file' => drupal_get_path('module', 'update') . '/update.authorize.inc',
    +    'file' => 'core/modules/update/update.authorize.inc',
       );
    

    Pointing to ourself via a "harcoded" absolute path? (2 occurrences). Can't we do better?

[EDIT: cross posted: 1 of my questions has been answered: apparently we do support that use case]

Wim Leers’s picture

#33 You reviewed my patch in #30, but it's wrong, as #31 showed. Please review #29 instead.

tstoeckler’s picture

:O :O :O WHAT!? Really?

I've never heard about that before.

Yeah, again, I don't want to be quoted saying that that's actually a good idea, but this has been possible since at least Drupal 6. Before that I have no clue. The only time I've ever seen that in use in the first version of the Profile2 module in D7 which actually was called just profile and was supposed to be a drop-in replacement for the core profile module. I remember something similar with content_translation in D7.

almaudoh’s picture

Issue summary: View changes
Status: Needs work » Needs review

Updated the IS, patch to review is #29 per #31 and #32

fietserwin’s picture

- I did not review for completeness. Given #26 and #27, there won't be many, if any at all.
- The points below are based on whether the old solution using drupal_get_path() would remain working correctly on moving a sub module, whereas our changes won't work correctly anymore.

  1. +++ b/core/modules/config/src/Tests/ConfigCRUDTest.php
    @@ -248,7 +248,7 @@ public function testDataTypes() {
    -    $original_content = file_get_contents(drupal_get_path('module', 'config_test') . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY . "/$name.yml");
    +    $original_content = file_get_contents(__DIR__ . '/../../tests/config_test/' . InstallStorage::CONFIG_INSTALL_DIRECTORY . "/$name.yml");
         $this->verbose('<pre>' . $original_content . "\n" . var_export($storage->read($name), TRUE));
    

    This goes via the "parent" module. if we move the test modules from tests to tests/modules - as in field and node below - , drupal_get_path() would not fail, whereas this solution will (2 occurrences in config).

  2. +++ b/core/modules/field/src/Tests/FieldImportCreateTest.php
    @@ -97,7 +97,7 @@ function testImportCreate() {
    -    $src_dir = drupal_get_path('module', 'field_test_config') . '/sync';
    +    $src_dir = __DIR__ . '/../../tests/modules/field_test_config/sync';
         $target_dir = $this->configDirectories[CONFIG_SYNC_DIRECTORY];
    

    idem.

  3. +++ b/core/modules/node/src/Tests/Config/NodeImportCreateTest.php
    @@ -64,7 +64,7 @@ public function testImportCreate() {
    -    $src_dir = drupal_get_path('module', 'node_test_config') . '/sync';
    +    $src_dir = __DIR__ . '/../../../tests/modules/node_test_config/sync';
         $target_dir = $this->configDirectories[CONFIG_SYNC_DIRECTORY];
    

    idem.

  4. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -265,7 +265,7 @@ function testClassLoading() {
    -    $templates = drupal_find_theme_templates($registry, '.html.twig', drupal_get_path('theme', 'test_theme'));
    +    $templates = drupal_find_theme_templates($registry, '.html.twig', __DIR__ . '/../../../tests/themes/test_theme');
         $this->assertEqual($templates['node__1']['template'], 'node--1', 'Template node--1.tpl.twig was found in test_theme.');
    

    idem.

almaudoh’s picture

if we move the test modules from tests to tests/modules - as in field and node below - , drupal_get_path() would not fail, whereas this solution will (2 occurrences in config).

This argument may well apply for any of the changes in this patch. If someone were to move the location of a file, they would need to adjust the path in code. Similarly, if someone were to move the location of a test module, they would need to adjust the path in the test. These changes apply mostly to tests. I don't see any use case where someone who's not a developer would want to move the location of a test module. If they did, they might as well adjust the path in the code as well.

I see the real use of drupal_get_path() for determining dynamically the paths where the module name is unknown, or perhaps known but cannot be controlled by the consuming code. Any other usage was basically for convenience because the function was available - and most of these are in tests.

fietserwin’s picture

re #38: No, as I wrote above, I only listed those replacements where moving a submodule would fail with the new code but would not cause any problems in the old case using drupal_get_path() passing in the sub module.

Thus yes, I agree that renaming or moving files within a module will lead to the need to update the calling code, but as a complete (sub)module is moved, I don't want a need for calling code to be changed (that's why #30 was reverted).

almaudoh’s picture

@fietserwin: I agree with the point of modules or sub-modules. But in the case where these sub-modules are for tests only (all the cases you mentioned), I still do not see the use case of moving a test module / sub-module to another location. If there is, then I'll update the patch...

fietserwin’s picture

There will be when we standardize where to put test sub modules: tests or tests/modules :). And,to me, it is still against the updated IS (but this is open to one's own interpretation).

almaudoh’s picture

Edit: somehow the original comment that came with this patch was lost in transit... I've reverted the changes per #37. Since we have a replacement for drupal_get_path() in the offing at #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList.

This should be RTBC again

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC per #26.

This is great progress.

catch’s picture

Component: file system » extension system

  • catch committed 18d44b5 on 8.1.x
    Issue #2351919 by almaudoh, Wim Leers, claudiu.cristea: Replace uses of...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

The 'replace an entire core module from sites/all' capability has been around for a long time - it's just because we look for modules in /sites/SITENAME /sites/all /modules in that order. This is what also allows a multisite install to potentially run two different versions of the same contrib module if they needed to.

It's a completely undocumented feature, and not one I'm sure we need to support even between minor releases, but agreed on not breaking it in this patch at least - needs its own discussion.

catch’s picture

While we're not deprecating drupal_get_path() yet, it might be worth a change record discouraging its use, and/or a follow-up issue documenting on drupal_get_path() the cases it's actually supposed to be used for or not. I couldn't find any mentions at https://www.drupal.org/list-changes/drupal/published?keywords_descriptio... even though we've been using it less and less for years now.

Wim Leers’s picture

catch’s picture

It should once it exists yeah.

almaudoh’s picture

Title: Replace uses of drupal_get_path() with __DIR__ or Extension::getPath() where possible » Replace uses of drupal_get_path() with __DIR__ where possible

Adjusted title to match actual scope.

Wim Leers’s picture

#48+#50: I said ExtensionList::getPath() because that issue says it, but it's actually Extension::getPath(), which already exists.

almaudoh’s picture

@wimleers Extension::getPath() already exists but you have to get hold of the extension object first (or instantiate one, which requires knowing the path). ExtensionList::getPath() doesn't yet exist but is more akin to drupal_get_path() because you only need to know the extension name.

Wim Leers’s picture

I see, thanks!

David_Rothstein’s picture

Version: 8.1.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D7

This could be considered for Drupal 7 backport (though with dirname(__FILE__) rather than __DIR__ for PHP version compatibility reasons).

I think it would mainly just be a micro-optimization that avoids some extra function calls, since drupal_get_path() itself should already be incredibly fast (see https://www.drupal.org/node/2442383#comment-10827200 for why) so I don't know if it's worth doing, but maybe it still is.

  • catch committed 18d44b5 on 8.3.x
    Issue #2351919 by almaudoh, Wim Leers, claudiu.cristea: Replace uses of...

  • catch committed 18d44b5 on 8.3.x
    Issue #2351919 by almaudoh, Wim Leers, claudiu.cristea: Replace uses of...

  • catch committed 18d44b5 on 8.4.x
    Issue #2351919 by almaudoh, Wim Leers, claudiu.cristea: Replace uses of...

  • catch committed 18d44b5 on 8.4.x
    Issue #2351919 by almaudoh, Wim Leers, claudiu.cristea: Replace uses of...
claudiu.cristea’s picture

I know this is old but we need a Change Notice as somehow creates a policy.

claudiu.cristea’s picture

Issue tags: -Needs change record