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
Comment | File | Size | Author |
---|---|---|---|
#42 | replace_uses_of-2351919-42.patch | 11.27 KB | almaudoh |
#42 | interdiff.txt | 3.32 KB | almaudoh |
#29 | replace_uses_of-2351919-29.patch | 14.59 KB | almaudoh |
#29 | interdiff.txt | 2.3 KB | almaudoh |
#27 | interdiff.txt | 7.32 KB | almaudoh |
Comments
Comment #1
fietserwinComment #2
catchPostponing #1308152: Add stream wrappers to access extension files on this.
Comment #3
almaudoh CreditAttribution: almaudoh commentedComment #4
almaudoh CreditAttribution: almaudoh commentedFirst patch with replacements in Tests that read from the filesystem. NR for testbot.
Comment #6
almaudoh CreditAttribution: almaudoh commentedPatch was rebased to 8.1.x
Comment #9
claudiu.cristeaThese cannot be replaced. In fact
__DIR__
is not the same asdrupal_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.Comment #11
almaudoh CreditAttribution: almaudoh commentedThis I discovered too. A solution I have thought about is a helper method like
determineRelativeFilePath()
orgetRelativeFilePath()
inWebTestBase
that would turn__DIR__
into a drupal-root-relative path:Then in webtests we can call
$this->getRelativeFilePath(__DIR__)
or$this->getRelativeFilePath(realpath(__DIR__))
Comment #12
claudiu.cristea@almaudoh, agree with the idea but...
Well, it shouldn't be in WebTestBase but in Drupal/Component somewhere because this can be used in many places.
Comment #13
claudiu.cristeaWhile reviewing this I found this quick fix bug #2655580: Dead code: hook_system_theme_info() removed but is still implemented.
Comment #14
Wim LeersAssigning to catch for feedback.
Comment #15
BerdirI 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:
We could support that syntax natively, which we already do in many other places where we use callbacks.
Comment #16
almaudoh CreditAttribution: almaudoh commented+1. That makes a lot of sense.
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)?
Comment #17
BerdirNo, 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.
Comment #18
almaudoh CreditAttribution: almaudoh commented#17: Ok, understood now. So that needs a new issue.
Comment #19
Wim LeersBerdir++
Comment #20
almaudoh CreditAttribution: almaudoh commentedCreated a new issue #2656766: Add support to batch to call service methods for #17.
Comment #21
Wim LeersDoing this would also allow us to close #2442383: Add the option to cache drupal_get_filename() most likely.
Comment #22
almaudoh CreditAttribution: almaudoh commentedReverted the batch stuff andfixed the test fail inNodeImportCreateTest
. Still more to come...Edit: Some of the batch stuff had already been reverted in #9, I just wasn't paying attention :\
Comment #24
almaudoh CreditAttribution: almaudoh commentedSorry, wrong patch :(
Interdiff is the same.Comment #25
almaudoh CreditAttribution: almaudoh commentedHere's the *correct* interdiff...
Comment #26
dawehnerWent 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.
Comment #27
almaudoh CreditAttribution: almaudoh commentedStill found some more, let's see if these ones pass...
Comment #29
almaudoh CreditAttribution: almaudoh commentedReverted the test fails.
Comment #30
Wim LeersI 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
.Comment #31
tstoecklerRe #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.
Comment #32
Wim Leers:O :O :O WHAT!? Really?
I've never heard about that before.
In that case, you're absolutely right of course.
Comment #33
fietserwinThe 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:
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:
core/modules/color/preview.html?
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).
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]
Comment #34
Wim Leers#33 You reviewed my patch in #30, but it's wrong, as #31 showed. Please review #29 instead.
Comment #35
tstoecklerYeah, 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.
Comment #36
almaudoh CreditAttribution: almaudoh commentedUpdated the IS, patch to review is #29 per #31 and #32
Comment #37
fietserwin- 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.
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).
idem.
idem.
idem.
Comment #38
almaudoh CreditAttribution: almaudoh commentedThis 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.Comment #39
fietserwinre #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).
Comment #40
almaudoh CreditAttribution: almaudoh commented@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...
Comment #41
fietserwinThere 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).
Comment #42
almaudoh CreditAttribution: almaudoh commentedEdit: 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
Comment #43
Wim LeersBack to RTBC per #26.
This is great progress.
Comment #44
catchComment #46
catchCommitted/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.
Comment #47
catchWhile 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.
Comment #48
Wim LeersShould that CR also recommend ExtensionList::getPath()? (See #2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname())
Comment #49
Wim LeersAlso unpostponed #1308152: Add stream wrappers to access extension files now.
Comment #50
catchIt should once it exists yeah.
Comment #51
almaudoh CreditAttribution: almaudoh commentedAdjusted title to match actual scope.
Comment #52
Wim Leers#48+#50: I said
ExtensionList::getPath()
because that issue says it, but it's actuallyExtension::getPath()
, which already exists.Comment #53
almaudoh CreditAttribution: almaudoh commented@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 todrupal_get_path()
because you only need to know the extension name.Comment #54
Wim LeersI see, thanks!
Comment #55
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
Comment #60
claudiu.cristeaI know this is old but we need a Change Notice as somehow creates a policy.
Comment #61
claudiu.cristeaCreated and published https://www.drupal.org/node/3034299