Problem/Motivation

Starting from #2351919: Replace uses of drupal_get_path() with __DIR__ where possible, when the PHP code needs to include or parse files inside the module or theme directory space, will simply use _DIR_ instead of drupal_get_path(). This is sufficient, intuitive and more performant than calling drupal_get_path().

However, drupal_get_path() is widely used to refer files (.js, .css, images, assets) outside a module or theme. This isn't very intuitive for new developers. Stream wrappers make much more sense and simplify the API for developers trying to refer directories or files from outside the current extension in code.

Proposed resolution

Introduce extension stream wrappers:

Scheme Description
profile:// Points to the installed profile root directory.
module://{name} Points to the module {name} root directory. Only enabled modules can be referred.
theme://{name} Points to the theme {name} root directory. Only installed themes can be referred.

Examples

Profile: profile://

Assuming the standard profile is installed:

  • Referring a directory:
    • Actual: drupal_get_path('profile', 'standard') . '/config'
    • Proposed: profile://config
    • Path: core/profiles/standard/config
  • Referring a file:
    • Actual: drupal_get_path('profile', 'standard') . '/config/install/automated_cron.settings.yml'
    • Proposed: profile://config/install/automated_cron.settings.yml
    • Path: core/profiles/standard/config/install/automated_cron.settings.yml

Module: module://

Assuming the node module is enabled but color module is not:

  • Referring a directory:
    • Actual: drupal_get_path('module', 'node') . '/config'
    • Proposed: module://node/config
    • Path: core/modules/node/config
  • Referring a file:
    • Actual: drupal_get_path('module', 'node') . '/config/install/node.settings.yml'
    • Proposed: module://node/config/install/node.settings.yml
    • Path: core/modules/node/config/install/node.settings.yml
  • Referring a resource in a uninstalled or inexistent module:
    • Actual: drupal_get_path('module', 'color') . '/config'
    • Proposed: module://color/config
    • Path: Throws \RuntimeException

Theme: theme://

Assuming the bartik theme is installed but seven theme is not:

  • Referring a directory
    • Actual: drupal_get_path('theme', 'bartik') . '/config'
    • Proposed: theme://bartik/config
    • Path: core/themes/bartik/config
  • Referring a file
    • Actual: drupal_get_path('theme', 'bartik') . '/color/color.inc'
    • Proposed: theme://bartik/color/color.inc
    • Path: core/themes/bartik/color/color.inc
  • Referring a resource in a uninstalled or inexistent theme:
    • Actual: drupal_get_path('theme', 'seven') . '/config'
    • Proposed: theme://seven/config
    • Path: Throws \RuntimeException

Remaining tasks

  1. Profiling to compare performance of the new code with the old.

API changes

New abstract class for extensions stream wrappers:

  • \Drupal\Core\StreamWrapper\ExtensionStreamBase

New stream wrapper classes:

  • \Drupal\Core\StreamWrapper\ModuleStream
  • \Drupal\Core\StreamWrapper\ThemeStream
  • \Drupal\Core\StreamWrapper\ProfileStream

Data model changes

None.

CommentFileSizeAuthor
#395 1308152-395.patch19.64 KBaspilicious
#361 interdiff_358-361.txt7.02 KBadamzimmermann
#361 1308152-361.patch30.57 KBadamzimmermann
#358 interdiff_356-358.txt652 bytesraman.b
#358 1308152-358.patch28.48 KBraman.b
#358 1308152-356-reroll.patch28.48 KBraman.b
#356 interdiff_352-356.txt3.57 KBmdupont
#356 1308152-356.patch28.59 KBmdupont
#352 interdiff_350-352.txt624 bytesnarendra.rajwar27
#352 1308152-352.patch28.57 KBnarendra.rajwar27
#350 interdiff-348-350.txt407 bytesHardik_Patel_12
#350 1308152-350.patch28.6 KBHardik_Patel_12
#348 interdiff-340-348.txt2.61 KBmdupont
#348 1308152-348.patch28.59 KBmdupont
#340 interdiff-337-340.txt2.77 KBhanoii
#340 1308152-n340.patch28.61 KBhanoii
#337 1308152-add-stream-wrappers-to-access-extension-files-337.patch28.83 KBzipymonkey
#332 1308152-327.patch29.57 KBizus
#330 1308152-330.patch29.48 KBaspilicious
#328 1308152-328.patch8.78 KBaspilicious
#324 1308152-324.patch29.62 KBborisson_
#324 interdiff-1308152-323-324.txt2.85 KBborisson_
#323 1308152-323.patch29.48 KBaspilicious
#319 1308152-319.patch29.62 KBvijaycs85
#319 1308152-diff-316-319.txt506 bytesvijaycs85
#316 1308152-315.patch29.47 KBpwolanin
#313 1308152-313.patch30.06 KBclaudiu.cristea
#313 interdiff.txt1.61 KBclaudiu.cristea
#308 interdiff.txt1.39 KBclaudiu.cristea
#308 1308152-308.patch29.67 KBclaudiu.cristea
#304 interdiff.txt675 bytesclaudiu.cristea
#302 interdiff.txt3.53 KBclaudiu.cristea
#302 1308152-302.patch29.77 KBclaudiu.cristea
#297 interdiff.txt3.27 KBclaudiu.cristea
#297 1308152-297.patch29.91 KBclaudiu.cristea
#293 interdiff.txt2.85 KBclaudiu.cristea
#293 1308152-293.patch31.46 KBclaudiu.cristea
#286 interdiff.txt10.47 KBdeviantintegral
#286 add_stream_wrappers_to-1308152-286.patch31.01 KBdeviantintegral
#281 1308152-281-interdiff.txt10.8 KBdeviantintegral
#281 add_stream_wrappers_to-1308152-281.patch38.36 KBdeviantintegral
#280 add_stream_wrappers_to-1308152-280.patch36.01 KBjuampynr
#280 interdiff.txt3.89 KBjuampynr
#279 add_stream_wrappers_to-1308152-279.patch36.71 KBjuampynr
#279 interdiff.txt6.1 KBjuampynr
#275 Selection_004.png225.46 KBjuampynr
#272 1308152-270-interdiff.txt1.58 KBdeviantintegral
#272 1308152-272.patch31.01 KBdeviantintegral
#270 interdiff.txt6.41 KBclaudiu.cristea
#270 1308152-270.patch30.59 KBclaudiu.cristea
#264 add_stream_wrappers_to-1308152-264.patch27.28 KBalvar0hurtad0
#259 1308152-259.patch27.93 KBalmaudoh
#259 interdiff.txt3.12 KBalmaudoh
#258 interdiff.txt3.15 KBclaudiu.cristea
#258 1308152-258.patch28.02 KBclaudiu.cristea
#256 interdiff.txt20.1 KBclaudiu.cristea
#256 1308152-256.patch28.09 KBclaudiu.cristea
#250 interdiff.txt52.45 KBclaudiu.cristea
#250 1308152-250.patch28.3 KBclaudiu.cristea
#243 interdiff.txt4.02 KBclaudiu.cristea
#243 1308152-243.patch38.32 KBclaudiu.cristea
#240 interdiff.txt2.27 KBclaudiu.cristea
#240 1308152-240.patch38.41 KBclaudiu.cristea
#228 interdiff.txt27.05 KBclaudiu.cristea
#228 1308152-228.patch40.03 KBclaudiu.cristea
#227 interdiff.txt33.91 KBclaudiu.cristea
#227 1308152-227.patch30.76 KBclaudiu.cristea
#225 add_module_theme-1308152-225-interdiff.txt19.04 KBmbovan
#225 add_module_theme-1308152-225.patch36.34 KBmbovan
#222 add_module_theme-1308152-222.patch35.05 KBmgifford
#218 add_module_theme-1308152-218.patch35.25 KBalmaudoh
#205 interdiff.txt2.16 KBalmaudoh
#205 add_module_theme-1308152-205.patch32.47 KBalmaudoh
#193 interdiff.txt1.27 KBgarphy
#193 add_module_theme-1308152-193.patch32.4 KBgarphy
#190 add_module_theme-1308152-190.patch32.23 KBJeroenT
#190 interdiff.txt1.56 KBJeroenT
#186 interdiff.txt8.46 KBalmaudoh
#186 add_module_theme-1308152-186.patch32.01 KBalmaudoh
#185 interdiff.txt1.94 KBfietserwin
#6 1308152-add-module-theme-profile-stream-wrappers.patch9.67 KBbrianV
#8 1308152-add-module-theme-profile-stream-wrappers-rev2.patch9.71 KBbrianV
#28 1308152-add-module-theme-profile-stream-wrappers-rev3.patch12.9 KBbrianV
#43 1308152-add-module-theme-profile-stream-wrappers-rev4.patch11.77 KBbrianV
#50 1308152-add-module-theme-profile-stream-wrappers-rev5.patch8.17 KBjthorson
#59 1308152-add-module-theme-profile-stream-wrappers-rev6.patch8.18 KBbrianV
#61 1308152-add-module-theme-profile-stream-wrappers-rev7.patch9.18 KBbrianV
#68 drupal-wrappers-1308152-68.patch9.19 KBParisLiakos
#71 drupal-wrappers-1308152-71.patch9.76 KBParisLiakos
#80 interdiff.txt2.58 KBParisLiakos
#80 drupal-wrappers-1308152-80.patch9.91 KBParisLiakos
#82 interdiff.txt5.54 KBjthorson
#82 drupal-wrappers-1308152-82.patch15.45 KBjthorson
#84 drupal-wrappers-1308152-83.patch15.04 KBjthorson
#85 drupal-wrappers-1308152-85.patch15.04 KBjthorson
#86 interdiff.txt5.13 KBjthorson
#95 interdiff-85-95.txt32.81 KBPancho
#95 drupal-wrappers-1308152-95.patch31.62 KBPancho
#131 drupal8.system_stream_wrappers.1308152-131.patch31.33 KBdisasm
#134 interdiff.txt1.14 KBdisasm
#134 drupal8.system_stream_wrappers.1308152-134.patch30.2 KBdisasm
#139 1308152-139-system-streamwrappers.patch28.71 KBalmaudoh
#143 1308152-143-system-streamwrappers.patch32.44 KBalmaudoh
#148 add_module_theme-1308152-148.patch33.41 KBArla
#148 add_module_theme-1308152-148.interdiff.txt15.78 KBArla
#153 add_module_theme-1308152-153.patch36.22 KBalmaudoh
#153 interdiff.txt22.34 KBalmaudoh
#158 add_module_theme-1308152-158.patch35.57 KBalmaudoh
#158 interdiff.txt3.65 KBalmaudoh
#161 1308152-159.patch35.66 KBpwolanin
#161 increment.txt728 bytespwolanin
#162 add_module_theme-1308152-162.patch36.45 KBalmaudoh
#162 interdiff-161.txt8.33 KBalmaudoh
#164 add_module_theme-1308152-164.patch38.33 KBalmaudoh
#164 interdiff.txt44.09 KBalmaudoh
#166 add_module_theme-1308152-166.patch38.29 KBArla
#166 add_module_theme-1308152-166.interdiff.txt4.69 KBArla
#170 add_module_theme-1308152-170.patch38.46 KBArla
#170 add_module_theme-1308152-170.interdiff.txt2.75 KBArla
#176 interdiff.txt38.98 KBalmaudoh
#176 add_module_theme-1308152-176.patch30.38 KBalmaudoh

Issue fork drupal-1308152

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Also related: #1185360: Refactor code loading and support loading from PHAR archives which completely replaces drupal_get_path() with a stream wrapper which I'm not sold on and also merges all the system types into one stream wrapper compared to here where it was easy to define individual stream wrappers.

Gábor Hojtsy’s picture

I think this would be a great improvement, lots of cleanup possibilities. Would it make sense to do the same for includes for consistency (even if practically, it would take more chars to type it this way)?

Dave Reid’s picture

I'm hesitant about includes since they live in a consistent directory. The usefulness here is that module and themes can vary in location and this is less for including code but for interacting with files in the directories (say for instance you wanted to imagecache the default user picture in Drupal 8's user.module directory).

Crell’s picture

Let's go ahead and add module, profile, and theme, as those are easy and obvious. We can debate includes in another thread.

Given that much of /includes is likely to go OO anyway, I think it becomes a moot point.

brianV’s picture

+1 to this. I also think that library:// would be very helpful.

brianV’s picture

Status: Active » Needs review
FileSize
9.67 KB

Attached is a patch that implements these stream wrappers.

One of the classes in this patch is LocalReadOnlyStream, which is also in my patch in #4 from #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend. If that issue gets committed, I will reroll this patch to remove the class.

brianV’s picture

Status: Needs work » Needs review
FileSize
9.71 KB

Updated patch, should pass testing.

Dave Reid’s picture

Ooh, we should probably add theme://default and theme://admin support for the default and admin themes respectively.

Dave Reid’s picture

And theme://current

betz’s picture

Ow yes, brilliant!

chx’s picture

Status: Needs review » Needs work

This looks good indeed but there is not test, no usage, no nothing. I am wondering whether you could just change drupal_get_filename or drupal_load as an interim step and immediately provide ton of testing by that? (Later we can remove either or both of those)

catch’s picture

drupal_get_filename() can't use this as the patch stands, because the stream wrappers are calling drupal_get_path(), which in turn calls drupal_get_filename(). Same with drupal_load().

If we wanted to look at refactoring those functions, then I think we need to decide whether the logic in drupal_get_filename() itself should move into the stream wrapper (or some other class). I don't really see the point of adding a stream wrapper here, unless it's part of a wider refactoring of this system (and if we refactored it nicely, we'd likely have less need to call these functions all over the place anyway).

Anonymous’s picture

not sure why we want this. like, at all. are people planning to extend this in some way? to read module etc files from some remote system?

this makes the code around file loading slower and more complex. i'm all for some simplification / improvement of calls to get a filename, but this just looks like overengineering.

dman’s picture

This is so sexy.
Will clear out all those drupal_get_path() clunky calls.

Dave Reid’s picture

@beejeebus: With other improvements to stream wrappers we can be able to easily generate image style derivatives of any images in modules, themes, or profiles. If the users wants to enter a path to an image or file in the UI that's contained in a module, they have to know where it is located. By providing stream wrappers we're giving a unified way to access those files, just like the public and private file system.

stevector’s picture

I just closed a duplicate: #908874: Provide module://, theme://, drupal:// schemes for files That issue also suggested libraries://

xjm’s picture

Issue tags: +VDC

Tagging.

Edit: The reason this is tagged is that this and #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend would also be useful for plugin DX (plugin://).

saltednut’s picture

Just noting #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend is also creating LocalReadOnlyStream and this patch might need to leave out that bit if it is committed.

Robin Millette’s picture

chx’s picture

Sorry to rain the parade but during the discussion of #1668892-7: Enable secure compiled information on disk it was found you can't APC cache anything with a streamwrapper unless you have the very latest APC (as in 3.1.11 beta released a week ago) and apc.stat=0. I am deeply afraid that kills this issue.

pounard’s picture

Agree with chx here, while this is patch that could prove itself to be useful, I don't want to see PHP includes going throught it right now. Everything else should be OK IMHO.

Dave Reid’s picture

Issue tags: +sprint, +Media Initiative
brianV’s picture

Bumping. While I agree w.r.t PHP includes, this could be very valuable for static assets (images, js, css etc.) provided by modules, themes and profiles.

Maybe something like this should be committed, but with documentation that specifies *not* to use it to include PHP through it?

chx’s picture

That might work. Given that the wrappers are provided by modules unless we want another circular dependency hell there's no way to include modules via it, anyways.

Crell’s picture

That makes sense to me. It still simplifies the API for people trying to refer to JS, CSS, etc. files.

brianV’s picture

Status: Needs work » Needs review

Attached is a new version of this patch.

Changes from previous:

1. LocalStream::getLocalPath() now rejects any .module, .theme, .engine., .inc., .install or .php file as not valid. This should prevent these files from being pulled via a Drupal stream wrapper at all.

2. Changed two appropriate drupal_get_path() entries to use it as a proof of concept.

We probably still need tests, but is this approach (filtering in LocalStream::getLocalPath()) correct?

brianV’s picture

Forgot to attach patch.

Lars Toomre’s picture

Status: Needs review » Needs work

Attached are some nit-picking review items from a documentation perspective:

+++ b/core/lib/Drupal/Core/StreamWrapper/LocalReadOnlyStream.phpundefined
@@ -0,0 +1,126 @@
+ * @file
+ * Definition of Drupal\Core\StreamWrapper\LocalReadOnlyStream.

'Definition of' should be 'Contains'. Changed in last few months.

+++ b/core/lib/Drupal/Core/StreamWrapper/LocalReadOnlyStream.phpundefined
@@ -0,0 +1,126 @@
+ * such as "sites/default/files/example.txt" and then PHP filesystem functions are

Exceeds 80 characters and needs to rewrapped.

+++ b/core/lib/Drupal/Core/StreamWrapper/LocalReadOnlyStream.phpundefined
@@ -0,0 +1,126 @@
+  /**
+   * Support for fwrite(), file_put_contents() etc.

Throughout class docblocks, need to start one line summaries with an active verb. In this case, perhaps 'Provides support for ...'?

+++ b/core/lib/Drupal/Core/StreamWrapper/LocalReadOnlyStream.phpundefined
@@ -0,0 +1,126 @@
+   * @param int $options
+   *   A bit mask of STREAM_REPORT_ERRORS and STREAM_MKDIR_RECURSIVE.

Suggest that this variable $options be renamed to $mask, as many in Drupal community think of $options as an array.

+++ b/core/lib/Drupal/Core/StreamWrapper/LocalReadOnlyStream.phpundefined
@@ -0,0 +1,126 @@
+   * @param int $options
+   *   A bit mask of STREAM_REPORT_ERRORS.

Ibid.

+++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.phpundefined
@@ -0,0 +1,25 @@
+  /**
+   * Implements abstract public function getDirectoryPath()
+   */
+  public function getDirectoryPath() {
+    return drupal_get_path('module', $this->getSystemName());
+  }
+}
\ No newline at end of file
diff --git a/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php

Needs a new line at end of the file.

+++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.phpundefined
+<?php
+/**
+ * @file

Should have a blank line after '<?php' and before @file docblock. Same error reoccurs several times in this patch.

+++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.phpundefined
@@ -0,0 +1,41 @@
+  }
+}
\ No newline at end of file
diff --git a/core/lib/Drupal/Core/StreamWrapper/SystemStream.php b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php

ProfileStream.php needs a blank line at end of that file.

+++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.phpundefined
@@ -0,0 +1,61 @@
+  /**
+   * Get the module, theme, or profile name of the current URI.

Needs to be active verb.

+++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.phpundefined
@@ -0,0 +1,61 @@
+
+  protected function getTarget($uri = NULL) {
+    if (!isset($uri)) {
+      $uri = $this->uri;

Missing docblock.

+++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.phpundefined
@@ -0,0 +1,61 @@
+  /**
+   * Overrides getExternalUrl().

Overrides from what class getExternalUrl()?

+++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.phpundefined
@@ -0,0 +1,25 @@
+  }
+}
\ No newline at end of file
diff --git a/core/modules/system/system.module b/core/modules/system/system.module

diff --git a/core/modules/system/system.module b/core/modules/system/system.module
index 6401361..dbdf370 100644

Missing newline at end of ThemeStream.php.

Xano’s picture

Throughout class docblocks, need to start one line summaries with an active verb. In this case, perhaps 'Provides support for ...'?

"Supports..."

Crell’s picture

Status: Needs work » Needs review

Lars: Please don't set an issue to CNR for nitpicky reviews if there are still architectural questions to be reviewed. It discourages people from making further reviews. Set to CNR when there are functional issues to be addressed.

pounard’s picture

While I really like the use of stream wrappers, a sum of details bothers me:

  • By design, PHP does not allow to fetch the realpath of a file handled by a stream URI: it forces us to manipulate directly stream wrappers instances (for example, when you build a file URL): this shouldn't be done, stream wrapper instances are only meant to be handled internally by PHP. It sounds wrong to use streams the way we do it
  • I hope we won't rely on this for PHP file inclusion for modules, because this would sound like unnecessary user space code just to replace a simple include, I know this patch doesn't but, I'd like to anticipate this because this could happen in the end

Aside of that that sounds cool.

fietserwin’s picture

1. LocalStream::getLocalPath() now rejects any .module, .theme, .engine., .inc., .install or .php file as not valid. This should prevent these files from being pulled via a Drupal stream wrapper at all.

Why would we like to prevent that? If we read a PHP file via a stream wrapper the code is not executed (in isolation) like would be done when .htaccess wouldn't prevent us to call http:///www.eample.com/themes/bartik/templates/page.tpl.php via the address bar. The code in itself is not secret or a security issue?!

pounard’s picture

Actually this answers one of my concerns, I'm OK with pulling off PHP files from the stream wrapper. It will both forbid code inclusion and code copy.

dman’s picture

@fietserwin Based on #21, #22, #24, #25 etc, I read this as "there may be a world of performance, security, and platform-version-based issues if we start to encourage that usage"

The sensible thing to do to get this actually in and useful is to (for now) limit the scope to a utility for accessing module (and theme) resource *files* and not module *code*. As was proposed and got the votes.

Trying to analyse all the reasons why stream wrappers maybe shouldn't be used for loading executable code may derail the issue.

pounard’s picture

Stream wrappers can load code, I don't really see any reason to pull that off except that I'm a bit worried about how APC would behave doing so. There was sometime ago bugs regarding APC and stream wrapper solved, but a lot of outdated installations still run out there. Anyway, Drupal owns its own files, knows all the paths, it doesn't need a stream wrapper to include files, it can do it properly without that much level of code indirection: loading code is already one of the slowest thing in Drupal.

chx’s picture

Not doing and documenting is enough. Ie. if drupal_load does not use any wrappers then we are good and we just need to tell people not to include a module through module:// at least, that's the typical case. But, if someone has a new APC and can live with apc.stat 0? We might want to add a $GLOBALS['conf']['code_prefix'] to allow things that go through module_load to live somewhere else -- yes, even inside a stream wrapper. Core should not do this but it should allow for it. Remember, our motto is if it's worth doing it's worth overdoing :) That's a followup however :) (and I changed my stance because the latest kernel patch allows for non-module bundles so should a followup turn stream wrappers into annotated plugins then we can move a stream wrapper into such a bundle)

Lars Toomre’s picture

Sorry @crell... I did not mean to change status.

fietserwin’s picture

Having said what I said in #33, I certainly do not envision to use stream wrappers to replace include's (as has also already been said many times by others), but that is no reason to prevent it completely.

On the other hand, if a visitor by means of user input manages to expose settings.php this way... But that seems more usage related and is still not up to the stream wrapper to prevent.

I could live though, and would even see it as a good feature, if the stream wrapper would allow to restrict on file type. So if the intended use is in image handling, restrict to (some) image file types. Core currently already allows to define so for file and image fields. Whether only to use "restrict to" or also implement "exclude file types of" is debatable.

brianV’s picture

@chx:

Are there any performance or other reasons that someone with apc.stat = 0 and a compatible build of APC would *want* to use stream wrappers to include PHP files?

ie., if we allow it to be user configurable, is there any good use case for the user actually doing it? Certainly, any code developed that way would be inconsistent with how it's handled everywhere else in Drupal and contrib.

@fietserwin:

> Having said what I said in #33, I certainly do not envision to use stream wrappers to replace include's (as has also already been said many times by others), but that is no reason to prevent it completely.

As I asked @chx above, wat's the use case where this would be useful? I'm not worried about core so much - I'm worried about bad contrib that breaks APC caching because the developer didn't get the memo that you can't be cache PHP loaded via a stream wrapper on most sites.

That said, if there was a compelling use case to allow them to do just that on properly configured installs, I would feel better about allowing it - either making it a settings.php option as @chx suggested, or just removing the code preventing it and documenting it in the d.o docs that you shouldn't be doing that.

fietserwin’s picture

What I can think of now:
- advanced help module: display .api.php which is a documentation file after all
- coder or other review modules: display (formatted and syntax coloured) code that contains warnings

So, the meta/reflection/review/self inspection type of modules.

Dave Reid’s picture

I would strongly disagree with adding logic to 'disallow' certain extensions rather than documenting our best practices for including PHP files like normal...

brianV’s picture

@fietserwin:

Ok, those are valid.

Attached patch:

1. Removes the restriction on loading php files via stream wrappers.
2. Accomodates Dave Reid's requests for theme://current and theme://admin shortcuts.

Crell’s picture

Doesn't #43 overlap with #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend, which also defines a read only stream wrapper?

(The patch looks good to me visually, but I think needs a test or two.)

brianV’s picture

@Crell,

Yes, this patch technically depends on the #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend as it uses the class I created that issue for. I have the class rolled into this patch so tests can run.

If we commit this issue, then that issue can be closed as redundant. If we commit that one, then I will reroll this patch without those classes.

pounard’s picture

@#42 Actually using advanced stream wrappers could open holes in certain cases (for example, we could trigger php file download while normally the webserver would block it even before php itself). I'm not sure thought, but disallowing php files to be accessed at all is certainly a good idea anyway.

fietserwin’s picture

Status: Needs review » Postponed

There is lots of activity going on in #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend, so let's sort that one out first.

jthorson’s picture

jthorson’s picture

Status: Postponed » Needs review
jthorson’s picture

Here's a re-roll with the duplicate LocalReadOnlyStream.php file from #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend pulled out.

jthorson’s picture

Status: Needs work » Needs review

Requeued on bot. Suspect the failure was due to broken HEAD in #347988: Move $user->data into own table.

fietserwin’s picture

Even while the current error indeed does not seem to be related, you will have to wait for #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend to be committed before this patch can succeed.

\ No newline at end of file

The new files in the patch don't contain a new line at the end of the file.

+++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.phpundefined
+  protected function getSystemName($uri = NULL) {
+    global $theme_key;

The global can be moved to the first if block. BTW: Is this still a global?

+  protected function getSystemName($uri = NULL) {
+    list($scheme, $target) = explode('://', $uri, 2);

Do we need to check the result of explode()? We will get a "Notice: Undefined offset: 1" if the uri is malformed (does not contain ://). This does not seems to be the most helpful message, neither for developers (testing/debugging) neither for admins/editors (if the file name came from them via an input form).

jthorson’s picture

Thanks, fietserwin ... realized the patch issue shortly after re-posting.

fietserwin’s picture

#1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend has been committed, let's see what the current status of this patch is.

fietserwin’s picture

Status: Needs work » Needs review
Issue tags: -sprint, -Media Initiative, -VDC
brianV’s picture

Status: Needs work » Needs review
FileSize
8.18 KB

Re-rolled patch against latest HEAD. Let's see how it handles the tests.

chx’s picture

Status: Needs review » Needs work

Let's add some documentation to drupal_get_path saying, this is the function to use when including PHP code, otherwise use the nice stream wrappers.

brianV’s picture

Here's a rev7 which adds the documentation @chx requested, along with the changes @fietserwin mentioned in #54.

The code throwing the InvalidArgumentException maybe could be pushed into it's own function since that block is repeated verbatim, but I still wanted to throw the exception from the function with the bad argument, so I've left it duplicated in the two functions.

This function doesn't fix the fails and exceptions the testbot highlighted in #59, for some reason, the menu routing system is failing when it tries to do:

$routing_file = "module://{$module}/{$module}.routing.yml";
if (file_exists($routing_file)) {
  $routes = $parser->parse(file_get_contents($routing_file));
  // ..snip
}

It passes the file_exists() check, but fails on the file_get_contents() call. Possibly file_get_contents() is passing a flag other than 'r' through to LocalReadOnlyStream::stream_open(), although there is no errors on the testbot reflecting the warning we throw in that use case.

It's unclear to me why this would work in -rev4 in #43, but now fails, especially since this is after drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE); is called, which registers the stream wrappers.

Is anyone aware of any relevant changes in the interim?

xjm’s picture

Status: Needs work » Needs review

Sending to the bot.

brianV’s picture

Looks like it needs another reroll for HEAD. Will fail testing regardless as it doesn't fix the problems from #59

nod_’s picture

Need this for #1996238: Replace hook_library_info() by *.libraries.yml file to reference JS and CSS files from yml files in modules.

sdboyer’s picture

Category: feature » task
Priority: Normal » Major

seconding @nod_'s note here. i have a kabillion other things to work on right now, but i'm escalating this to major.

it's also a task, not a feature; not having this essentially forces *any* logic that needs to locate a file within a module to be given direct execution control in order to call drupal_get_path(). that ends up being at the very least awkward, and possibly API-restricting for #1762204: Introduce Assetic compatibility layer for core's internal handling of assets.

Pancho’s picture

It's been asked quite a few times whether we would also add library:// (see #5, #17, #20), and a followup issue has been created at #1702958: Add libraries:// stream wrappers to access system files .
Libraries are covered by the D7 contrib module, and I think, we should add support for them from the beginning.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
9.19 KB

lets start with a reroll

ParisLiakos’s picture

ParisLiakos’s picture

should be tested, after the issue above is committed

hass’s picture

Can you make sure we also have base theme support, please? I'm including a lot of files from the base theme in my subthemes.

Pancho’s picture

@Paris: Awesome you found the source of this failure!
With #1954180: LocalReadOnlyStream::stream_open refuses to open a file when 'rb' flag is passed in at least the install runs through. Tests we need to see.

ParisLiakos’s picture

yeah, in my system file_get_contents passes 'rb' flag..probably its same for bot.
this issue definitely needs this tag

ParisLiakos’s picture

Status: Needs work » Needs review

blocker is in!

sdboyer’s picture

@hass #72 - file a followup. don't derail this.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Go for it. If this gets in, then akin to https://drupal.org/project/field and https://drupal.org/project/view a new project https://drupal.org/project/current will be necessary to avoid anyone creating a 'current' profile.

fietserwin’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.phpundefined
@@ -0,0 +1,76 @@
+  /**
+   * Get the module, theme, or profile name of the current URI.
+   */
+  protected function getSystemName($uri = NULL) {

@param and @return missing

+++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.phpundefined
@@ -0,0 +1,76 @@
+  protected function getSystemName($uri = NULL) {
...
+    $uri_parts = explode('://', $uri, 2);
+    if ($uri_parts == $uri) {

Comparing an array with a string? From the PHP manual : Arrays are always converted to the string"Array";

+++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.phpundefined
@@ -0,0 +1,76 @@
+
+  protected function getTarget($uri = NULL) {

documentation missing

+++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.phpundefined
@@ -0,0 +1,76 @@
+  protected function getTarget($uri = NULL) {
...
+    $uri_parts = explode('://', $uri, 2);
+    if ($uri_parts == $uri) {

Again comparing an array with a string

Pancho’s picture

Status: Needs work » Needs review

#71: drupal-wrappers-1308152-71.patch queued for re-testing.

ParisLiakos’s picture

that should fix #78

Pancho’s picture

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

Looks good.
However, the remaining bugs in #73 weren't tested to an error, so obviously the one implementation we have in DisplayPluginBase doesn't really suffice instead of tests. #1996238: Replace hook_library_info() by *.libraries.yml file might come or not, but IMHO we either need tests on the stream wrappers or more implementations in core, or both... :/

jthorson’s picture

Status: Needs work » Needs review
FileSize
5.54 KB
15.45 KB

First cut at adding some tests.

jthorson’s picture

Status: Needs work » Needs review
FileSize
15.04 KB

Ooops ... a bit of junk snuck in that last patch.

jthorson’s picture

Correcting a cut and paste error (renaming the test class).

jthorson’s picture

FileSize
5.13 KB

Updated interdiff (#80 to #85).

fietserwin’s picture

+++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.phpundefined
@@ -0,0 +1,86 @@
+   * @return string
+   *   The extension name.
+   */
+  protected function getSystemName($uri = NULL) {

I think it returns the name of the module, theme or profile.

Crell’s picture

+++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
@@ -0,0 +1,86 @@
+    return $GLOBALS['base_url'] . '/' . $dir . '/' . drupal_encode_path($path);

I don't think $GLOBALS['base_url'] is reliable anymore. We'll have to get this value from the request object.

Pancho’s picture

Assigned: Dave Reid » Pancho

Working on this some more minutes.

Pancho’s picture

Some more minutes... :)
I worked on the tests, and found more than a couple of points to be resolved:

1.)
Calling getSystemName() and getTarget() caused the fatal errors. They need to be public, and why shouldn't they?

2.)
Now we didn't disallow PHP files to be streamed, but a number of objections have been raised. So while we should probably test whatever is possible with our wrappers, we shouldn't feature the .inc file case so much in testModuleStream().

3.)
Also, I believe getSystemName() should be renamed to something more specific. It doesn't return the system name of the streamed file, but it returns the module/theme/profile name. Now we have not really settled on whether we call the aggregate of them "extensions" or "components", and both have their caveats. But in the end, they are all owners of the resources, so I propose getOwnerName(), a concept that is already used by _drupal_theme_initialize()

4.)

    if (count($uri_parts) === 1) {
      // The delimiter ('://') was not found in $uri, malformed $uri passed.
      throw new \InvalidArgumentException('Malformed $uri parameter passed: %s', $uri);

This should be for all streams not only system streams, so moving up to LocalStream::getTarget().

5.)

    // Remove erroneous leading or trailing, forward-slashes and backslashes.
    $target = trim($this->getTarget($uri), '\/');

    // Remove the module/theme/profile name form the file path.
    $target = substr($target, strlen($this->getSystemName()));

    // Trim again.
    $target = trim($target, '\/');

Why do we want to trim twice? Because the offset starts with 0, substr() already strips the component name including the following slash. That's fine and absolutely enough. If there should be more than one slash between module/theme/profile name and the rest of the target, then the URI is broken. So removing the second trim.

6.)
We should extend this by theme engines to match drupal_load() and drupal_get_filename():
theme_engine://twig
Libraries are a bit tricky to do right. Even though we didn't manage to get libraries API in core, we need to foresee what Libraries API module might need.

More and a patch to come...

fietserwin’s picture

#91.3: Now I get the name "extension". I find that quite confusing, as we are working with file names that tend to have an extension part as well. For me, the name "owner" is also not clear, whereas component or project (as on drupal.org) would be.

Pancho’s picture

Re #92)
"Extension" is ambiguous, and while it works fine for contrib modules/themes, it's less obvious for modules like system or node.
"Component" is used in many contexts either and I don't find it obvious either. If we wanted to settle for "component", this needed to be enforced throughout core IMHO.
A "project" might consist of several modules, so would be the wrong concept here.
"Owner" is very descriptive from the stream perspective. The module/theme/profile owns a filespace, that might wherever be located, so this part is mapped (think of 'current' or 'default' or the different possible locations). Whatever is following is the hierarchical filesystem part that finally leads us to the owned file to be streamed.

Re 6.)
No more sure if we should add a theme engine wrapper. While it would be there for completeness, it probably wouldn't be used at all and could return only a PHP file which we didn't want to encourage. I'm rolling it in but we can throw it out again, if the tendency is to throw it out.
Regarding non-system libraries, we still don't know where they are located, if and how they are registered. We really should leave libraries to #1702958: Add libraries:// stream wrappers to access system files as a followup and postpone that one to #667058: Add a libraries folder with a README.txt in it to DRUPAL_ROOT.

7.)
We're still missing stream:://default. Added that.

8.)
The test really should actually be based on UnitTestBase DrupalUnitTestBase, not WebTestBase. Not sure if that works out, though...

Even more and a new patch to come...

fietserwin’s picture

#93.7) where should default point to? Drupal base? Then call it base or drupal-base.

#93 re#92) you're right, making it even more difficult to come up with a good name. I still don't like owner though.

#93 6) I would favor dropping the theme_engine wrapper.

Pancho’s picture

Status: Needs work » Needs review
FileSize
32.81 KB
31.62 KB

There's still some more work to do, including but not only regarding tests, so this can't pass the testbot.

Still I'm posting the preliminary patch with already lots of fixes, improvements etc., including:
- #89
- #91 (except 6. that we don't want per #93 and #94)
- #93

Vastly expanded tests that are now based on DrupalUnitTestBase revealed a lot more issues.
Also added a few more implementations throughout core.
Some aspects need additional consideration, but we might be pretty close now.
Just posting it for another round of preliminary reviews.

Pancho’s picture

Issue tags: +API addition

Forgot to mention:
9.)
getDirectoryPath() needs to take $uri as parameter just the way getTarget() and others do. Contrary to PublicStream or TemporaryStream, the directory path depends here on the concrete $uri.

Re #95:
That's a bit more fails than I personally expected, but of course every fail multiplies for an API construct.
A propos API, I'm adding the API addition tag - we're not changing existing API, so have enough time to do this 100% right.

10.)
A couple of notes to myself:
a. Replace double-quotes where not needed
b. Check implementations can't we directly access the themes in many cases?
c. Reuse test_theme in our unit tests.
d. Do we really need core/modules/file/tests/file_test/file_test.dummy.inc? If yes, fix reference to renamed test.
e. Fix "theme://current" and figure out "theme://default"
f. Measure and improve performance.

Pancho’s picture

11.)

LocalStream::getTarget() is the equivalent to file_uri_target(), and it should give us the part of the URI after <scheme>://. So stripping the module/theme/profile name in SystemStream::getTarget() might be convenient, but it looks like abuse of what target means. Finally, from the stream perspective, the URI always consists of the scheme, the :// delimiter and the target.
While from the filesystem perspective this shift is matched by getDirectoryPath() including the module/theme/profile name, this seems to make things even more complicated.

From the stream perspective it's:
<owner_type> :// <owner_name> / <resource>
mapping to:
module :// node / node.js
theme :// admin / logo.png
profile :// current / config/node.type.article.yml

which in the filesystem represents:
<drupal_basedir> / <owner_basedir> / <owner_name> /
mapping to:
/var/www / core/modules / node / node.js
/var/www / core/themes / bartik / logo.png
/var/www / core/profiles / standard / config/node.type.article.yml

This is a bit obscure, because scheme and the first part of the target together define the owner (which might be substituted if something like "current"), while the rest represents the hierarchical filesystem part leading us to the actual file.

We can do that for convenience, but logically it might be a better idea to have a single system:// scheme with the owner being completely defined in the first part of the target:

system :// <owner> / <resource>
mapping to:
system :// module:node / node.js
system :// theme:admin / logo.png
system :// profile:current / config/node.type.article.yml

which in the filesystem would represent:
<drupal_basedir> / <owner> / <resource>
mapping to:
/var/www / core/modules/node / node.js
/var/www / core/themes/bartik / logo.png
/var/www / core/profiles/standard / config/node.type.article.yml

We'd need to make our generic system:// stream wrapper extendable though, so contrib can add:
system :// library:openlayers / OpenLayers.js
system :// whatever:foobar / foo.bar
system :// theme:sectionX / theme.css

(spaces added for clarification, bold is what belongs together representing the owner, in italics are placeholders)

This is important before we move on, and I believe while it is slightly more verbose, it logically makes more sense, and is easier to grasp, therefore more failproof.

Instead of system:// we could also use drupal:// which might make the distinction between installation files, user data (public:// and private://) and temporary data (temporary://) even clearer. But that would be just an option.

Thoughts?

Pancho’s picture

Status: Needs work » Needs review

"Needs review" in order to discuss #98.

fietserwin’s picture

I don't have a real preference other then backward compatibility. The imageacache_actions module (which I co-maintain) actively promotes the use of the sytem stream wrapper module and thus stores the current notation in the image effect data.

Looking at the syntax for some other stream wrappers I found this page: http://php.net/manual/en/wrappers.php.php, which in our case would suggest something like: drupal:\\module\name=node\resource=node.js. That's too far off for me.

Moreover the (system or) drupal syntax would suggest that we also should convert the current public, private and temp to drupal:\\public, drupal:\\private and drupal:\\temp (the scheme is the provider/implementor), which we can't do anymore (at least not without leaving the current syntax as is, albeit in a deprecated state).

All in all, I favor the way it currently is, thus module:\\, theme:\\ and profile;\\.

jibran’s picture

effulgentsia’s picture

This is a bit obscure, because scheme and the first part of the target together define the owner

I don't get why that's obscure. ftp://user@example1.com, mailto://user@example1.com, ftp://user@example2.com, and mailto://user@example2.com, can all get resolved to completely different servers.

So I think module://, theme://, and profile:// are fine. However, I'm also not opposed to a single scheme (system:// or drupal://) if people prefer that.

pwolanin’s picture

Like drupal://module/node/$filesname vs drupal://theme/$themename/css/$filename ?

That would seem less in line with how we are using local stream wrappers currently, though maybe fine. That would be a more or less replacement for drupal_get_path($type, $name).

effulgentsia’s picture

Like drupal://module/node/$filesname vs drupal://theme/$themename/css/$filename ?

No, the suggestion in #98 would be drupal://module:node/css/node.admin.css and drupal://theme:bartik/css/layout.css. Or possibly system:// instead of drupal://. Per #102, I don't really see the benefit of that vs. doing what the issue title says, but just clarifying the suggestion.

pwolanin’s picture

@effulgentia - oh, that's ugly - why not what I suggest? Certainly what's in the title would be better?

Or even, if D8 is enforcing uniqueness among module, theme, and profile names, why not just like:

drupal://bartik/css/layout.css

Given that modules, themes, and profiles have come to have more overlap in capacity.

ParisLiakos’s picture

we dont enforce unique names among themes/modules at least not in d7
i suggest we stick to the issue title

jibran’s picture

+1 to #106

thedavidmeister’s picture

+1 to #106

jcisio’s picture

#106 even that, why not #98 or #102 for single scheme? KISS.

effulgentsia’s picture

How is system://module:node/ more KISS than module://node/ ?

jcisio’s picture

#110 module://node is more convenient, but less KISS. Because it adds unnecessarily many stream wrappers (theme, module, profile, then probably library one day). They are all in the same file system, why pretend not?

fietserwin’s picture

Status: Needs review » Needs work

I see a majority for sticking to module://, theme:// and profile://. Back to NW as per #96 (and #97). Pancho? (you assigned it to yourself)

ParisLiakos’s picture

They are all in the same file system, why pretend not?

thats also true for private:// and public://
and i think i like it the way it is

jcisio’s picture

#113 Nope. private:// and public:// are for files that are not used directly by Drupal, they are users' stuffs. They can be in a different file system and have completely different urls. That's not the case for themes, modules and profiles, which are always at the same partition (and precisely, in sub directories of Drupal's index.php).

#98, #100 In no way PHP suggests doing "drupal:\\module\name=node\resource=node.js", it's just the syntax for parameters in the php://filter resource. And I agree that drupal://module:node is ugly. I prefer drupal://module/node.

jcisio’s picture

Title: Add module://, theme:// and profile:// stream wrappers to access system files » Add drupal:// stream wrapper to access system files
Status: Needs work » Needs review

Ok, I'm trying to change issue title to see reaction. As the original title said, they are all "system files", there is no reason to add 3 stream wrapper.

tstoeckler’s picture

Title: Add drupal:// stream wrapper to access system files » Add module://, theme:// and profile:// stream wrappers to access system files

I am strongly against a single drupal:// stream wrapper. There really is no technical argument here. You say adding three stream wrappers for supposedly the same thing is "bloat", but how does that bloat show itself? Are there any performance implications I'm missing?

And just like you can put private:// outside of the webroot, one could override the stream wrappers and put modules and themes someplace completely different. Not that that's a necessary feature, but saying that all system files necessary live in the same place is just wrong. That's just how it has always worked, because we didn't have the abstraction layer of stream wrappers.

And in terms of DX I strongly prefer module:// over drupal://module. A potential libraries:// stream wrapper will behave a bit differently, so in order to support that the drupal:// would have to provide some sort of plugin/hook system to provide these types. Let's not go there, please.

ParisLiakos’s picture

Title: Add module://, theme:// and profile:// stream wrappers to access system files » Add drupal:// stream wrapper to access system files

Nope. private:// and public:// are for files that are not used directly by Drupal, they are users' stuffs. They can be in a different file system and have completely different urls. That's not the case for themes, modules and profiles, which are always at the same partition (and precisely, in sub directories of Drupal's index.php).

actually no, this, might be true for temporary:// but not for public://
public:// is supposed to be always under DRUPAL_ROOT..i am not sure about private:// cause i never used it

ParisLiakos’s picture

Title: Add drupal:// stream wrapper to access system files » Add module://, theme:// and profile:// stream wrappers to access system files

x-post
agree with #116

tim.plunkett’s picture

Please leave the title as is. +1 for module:// theme:// profile:// and actually just getting this done.

jcisio’s picture

@ParisLiakos @tstoeckler What is the reason for putting system files (modules, themes etc.) outside the DRUPAL_ROOT? Not to say it is impossible. System files path is determined by drupal_get_filename() and is based on DRUPAL_ROOT. Other stuffs (public, private, temp) are accessed directly, or are exposed to the browser using file_create_url() which itself uses the stream wrapper to determine the url.

I don't find any issue that says libraries will behave differently and will be pluggable.

I give my thought but certainly don't want to hold this issue off.

ParisLiakos’s picture

just for safety..any file that doesnt have to be in web accessible directory shouldnt..eg what symfony does..exposes only index.php and assets from /PATH_THAT_CONTAINS_COMPONENTS/web/index.php . or sth like that
you see there the web root is below the web folder, but the actual code leaves one level upper

and actually just getting this done.

+1000

sdboyer’s picture

echoing #116, #119. i am absolutely opposed to a single unified wrapper. the use case here is, and has always been, for the three wrappers: module://, theme:// and profile://. that is what makes better DX. you know how i know? by imagining explaining this patch to someone. here's the explanation for using the original three:

We added stream wrappers for modules, themes, and profiles. No more drupal_get_path()!

essentially self-explanatory, assuming you know what a stream wrapper is.

now, here's that explanation for system://:

We added a system stream wrapper. No, it's not for system module. No, it's not for accessing the underlying OS's system resources. No, y'see, It lets you target modules, themes, and profiles - no more drupal_get_path().

now, here's that explanation for drupal://:

We added a drupal stream wrapper. No, it's not for accessing stuff on drupal.org. No, we didn't have a long dialogue with Wordpress and they also have a wordpress:// stream wrapper now, and the two are somehow similar. It's just for being able to address files in modules, themes, and profiles as a replacement for drupal_get_path().

the followup Q&A on the last two is, "why did they name the stream wrapper 'drupal'/'system'?"

"oh, it's because [blah blah blah same filesystem blah blah internal consistency blah blah resource ownership blah blah DETAILS YOU SHOULDN'T HAVE TO CARE ABOUT]."

when you have to understand the internals of an interface to understand why it's constructed the way it is, it sorta misses the point of being an interface. there are cases where surfacing implementation details is worthwhile because of the lesson it teaches the user...but only if there's an unambiguously useful lesson. there is no such unambiguous lesson here, a conclusion i draw from the range of opinions in the last 30 comments about whether or not a single unified wrapper for this use case is or is not like public:// and private://.

fietserwin’s picture

Status: Needs review » Needs work

I see a large majority for sticking to module://, theme:// and profile://. It is good this has been discussed prior to being added to core, but I think we have a conclusion and thus suggest we go back to Needs Work now (as per #96 and #97)

and actually just getting this done.

sdboyer’s picture

Issue tags: +happy princess API

though a bit far removed, it's still accurate to say this is happy princess...

seems like this is kinda dead in the water. what'll it take to restart it?

webchick’s picture

I don't understand why we'd do this now that we're post-API freeze. The issue summary does not explain what makes this a "major" task.

jthorson’s picture

... why we'd do this now that we're post-API freeze.

I'd interpret this as an API addition which provides a simplified and improved DX experience ... but doesn't 'break' existing methods for accessing these same files while doing so.

webchick’s picture

So this won't spin off 300 sub-issues to convert all drupal_get_path() calls to module:// or theme:// or whatever? :P Somehow I doubt it. ;)

I assume the missing tag was a weird Drupal.org thing, since the issue summary is still not clear on what's being proposed here and why it's major.

jthorson’s picture

D.o tag monster

thedavidmeister’s picture

So this won't spin off 300 sub-issues to convert all drupal_get_path() calls to module:// or theme:// or whatever? :P Somehow I doubt it. ;)

It might, but they'd be novice issues, right?

webchick’s picture

Probably. But we already have about 50 such initiatives in D8 targeting novices, most of which are between 0% and 40% done, all of which are going to leave Drupal 8 in an inconsistent state if they're not finished all the way to completion.

We need to be focusing on getting a beta out the door. I don't understand how this issue gets us there faster. I'm happy to consider arguments to that effect in the issue summary, though.

disasm’s picture

Issue summary: View changes

Applying template

disasm’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
31.33 KB

Issue summary is updated. Starting with a reroll. If it passes, I'll start working on some tests. Also, have some possible ideas as to how to script conversion of the bulk of them. There will obviously be some nasty ones that won't match a simple regexp, but if we can get 75% there, that's progress. I'm doing this because I cringe everytime I have to add a drupal_get_path to a route conversion and this seems like a sane idea to me...

Crell’s picture

Status: Needs work » Needs review

To Webchick's point, as long as we don't remove the old "drupal_get_path() and concatenate" mechanism and just mark it deprecated there's no API break, and we can convert code over to the stream wrappers at our leisure, including after 8.0. I don't see why it would have to become a release-blocking issue.

disasm’s picture

I think I went to deep when I tried to fix a merge conflict. reverted ModuleHandler class, and it installs now.

Anonymous’s picture

Issue summary: View changes

forgot problem

BTMash’s picture

Assigned: Pancho » BTMash

Reassigning to self for rerolling as there is no new development on this. Seeing that the stream wrappers are going to become services, better to essentially wait until #2028109: Convert hook_stream_wrappers() to tagged services. as I think that patch will get in before this one has a chance.

BTMash’s picture

almaudoh’s picture

+++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
@@ -0,0 +1,74 @@
+    $start = strpos($target, '/') + 1;
+    $target = ($start === FALSE) ? '' : substr($target, $start);

$start will never be FALSE because of the addition to 1.
Better:

$start = strpos($target, '/');
$target = ($start === FALSE) ? '' : substr($target, $start + 1);
almaudoh’s picture

Status: Needs work » Needs review
FileSize
28.71 KB

Re-roll + #138

t0xicCode’s picture

Issue tags: +Needs reroll
almaudoh’s picture

Assigned: BTMash » Unassigned
Issue tags: -Needs reroll
FileSize
32.44 KB

Re-rolled patch with test fixes.

almaudoh’s picture

Status: Needs work » Needs review

Oops! :p

Arla’s picture

Assigned: Unassigned » Arla
Arla’s picture

As far as I can see, we cannot test ProfileStream::getOwnerName() for profile://current, because there is no current profile during a unit test.

Arla’s picture

Status: Needs work » Needs review
FileSize
33.41 KB
15.78 KB
  • Avoid appending "/" in getExternalUrl() if there is no target.
  • Check file_exists() in getTarget() but not getExternalUrl().
  • Some standards-related updates to the test.
  • Some changes in expected results in the test.

The problem in my last comment #147 could be solved by modifying Settings (as seen in the patch). Still failing most ThemeStream tests, for similar reasons. Is there a similar modification that can be made?

almaudoh’s picture

Thanks for taking this up. Some quick reviews:

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -48,6 +48,20 @@ public function getOwnerName($uri = NULL) {
    +    $target = $this->extractTarget($uri);
    +    return file_exists($this->getDirectoryPath($uri) . '/' . $target) ? $target : NULL;
    +  }
    ...
    +  protected function extractTarget($uri = NULL) {
    
    @@ -69,9 +82,8 @@ public function getExternalUrl($uri = NULL) {
    +    $target = $this->extractTarget($uri);
    +    $path = $target != '' ? '/' . UrlHelper::encodePath(str_replace('\\', '/', $target)) : '';
    +    return \Drupal::request()->getBaseUrl() . '/' . $dir . $path;
    

    We may need to define how we want each of the methods to behave here. Does getExternalUrl() fail if the resource doesn't actually exist?

  2. +++ b/core/modules/system/src/Tests/File/SystemStreamUnitTest.php
    @@ -21,28 +24,29 @@ class SystemStreamUnitTest extends DrupalUnitTestBase {
    +    new Settings(Settings::getAll() + array(
    +      'install_profile' => 'standard',
    +    ));
    ...
    +  public function tearDown() {
         parent::tearDown();
       }
    

    Nice!! Shouldn't we restore the original settings in the tearDown() method? e.g.

    
    public function setUp() {
      ...
      $this->originalSettings = Settings::getAll();
      new Settings($this->originalSettings + array(
        'install_profile' => 'standard',
      ));
      ...
    }
    
    public function tearDown() {
      ...
      new Settings($this->originalSettings);
    }
    
    
  3. +++ b/core/modules/system/src/Tests/File/SystemStreamUnitTest.php
    @@ -196,16 +204,16 @@ function testThemeStream() {
    +    $this->assertEqual($instance->getExternalUrl($uri5), $base_url . 'core/themes/standard', format_string('Lookup real directory path of %current for a partial URI.', array('%current' => 'profile://current')));
    +    $this->assertEqual($instance->getExternalUrl($uri6), $base_url . 'core/themes/standard', format_string('Lookup real directory path of %current for a resource.', array('%current' => 'profile://current')));
    +    $this->assertEqual($instance->getExternalUrl($uri7), $base_url . 'core/themes/stark', format_string('Lookup real directory path of %default for a partial URI.', array('%default' => 'profile://default')));
    +    $this->assertEqual($instance->getExternalUrl($uri8), $base_url . 'core/themes/stark', format_string('Lookup real directory path of %default for a resource.', array('%default' => 'profile://default')));
    +    $this->assertEqual($instance->getExternalUrl($uri9), $base_url . 'core/themes/seven', format_string('Lookup real directory path of %admin for a partial URI.', array('%admin' => 'profile://admin')));
    +    $this->assertEqual($instance->getExternalUrl($uri10), $base_url . 'core/themes/seven', format_string('Lookup real directory path of %admin for a resource.', array('%admin' => 'profile://admin')));
    

    these should be 'theme://current', 'theme://default' and 'theme://admin'

I'm also working on the current, default and admin theme settings which don't get carried over into Unit tests.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
36.22 KB
22.34 KB

The main problem is that \Drupal::container is not the same as the KernelTestBase::container and so the themes list is not carried over into \Drupal::container when KernelTestBase is setUp(). I've added a line to work around this for themes test to work, but this is not the real solution to the problem. We need to have only one drupal container in tests and there are other issues where this symptom comes up.

Here's a patch with the tests fixed for this.

Arla’s picture

#152:

1. I spoke to slashrsm, who indeed argued that getExternalUrl() should not check for file existence, for performance reasons.

2. Aren't things like Settings reset automatically after a test is run? Looking at two similar cases where Settings is modified (KernelTestBase and SettingsTest), nothing is done in tearDown(). (Of course, good practice may say something else...)

neclimdul’s picture

2) The run method cleans up the environment directly by calling the restoreEnvironment method.
http://cgit.drupalcode.org/drupal/tree/core/modules/simpletest/src/TestB...

neclimdul’s picture

Scanned through some of the code. Wouldn't call it thorough but these things stood out.

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
    @@ -87,10 +87,17 @@ protected function getTarget($uri = NULL) {
    -    list(, $target) = explode('://', $uri, 2);
    +    $uri_parts = explode('://', $uri, 2);
    +    if (count($uri_parts) === 1) {
    +      // The delimiter ('://') was not found in $uri, malformed $uri passed.
    +      throw new \InvalidArgumentException('Malformed $uri parameter passed: %s', $uri);
    +    }
    +    else {
    +      list(, $target) = $uri_parts;
    
    +++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php
    @@ -0,0 +1,49 @@
    +/**
    

    unrelated but seems reasonable.

    Should this get tested though?

  2. +++ b/core/modules/system/src/Tests/File/SystemStreamUnitTest.php
    @@ -0,0 +1,260 @@
    +    new Settings(array_merge(array('install_profile' => 'standard'), $this->originalSettings));
    

    originalSettings is for internal use. Sun will not be happy with you using it in your test. Mock what you need explicitly.

  3. +++ b/core/modules/system/src/Tests/File/SystemStreamUnitTest.php
    @@ -0,0 +1,260 @@
    +//    $this->originalThemeFiles = \Drupal::state()->get('system.theme.files');
    +//    \Drupal::state()->set('system.theme.files', $this->container->get('state')->get('system.theme.files'));
    

    ??

  4. +++ b/core/modules/system/src/Tests/File/SystemStreamUnitTest.php
    @@ -0,0 +1,260 @@
    +    new Settings($this->originalSettings);
    

    As noted, this is handled as part of the runner.

  5. +++ b/core/modules/system/src/Tests/File/SystemStreamUnitTest.php
    @@ -0,0 +1,260 @@
    +    $instance = file_stream_wrapper_get_instance_by_scheme('module');
    +    /* @var $instance \Drupal\Core\StreamWrapper\ModuleStream */
    ...
    +    $instance = file_stream_wrapper_get_instance_by_scheme('profile');
    +    /* @var $instance \Drupal\Core\StreamWrapper\ProfileStream */
    ...
    +    $instance = file_stream_wrapper_get_instance_by_scheme('theme');
    +    /* @var $instance \Drupal\Core\StreamWrapper\ThemeStream */
    

    We generally put documentation before the code.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
35.57 KB
3.65 KB

#157:
1. Not done yet. Wondering if this should be in a new LocalStreamUnitTest test class.
2. Oops :P. Didn't see this in the base class. Removed - see 4.
3. Removed - see note 4.
4. Because we don't need to restore previous settings all the artefacts related to this have been removed (including 2 and 3 above).
5. Fixed.

I still expect 30 test fails from this patch because run-tests.sh doesn't initialize a theme environment (patch passes on the SimpleTest UI). There's currently no documented way to install themes in tests (like we have for modules). Sun advised on irc that I raise an issue for that.

almaudoh’s picture

I have raised #2313329: Provide a means to install and enable themes in a KernelTest to address the fails due to theme info not being available in KernelTestBase.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
35.66 KB
728 bytes

Following Sun's suggesiton on the other issue seems to let it pass.

almaudoh’s picture

Following Sun's suggesiton on the other issue seems to let it pass.

Wow!! Awesome! Simple and elegant. :)

I've updated the patch:
1. Removed unnecessary theme-related code - everything is now done by the ThemeHandler
2. Moved the ThemeHandler::enable() calls to testThemeStream() method since it is only needed for theme stream wrapper tests.

Patch now passes, yay!!

After reviews, will post a follow-up for conversion of drupal_get_path() calls.

Edit:
3. Also added a new test for the \InvalidArgumentException thrown for malformed uris (per #157.1) - fixed a bug in the process too. Nice :)

fietserwin’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
    @@ -87,10 +87,17 @@ protected function getTarget($uri = NULL) {
    +      throw new \InvalidArgumentException('Malformed $uri parameter passed: %s', $uri);
    +    }
    ...
    +      list(, $target) = $uri_parts;
     
    

    Same error you already found elsewhere.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
    @@ -87,10 +87,17 @@ protected function getTarget($uri = NULL) {
    +    else {
    +      list(, $target) = $uri_parts;
    

    After throwing an exception this code does not need to be in an else branch.

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
    @@ -87,10 +87,17 @@ protected function getTarget($uri = NULL) {
    +      list(, $target) = $uri_parts;
     
    

    $target = $uri_parts[1]; ???

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
    @@ -0,0 +1,46 @@
    +  /**
    +   * Get the module name of the current URI.
    +   *
    +   * @param string $uri
    +   *   Optional URI.
    +   *
    +   * @return string
    +   *   The extension name.
    +   */
    +  public function getOwnerName($uri = NULL) {
    +    $name = parent::getOwnerName($uri);
    +    return \Drupal::moduleHandler()->moduleExists($name) ? $name : FALSE;
    +  }
    +
    

    We can use {@inheritdoc}, FALSE is not a string and should not be returned: throw an exception instead (like in the parent).

  5. +++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
    @@ -0,0 +1,46 @@
    +  public function getDirectoryPath($uri = NULL) {
    +    return drupal_get_path('module', $this->getOwnerName($uri));
    

    We can use {@inheritdoc},unless we want to document a @throws.

  6. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,89 @@
    +  public function getOwnerName($uri = NULL) {
    +    if (!isset($uri)) {
    

    Should we document a @throws?

  7. +++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
    @@ -0,0 +1,46 @@
    +/**
    + * Defines a read-only Drupal stream wrapper base class for modules.
    + *
    + * This class extends the complete stream wrapper implementation in LocalStream.
    + * URIs such as "module://system" are expanded to a normal filesystem path
    + * such as "modules/system" and then PHP filesystem functions are
    + * invoked.
    + */
    +class ModuleStream extends SystemStream {
    +
    

    IMO this sentence is enough:
    Defines the read-only module:// stream wrapper for for module files.
    We don't have to explain stream wrappers here. BTW: this also holds for the other (profile and stream) classes.

  8. +++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php
    @@ -0,0 +1,49 @@
    +   *
    

    IIRC, there once was an issue with the install profile being uninstalled. So perhaps we should also check if the current profile still exists... and of course throw an exception, not return FALSE.

  9. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,89 @@
    +    else {
    +      list($scheme, $target) = $uri_parts;
    +    }
    +    // Remove the trailing filename from the path.
    

    we can remove the else around this code.

  10. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,89 @@
    +  /**
    +   * Returns the local target of the resource, regardless of whether it exists.
    +   *
    +   * @param string $uri
    +   *   Optional URI.
    +   *
    +   * @return bool|string
    +   *   A path to the local target.
    +   */
    +  protected function extractTarget($uri = NULL) {
    +    // If the owner doesn't exist at all, we don't extract anything.
    +    if ($this->getOwnerName($uri) === FALSE) {
    +      return FALSE;
    +    }
    +    $target = parent::getTarget($uri);
    +    // Remove the preceding owner name including slash from the path.
    +    $start = strpos($target, '/');
    +    $target = ($start === FALSE) ? '' : substr($target, $start + 1);
    +    return $target;
    +  }
    +
    

    getOwnerName() does not return FALSE but throws an exception, at least the base implementation currently does and, see above, I propose that the child classes follow that pattern. So let's throw (not catch) an exception here as well, update the @return accordingly and add a @throws.

  11. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,89 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getExternalUrl($uri = NULL) {
    +    $dir = $this->getDirectoryPath($uri);
    +    if (empty($dir)) {
    +      return FALSE;
    +    }
    +
    +    $target = $this->extractTarget($uri);
    +    $path = $target != '' ? '/' . UrlHelper::encodePath(str_replace('\\', '/', $target)) : '';
    +    return \Drupal::request()->getUri() . $dir . $path;
    +  }
    +}
    

    Same story here: FALSE is not a string. => use throw

  12. +++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.php
    @@ -0,0 +1,62 @@
    +    // Return name only for enabled themes.
    +    return array_key_exists($name, \Drupal::service('theme_handler')->listInfo()) ? $name : FALSE;
    +  }
    

    Besides the false/throw remark, in D7 a base theme does not have to be enabled to allow a deriving theme to work correctly, including using file resources from it. If that is still the case in D8, I think this might fail, unless listInfo() returns parent themes as well, even if not enabled. (Might be an extra test case)

  13. +++ b/core/modules/system/src/Tests/File/SystemStreamUnitTest.php
    @@ -0,0 +1,278 @@
    +        $this->pass(format_string('Throw exception on invalid uri %uri supplied.', array('%uri' => $bad_uri)));
    +      }
    

    Use String::format(). BTW: this holds for every usage of format_string and also the sprintf used in the exception message formatting (though that might become a bit tricky if stream wrappers become services that can be loaded very early).

Quite some points, but most are documentation and error handling. 1 possible real problem though, the theme remark

almaudoh’s picture

Status: Needs work » Needs review
FileSize
38.33 KB
44.09 KB

#163: Great reviews @fietserwin:
1. Fixed
2. Done
3. Done
4, 5, 6, 7, 8, 9, 10 & 11 all done.
12. Valid point. However, recent work in D8 ThemeHandler and Extension system requires that base themes be enabled before sub-themes can be enabled. See #1067408: Themes do not have an installation status.
13. Replaced format_string() in the tests. However because stream wrappers may soon become services (#2028109: Convert hook_stream_wrappers() to tagged services.) I've left the sprintf in the InvalidArgumentExceptions. Did a quick search through D8 core and found no consistent approach. Some use t(), some use format_string(), others concatenate strings with variables.

Moved code around a bit in the tests to allow for easier addition of tests, reduce boilerplate code duplication and handle the InvalidArgumentExceptions().

fietserwin’s picture

Status: Needs review » Needs work

Thanks for reworking the patch. As the interdiff is larger than the patch itself, it must have been some work.

  1. On applying I got:
    <stdin>:568: trailing whitespace.
    <stdin>:589: trailing whitespace.
    <stdin>:650: trailing whitespace.
    <stdin>:787: trailing whitespace.
    <stdin>:802: trailing whitespace.
    warning: squelched 2 whitespace errors
    warning: 7 lines add whitespace errors.

    (Seems to be in SystemStreamUnitTest.php)

  2. Regarding sprintf() vs String::Format() versus t(): String::Format seems to be/become the standard. It gives us escaping (on possibly user entered data and in imagecache_actions it can actually be user entered data) and if I see that UrlHelper and other services are used in the same functions, I'm not afraid that this will lead to problems. Translating exceptions is discouraged (#2055851: Remove translation of exception messages) as it was a bad idea after all.
  3. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
    @@ -301,14 +308,14 @@ public function stream_close() {
    -   * @return resource|false
    +   * @return resource|FALSE
    

    Should be false in lowercase: https://www.drupal.org/coding-standards/docs#types

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,104 @@
    +      throw new \InvalidArgumentException(sprintf('Target %s does not exist', $uri));
    +    }
    

    Target or Uri does not exist? (Uri is the name of the parameter and is what gets printed, target is the name of the internal variable and appears in the method name)

  5. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,104 @@
    +    if ($this->getOwnerName($uri)) {
    +      $target = parent::getTarget($uri);
    

    This will always be true, so we can remove the if around the code.

  6. getTarget() is documented in LocalStream as possibly returning a bool: it doesn't anymore.
  7. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,104 @@
    +  public function getExternalUrl($uri = NULL) {
    +    $dir = $this->getDirectoryPath($uri);
    

    Where does the $uri parameter come from? The interface does not declare it and it is not documented (nor is the null value). See also below for inconsistencies in stream wrappers its current state.

  8. To get rid of procedural code we can replace drupal_get_path in ModuleStream::getDirectoryPath() with:
    return \Drupal::moduleHandler()->getModule($this->getOwnerName($uri))->getPathname();
    I'm not sure if the same is possible for the other 2 wrappers.

Looking further into this, i found a number of inconsistencies in and between existing and new code. So, how do we actually envision usage of these Drupal specific features?

  • drupal_realpath($uri)
    drupal_dirname($uri)
    

    Both create an instance, call setUri() and call the accompanying method without $uri parameter. So we can do without that parameter, but do we want to? Do we also want procedural wrappers fro getExternalUrl or

  • getTarget() is public in SystemStream, yet protected in LocalStream: why? While we (still) have a file_uri_target($uri) (in file.inc)
  • Should protected methods work on $this->uri or get it passed as parameter (getTarget(), getLocalPath() in LocalStream, extractTarget() in SystemStream)? If uri gets passed in, may it be null?
  • getExternalUrl() does not accept a uri in StreamWrapperinterface, yet it does in SystemStream (yes, this is allowed in PHP, but is also often a code smell). Note: we (still) do have a file_create_url($uri).

So, I think that StreamWrapperInterface and LocalStream are already flawed (or at least inconsistent) and that therefore SystemStream (and its children) on top of that cannot be implemented in a consistent way.

My ideas to the above:
- get rid of the $uri parameter in Drupal specific public methods (thus of course not those following PhpStreamWrapperInterface) and all protected methods.
- add a constructor that optionally accepts a uri (this allows for shorter code in direct usage of these classes).
- make getTarget() public.

Arla’s picture

Status: Needs work » Needs review
FileSize
38.29 KB
4.69 KB

Fixed 1, 3, 5 and 6.

Also removed extractTarget(), which I added earlier, in favour of strstr().

2. Seems reasonable, so I leave it as is.

4. I suggest "File %s does not exist" with this patch. It is pretty standard, and it is actually triggered if file_exists() fails.

7. I totally approve adding a constructor to LocalStream with optional $uri parameter, and removing the optional param from the get methods. But I'm thinking that might belong to another issue.

I skipped point 8 for now.

(9.) If I understand correctly, SystemStream::getTarget() has quite a different purpose than LocalStream::getTarget() and file_uri_target(). The latter simply give the part after ://, while the former gives a path relative to the owner (in other words: skips until after the first '/'). Maybe we should leave LocalStream::getTarget() protected, and rename SystemStream::getTarget() to something like getRelativePath()?

fietserwin’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
    @@ -0,0 +1,41 @@
    +      throw new \InvalidArgumentException(sprintf('Module %s does not exist or is disabled', $name));
    +    }
    

    I thought that we can no longer disable modules and themes, only uninstall them?

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,90 @@
    +    if ($path = strstr(parent::getTarget($uri), '/')) {
    +      // Clean the path.
    

    Why do we need to call the parent implementation here? This is a code smell.

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,90 @@
    +  public function getExternalUrl($uri = NULL) {
    ...
    +    return \Drupal::request()->getUri() . $dir . $path;
    +  }
    

    Oops, do the tests not catch this? Looking at the implementation of Symfony\Component\HttpFoundation\Request it should be \Drupal::request()->getSchemeAndHttpHost() . \Drupal::request()->getBaseUrl(), nicely wrapped by \Drupal::request()->getUriForPath('');

  4. Error handling:
    Many of our non PhpStreamWrapperInterface methods now throw an exception. But this also means that, indirectly, many of the PhpStreamWrapperInterface methods now may throw an exception, which is against that interface. to correct, I suggest:
    • Make the realpath() method follow the PHP realpath() function specification. Thus adapt the doc and have the code catch InvalidArgumentException and return false.
    • Make the dirname() method follow the PHP dirname() function specification. Thus have the code catch InvalidArgumentException and return ... . (Note: PHP's dirname() always return a string, never FALSE, because it actually naively operates on the string.)
    • Make the methods that are part of the PhpStreamWrapperInterface implementation, use realpath() instead of getLocalTarget() (and have them handle a return value of false).
  5. I'm willing to accept your reply to #2 and have a core committer have its say on it.
  6. Your suggestion to point 4 is good: please use that phrase.
  7. (#9) I don't think that getTarget() serves different purposes, it is that the starting point in the file system is defined by either the scheme name only (public, private, temporary) or by the scheme name plus owner name (module, theme, profile, library). In both cases, getTarget() returns the relative path within that starting directory.

Regarding the state of the API for our stream wrappers
Having reviewed this code now for the 3rd time in a short period, I mentioned I kept jumping from method to method, following call chains and going round in circles. To me, this is a sign that something structural is wrong with what we are doing here. Think of:
- Is the inheritance tree logical?
- What methods are there and what methods would I expect?
- where are the methods implemented and overridden and is this logical?
- Is method naming logical and consistent?

So back to the tasks and responsibilities of a stream wrapper: implement PHPStreamWrapperInterface so it is accessible via and reacts like standard PHP file functions. This is done by LocalStream and LocalReadonlyStream. All subclasses of these are only there to define different parts of the local file system.

However, besides this main task, there are some Drupal specific functionality to be provided. Or, what public methods would I expect (besides the stream wrapper functions):
Stream wrapper as a file wrapper:

  • realpath(): I doubt that this will be used a lot (directly), but it doesn't hurt to have it public.
  • dirname(): This is even more doubtful, but it could be used to create another file in the same directory, though we could simply use dirname($wrapper->realpath()) for that.

Stream wrapper as an externally accessible resource:

  • getExternalUrl(): this is of course where these stream wrappers shine and provide additional value above just being a stream wrapper.

As all these stream wrappers are expected to operate within the hierarchical local file system, the only difference is the starting point they define within that local file system and the url that corresponds to that starting point. Given this, what (protected) methods would I expect?
Stream wrapper as a file wrapper:

  • getDirectoryPath(): the starting point within the local file system that this stream wrapper covers. I don't think this name is logical and would like to suggest to rename it to something like getStreamDirectory() or getBaseDirectory(), or getBasePath(). (The latter following a method in Symfony\Component\HttpFoundation\Request.) Note: there is a todo about the naming of this method, but the issue it refers to is long closed.
  • getTarget(): the relative path within the sub file system that this stream wrapper covers. I think that this name can also be better, but looking at file_uri_target(), I can understand the name. Perhaps something like getRelativePath() would be better given that for system streams the first part of the target is also part of what defines the base directory(, or getPathInfo() to follow Symfony\Component\HttpFoundation\Request again).

Stream wrapper as an externally accessible resource:

  • and at the url level, getStreamUrl(): the (base) url that this stream wrapper covers.
  • getTargetUrl(): given that we are working within a hierarchical file system, this will be equal to getTarget(), except perhaps replacing \ with /. The same suggestion here as to the name of this method: getRelativeUrl() would be better.
  • and for the system stream wrappers, getOwner(): returns the first part after the scheme (module or theme name) used for:
  • getOwnerDirectory(): returns the directory where the owner (module, theme or profile) is located.

Now this is quite a change, which will also touch the already existing Local(ReadOnly)Stream. So we can postpone that to a follow-up, but I'm not happy with the current state of it (existing and new code).

What do other people think?

Arla’s picture

I basically agree with your analysis of the design around LocalStream and its descendants. I suppose a new issue will be in order? ("Review API of implemented Stream Wrappers"?)

Regarding the points more relevant to the current issue...

In SystemStream, why does getTarget() check for file_exists() at all? (It seems to have been introduced in #95.) If we can skip that, we won't need to call the parent implementation in getExternalUrl().

I agree with your point about error handling.

I also just realised something else: why don't we use parse_url() for extracting the different parts?

almaudoh’s picture

I suggest the architectural concerns raised here be also reflected at #2028109: Convert hook_stream_wrappers() to tagged services. where some if not all can be addressed. IMO that patch should land before this one.

Arla’s picture

Status: Needs work » Needs review
FileSize
38.46 KB
2.75 KB

Rerolled and fixed #167 stuff.

1) Changed to "not enabled"
2) The reason is that the self implementation does file_exist() and getExternalUrl() wants to avoid that. The current tests expects getTarget() to fail when file does not exist, but expects getExternalUrl() to not care. I am not sure if those expectations are correct.
3) Not sure why the tests didn't mind. Changed it here to getUriForPath() anyway.

Also renamed SystemStreamUnitTest to SystemStreamTest, to reflect that it is a kernel test and not a PHPUnitTest. Interdiff is made with -M (detect renames) for clarity. And I modified it slightly to debug test fails more easily, please tell me if I used a discouraged pattern.

To summarise current status: When this gets comitted, a follow-up issue should review and redefine the API including error handling, generally according to #167 IMHO. Or at least correct the current doc contradictions. The issue referenced in #169 we should be able to treat in parallell, I don't think there is any dependency between these issues.

slashrsm’s picture

Status: Needs review » Needs work

Few minor comment. Otherwise looks OK and almost ready.

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php
    @@ -0,0 +1,45 @@
    +    if (!is_null(drupal_get_filename('profile', $name))) {
    +      return $name;
    

    Could we use \Drupal::moduleHandler()->moduleExists()?

    Also, no need to is_null() as NULL casts to FALSE.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.php
    @@ -0,0 +1,60 @@
    +    if (array_key_exists($name, \Drupal::service('theme_handler')->listInfo())) {
    

    themeExists()?

  3. +++ b/core/modules/system/src/Tests/File/SystemStreamTest.php
    @@ -0,0 +1,415 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function tearDown() {
    +    parent::tearDown();
    +  }
    

    Can be removed if only calls parent.

  4. +++ b/core/modules/system/src/Tests/File/SystemStreamTest.php
    @@ -0,0 +1,415 @@
    +        if (!$this->assertEqual($instance->$method($uri), $info[0], $info[1])) {
    +          debug($instance->$method($uri), get_class($instance) . "::$method('$uri')");
    +          debug($info[0], 'expected');
    +        }
    

    Debug code.

fietserwin’s picture

  1. +++ b/core/modules/system/src/Tests/File/SystemStreamTest.php
    @@ -0,0 +1,415 @@
    +    $theme_handler->enable(array('bartik', 'seven', 'stark'));
    

    enable() does not exist, I think it should be install(), same for disable() below, replace with uninstall().

  2. +++ b/core/modules/system/src/Tests/File/SystemStreamTest.php
    @@ -0,0 +1,415 @@
    +          if (get_class($e) == get_class($info[0])) {
    +            $this->pass($info[1]);
    +          }
    +        }
    

    And what if the exception is not the right type:
    - $this->fail() (will loose the exception type) or
    - rethrow (my preference, we shouldn't be catching it).

    I think we should use is_a() instead of class equality, makes the test more robust against future changes,while remaining a valid test.

  3. +++ b/core/modules/system/src/Tests/File/SystemStreamTest.php
    @@ -0,0 +1,415 @@
    +        if (!$this->assertEqual($instance->$method($uri), $info[0], $info[1])) {
    +          debug($instance->$method($uri), get_class($instance) . "::$method('$uri')");
    +          debug($info[0], 'expected');
    +        }
    +      }
    

    If you want debug info, use this:
    $this->assertEqual($instance->$method($uri), $info[0], String::format("@class::@method('%uri'): @info", array('@class' => get_class($instance), '@method' => $method, '%uri' => $uri, '@info' => $info[1])));

almaudoh’s picture

Assigned: Arla » almaudoh

I had some time yesterday to take a long hard look at this patch, considering also previous comments in #165, #167, #168 and #170. I think the following points are more or less agreed by most commenters:

  1. getTarget(), getDirectoryPath() and getOwnerName() are all internal methods that help in the implementation of the external streamwrapper interfaces and should be protected methods.
  2. The external interfaces of where getTarget() is used, drupal_dirname() for example, don't bother about whether the file exists or not, hence this check is not needed and should be removed. Actually in some places, e.g. file_destination(), it is expected that the file should not exist.
  3. The use of $url parameter in protected method calls is inconsistent and not the best approach. Constructor injection of $url would be better, setter injection would be a less desirable second choice. However, LocalStream::getLocalPath() uses this pattern inconsistently in several places, hence we cannot handle that in this issue. It is possible to remove the ones that were introduced in this patch.
  4. A follow up issue should be raised to address the architectural issues that are beyond the scope of this issue.

Additionally, even though getTarget() for SystemStream wrappers is a slightly different implementation from getTarget() in LocalStream, its purpose is the same - to return the internal path relative to the scheme base path. The implementation is different because SystemStream wrappers need the first component of the path after the '://' to fully define its base path, i.e. public:// is sufficient to define the base path as 'sites/default/files', but module:// is not sufficient, you need module://{module_name}. For this reason, I don't think we have to rename SystemStream::getTarget().

I will rework the patch with above as well as other review comments shortly.

almaudoh’s picture

Assigned: almaudoh » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
38.98 KB
30.38 KB

#172 - fixed 2, 3 and 4. Not sure if ModuleHandler::moduleExists() is the best method to use (it works though) considering that there's ongoing work on the Extension system that may take profile stuff out to a dedicated profile manager/handler. What do others think?

#174 - fixed.

#175 - rescoped getTarget(), getDirectoryPath() and getOwnerName() to protected methods. Removed file_exists() check in getTarget() and also removed all the optional $url parameters except in LocalStream::getLocalPath(), which has to be done in a follow-up.

The changes required re-writing the tests due to the change in visibility of getTarget(), getDirectoryPath() and getOwnerName(). I've removed tests on the protected methods and introduced tests for the public interface methods of StreamWrapperInterface, i.e. dirname() and realpath(). In the process, I fixed another bug that would have caused test failures during future conversions.

Also cleaned up the patch, deleted changes to KernelTestBase which are no longer necessary, and removed dead files / changes that the patch was adding back to the repo.

If we want to test getTarget(), getDirectoryPath() and getOwnerName(), we can write PHPUnit tests for those in a separate issue.

almaudoh’s picture

almaudoh’s picture

pwolanin’s picture

I would like this to go in, though it looks the the implementation is perhaps just a bridge to a full OO implementation later?

Can we totally get rid of drupal_get_path() as public facing and just support module_load_include() for including code? Maybe in a follow-up?

almaudoh’s picture

fietserwin’s picture

  1. +++ b/core/includes/common.inc
    @@ -837,6 +837,9 @@ function drupal_set_time_limit($time_limit) {
    + * This function should only be used when including a file containing PHP code;
    + * the 'module://', 'profile://' and 'theme://' stream wrappers should be used
    + * for other use cases.
      * @param $type
      *   The type of the item; one of 'core', 'profile', 'module', 'theme', or
    

    Empty comment line before @param.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
    @@ -0,0 +1,38 @@
    +  /**
    +   * Gets the module's directory path.
    +   *
    +   * @return string
    +   *   String specifying the path.
    +   */
    +  protected function getDirectoryPath() {
    +    return drupal_get_path('module', $this->getOwnerName());
    

    {@inheritdoc}

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php
    @@ -0,0 +1,42 @@
    +/**
    + * Defines the read-only profile:// stream wrapper for profiles.
    + */
    +class ProfileStream extends SystemStream {
    

    Defines the read-only profile:// stream wrapper for profile files.

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php
    @@ -0,0 +1,42 @@
    +  /**
    +   * Gets the profile's directory path.
    +   *
    +   * @return string
    +   *   String specifying the path.
    +   */
    +  protected function getDirectoryPath() {
    +    return drupal_get_path('profile', $this->getOwnerName());
    

    {@inheritdoc}

  5. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,89 @@
    +use \Drupal\Component\Utility\UrlHelper;
    +
    

    Not used: can be removed.

  6. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,89 @@
    + * This class provides a read-only Drupal stream wrapper base class for system
    + * files such as modules, themes and profiles.
    + */
    

    Not correct english (at the semantic level).

  7. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,89 @@
    +  /**
    +   * Returns a web accessible URL for the resource.
    +   *
    +   * This function should return a URL that can be embedded in a web page
    +   * and accessed from a browser. For example, the external URL of
    +   * "youtube://xIpLd0WQKCY" might be
    +   * "http://www.youtube.com/watch?v=xIpLd0WQKCY".
    +   *
    +   * @param string $uri
    +   *   An optional URI.
    +   *
    +   * @return string
    +   *   Returns a string containing a web accessible URL for the resource.
    +   *
    +   * @throws \InvalidArgumentException
    +   */
    +  public function getExternalUrl() {
    +    $dir = $this->getDirectoryPath();
    

    {@inheritdoc} (will remove the non existing @param error)

  8. +++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.php
    @@ -0,0 +1,66 @@
    +  /**
    +   * Gets the theme's directory path.
    +   *
    +   * @return string
    +   *   String specifying the path.
    +   */
    +  protected function getDirectoryPath() {
    +    return drupal_get_path('theme', $this->getOwnerName());
    

    {@inheritdoc}

  9. +++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.php
    @@ -0,0 +1,66 @@
    +  protected function themeHandler() {
    +    return \Drupal::service('theme_handler');
    +  }
    +}
    

    This is good, we should do this in the module stream as well.

No serious remarks, we are almost there! With this corrected I will RTBC (though I will do some reviewing in the meantime).

fietserwin’s picture

Status: Needs review » Needs work
FileSize
1.94 KB

To get SystemStreamTest working on Windows, I had to make the changes as per the interdiff. Can you add these to your next patch?

Furthermore, I had a hard time figuring out what all the functions return, and if that is including or excluding a leading or trailing slash. So adding some examples in the phpdoc of methods like getLocalPath, getTarget, etc. would be helpful, but I wouldn't mind if you do that only for methods added by this patch.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
32.01 KB
8.46 KB

@fietserwin, thanks for another review :) Patch addresses #184 & #185.

Looking at it, I found this patch only introduces one method getOwnerName(). Tried putting some documentation on that. Any further suggestions? There are no leading or trailing slashes from any of the functions/methods except PHP's dirname() method which leaves a leading slash, so we didn't use the path separator in Systemstream::dirname()

return rtrim($scheme . '://' . $this->getOwnerName() . $dirname, '/');

We can properly clean-up the code in LocalStream and LocalReadOnlyStream in the follow-ups.

This should be RTBC now.

fietserwin’s picture

Status: Needs review » Needs work

Thanks for updating the patch. 1 more (last) nitpick: if you document a @return for the wrappers you introduced, we have code completion (and checking and parameter lists) when using a proper IDE, plus that the standard is to fully document type info.

garphy’s picture

@fietserwin, regarding #187, which @return are still not properly documented in proposed patch ?

fietserwin’s picture

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
    @@ -0,0 +1,42 @@
    +  /**
    +   * Wraps the ModuleHandler service.
    +   */
    +  protected function moduleHandler() {
    +    return \Drupal::moduleHandler();
    +  }
    +}
    

    1

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,88 @@
    +  /**
    +   * Wraps the drupal request.
    +   */
    +  protected function request() {
    +    return \Drupal::request();
    +  }
    +
    

    2

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.php
    @@ -0,0 +1,70 @@
    +  /**
    +   * Gets the currently active theme.
    +   */
    +  protected function getActiveTheme() {
    +    return \Drupal::service('theme.negotiator')->determineActiveTheme(\Drupal::service('current_route_match'));
    +  }
    +
    

    3

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.php
    @@ -0,0 +1,70 @@
    +  /**
    +   * Wraps the drupal configuration service.
    +   */
    +  protected function config($config_name) {
    +    return \Drupal::config($config_name);
    +  }
    +}
    

    4

JeroenT’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
32.23 KB

I documented the @returns as suggested by fietserwin in comment 189.

Patch attached.

fietserwin’s picture

Status: Needs review » Needs work

Thanks JeroenT, but

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.php
    @@ -0,0 +1,75 @@
    +   * @return mixed
    +   */
    

    @return string|null (according to the documentation on \Drupal\Core\Theme\ThemeNegotiatorInterface::determineActiveTheme())

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.php
    @@ -0,0 +1,75 @@
    +   * @param $config_name
    +   * @return \Drupal\Core\Config\Config
    +   */
    

    Needs an empty line before the @return line as per the documentation standards.

garphy’s picture

I'm taking care of it.

garphy’s picture

Status: Needs work » Needs review
FileSize
32.4 KB
1.27 KB

Took care of the comment issues mentioned in #191.

garphy’s picture

Status: Needs work » Needs review

SQLSTATE[08004] [1040] Too many connections

Let's trigger it one more time. Drupalcon folks are all either gone drinking or sleeping, right now.

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

Good to go!

slashrsm’s picture

Issue tags: +Amsterdam2014

Yay!

almaudoh’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs review

I'm not sure about this one, but I've been having trouble articulating why. Discussing the issue with people at Amsterdam helped a bit, trying to summarize below.

Concerns are these:

1. drupal_get_path() is horrible and fragile, but the end result of the DX improvement is that it still gets called.

2. Using the stream wrappers hides the dependency tree. Code using dependency injection could presumably use the extension handler and https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension... (or if it can't, that feels like an omission that we should support.

3. There are some drupal_get_path() uses in core that should be __DIR__, even the example in the issue summary could be done using __DIR__.

https://api.drupal.org/api/drupal/core%21modules%21book%21book.module/fu... is an example where the stream wrapper would be an inappropriate conversion. Also see #2248149: Use __DIR__ instead of drupal_get_path() for local PSR-0/PSR-4 directories..

That doesn't mean I think the stream wrappers have no use case, but it's a much, much more restricted use case than the issue summary suggests at the moment from what I can see.

almaudoh’s picture

#202.1: This issue and #2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname() will help in limiting the usage of drupal_get_path(), so that a possible follow-up to address drupal_get_path() brittleness would be less disruptive to commit.

#202.2: A related issue #2028109: Convert hook_stream_wrappers() to tagged services. will turn stream_wrappers into services which would allow dependency injection. I'm not sure if that should go in before this one...
Is Extension::getPath() a suitable replacement for drupal_get_path()? That would fix #202.1 easily. What do others think?

#202:3: Raised new issue #2351919: Replace uses of drupal_get_path() with __DIR__ where possible. This while current issue will focus on the use cases where __DIR__ won't work (mostly modules that use other modules' resources).

That doesn't mean I think the stream wrappers have no use case, but it's a much, much more restricted use case than the issue summary suggests at the moment from what I can see.

Will update issue summary.

Edit: update for more clarity.

fietserwin’s picture

Status: Needs review » Needs work

I can agree with the responses in #203 to the valid objections/concerns in #202.

Is Extension::getPath() a suitable replacement for drupal_get_path()?

I guess that would be as easy as to replace

+++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
@@ -0,0 +1,44 @@
+  protected function getDirectoryPath() {
+    return drupal_get_path('module', $this->getOwnerName());
+  }
+

with (and, if necessary, add proper exception handling to meet the method specification):

return $this->moduleHandler()->getModule($this->getOwnerName())->getPath();

I guess (hope) that other extensions have a similarly easy solution.

With a new patch containing this change and the follow-up issues defined, it can be set to RTBC again.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
32.47 KB
2.16 KB

Profiles are still using old procedural code :(

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

For me, this patch is about adding a feature that might be used to replace (some usages of) drupal_get_path(), but is not about actually replacing it, therefore we have the follow up issues.

Too bad that profiles are not like other extensions, but that is also not the point of this issue. Are there any issues that try to make profiles behave like an extension or are they really different and dothey only share a few things such as that they have a storage location (on a hierarchical file system)?

catch’s picture

Status: Reviewed & tested by the community » Needs work

The issue summary still needs updating, and we need to know what the valid use-cases are so they can be put in a change notice.

Quick review found the following. Can't promise this is everything.

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php
    @@ -0,0 +1,40 @@
    +    if (!is_null(drupal_get_filename('profile', $name))) {
    

    This can also use the module handler no?

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php
    @@ -0,0 +1,40 @@
    +    return dirname(drupal_get_filename('profile', $this->getOwnerName()));
    

    And here?

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,90 @@
    +  public function getExternalUrl() {
    

    Is there ever a valid use case for this?

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,90 @@
    +  protected function request() {
    

    If we can't use constructor injection that's OK, but what about setter injection of the request stack. Not familiar enough with stream wrappers to know how doable that is.

fietserwin’s picture

RE #207.1/2:
It seems that profiles are NOT extensions and as such Extension::getPath() cannot be used for profiles. I am not aware of another solution.

RE #207.3:
Short answer: this is existing functionality, mainly used for the public stream wrapper.

Long answer: I think this is the crux of this feature. Image, css, js and other resources that live in the directory structure of an extension (or profile or library) are often needed by functionality from other modules or themes. These resources can be needed internally, i.e. as a file, or externally, e.g. in a link, style or img tag on a rendered page.

Ideally, if they are needed as file, just passing the wrapper should suffice, and it does in most cases (base php file systems accepts stream wrappers). So the realpath() method on the StreamWrapperinterface should be superfluous. Unfortunately, not all functions accept a stream wrapper, e.g. exif_read_data() doesn't. So realpath() is also necessary (and the wrapper drupal_realpath()).

OTOH, as web url, stream wrappers cannot be used, unless we would implement a router/controller for that to map an url like http://www.example.com/public/my-image.jpg to the file /document_root/sites/default/files/my-image.jpg. However, performance wise that is not the way to go. As existing files should preferably be handled by the web server, not php. (aside: it could be an idea when we want to move code outside the web root.) Thus, if some module wants to add ckeditor.js to a page, it would be nice if it could do by just attaching 'module://ckeditor/js/ckeditor.js' and let the mapping be done by the attacher.

It should indeed be better documented that this is 1 of the main use cases: add resources from other modules to a page.

RE #207.4:
I am not sure, but I think this will be hard. Many times, the stream wrapper will be instantiated by PHP, e.g. when calling fopen('module://image/sample.png') and subsequently stream_open('module://image/sample.png') gets called. So no time or place to inject anything.

@almaudoh: Can you update the issue summary? Can we already prepare a change notice?

almaudoh’s picture

@catch: #201.1/2: Currently, profiles are still being handled like modules, but ModuleHandler::getModule('profile_foo') fails with Exception "module profile_foo not found", so I couldn't use that. Maybe @sun can help out here.

Also re #201.4: #2028109: Convert hook_stream_wrappers() to tagged services. will make it very easy to inject $request, $module_handler, etc. in the constructor. If that goes in first, then we could re-roll this patch to inject dependencies correctly. If this patch, goes in first, we can do it in a follow-up or expand the scope of that one when it's re-rolled.

I took some time to look at D8 core with respect to use cases. There are 4 possible use cases:

Asset files attached in PSR-4/PSR-0 classes:

In a \Drupal\foo_module\Form\FooEntityForm where assets are attached, $form['#attached']['css'][] = 'module://foo_module/js/foo_bar.js'; is a far better DX than $form['#attached']['css'][] = __DIR__ . '/../../js/foo_bar.js'; or even $form['#attached']['css'][] = drupal_get_path('module', 'foo_module') . '/../../js/foo_bar.js';

Modules that provide resources consumed by other modules or themes

  • color module
  • ckeditor module

Modules that use resources provided by other modules

Mainly in contrib, these would utilize js, css and other files from modules in core.

Hooks for asset management

hook_library_info_alter(), hook_library_alter(), hook_css_alter(), etc. existing examples in system.api.php

The real benefit of this issue is the DX win of using "module://foo_module/js/foo_bar.js" which makes it easier to understand the code.

almaudoh’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated issue summary

almaudoh’s picture

Status: Needs work » Needs review

Raised draft change notice: https://www.drupal.org/node/2352923.

Other change notices that may also need update once this is committed:
https://www.drupal.org/node/2201089
https://www.drupal.org/node/2169605
https://www.drupal.org/node/2235431
https://www.drupal.org/node/2090637
https://www.drupal.org/node/1876600
https://www.drupal.org/node/1876152

If catch still feels strongly about the setter injection, I'll put in another patch. But that will likely go away again once we reroll #2028109: Convert hook_stream_wrappers() to tagged services. (or this patch if that one goes in first)

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

@almaudoh: I don't see any injection in the public and private stream wrappers in #2028109: Convert hook_stream_wrappers() to tagged services. and as explained, I don't see how that should work?!

The change notices:
- 2201089, 1876600, 1876152: can already be changed as these are the __DIR__ cases (or can be changed when #2351919: Replace uses of drupal_get_path() with __DIR__ where possible gets in).
- 2169605 should distinguish the asset in own module versus other module's assets cases.
- others are all use cases for this issue's new features.

@catch: I hope that #208 to #211 answered your concerns.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Asset files attached in PSR-4/PSR-0 classes:
In a \Drupal\foo_module\Form\FooEntityForm where assets are attached, $form['#attached']['css'][] = 'module://foo_module/js/foo_bar.js'; is a far better DX than $form['#attached']['css'][] = __DIR__ . '/../../js/foo_bar.js'; or even $form['#attached']['css'][] = drupal_get_path('module', 'foo_module') . '/../../js/foo_bar.js';

I actually prefer

$form['#attached']['css'][] = __DIR__ . '/../../js/foo_bar.js'; 

Using the stream wrapper here results in a dependency on the module handler and stream wrapper system in that class, that you can't figure out without finding the module:// string - very easy to miss. With __DIR_ . '/../../../whatever' there's no dependency at all. It's a bit uglier but it's very self-documenting.

Modules using resources from other modules, they could do the following:

+++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
@@ -0,0 +1,45 @@
+    return $this->moduleHandler()->getModule($this->getOwnerName())->getPath();

If we had a helper like $this->moduleHandler->getPathForModule($module); that could be used for the 'assets provided by another module' and is only slightly more verbose than the stream wrapper, as well as showing the dependency more explicitly.

alexpott’s picture

I think the dependency point raised by @catch in #213 is a good one. Also I'm wondering about whether or not we should all just be using the library system more. If you need to assets that depend on something from another module should should just make a library that has is dependent on that module's library that has the assets you need and attach your library. No?

Wim Leers’s picture

Also I'm wondering about whether or not we should all just be using the library system more. If you need to assets that depend on something from another module should should just make a library that has is dependent on that module's library that has the assets you need and attach your library. No?

Yes!

At DrupalCon Amsterdam, catch and I discussed the possibility of removing the ability to add individual CSS and JS assets altogether, and only keeping libraries. Because only with libraries, you can specify dependencies. And because then it's a much more consistent system.
catch would still like to see that happen.

Perhaps you do too, alexpott?

almaudoh’s picture

Status: Needs work » Needs review
FileSize
35.25 KB

#2028109: Convert hook_stream_wrappers() to tagged services. just went in, attached patch was already in my repo.

Following #213, #214 and #215, the other use-cases not yet discussed (#2347783-8: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname()):

- other .yml: 2 usages: replace with module://...?
- xml files: 4 usages: replace with module://...?

effulgentsia’s picture

That doesn't mean I think the stream wrappers have no use case, but it's a much, much more restricted use case than the issue summary suggests at the moment from what I can see.

I agree with #214 that ideally all CSS and JS attachment should be done via libraries, so those are not great use cases for this issue. I think therefore the primary use case of this issue would be images and other media files. For example, in template_preprocess_system_themes_page():

$current_theme['screenshot'] = array(
  '#theme' => 'image',
  '#uri' => drupal_get_path('module', 'system') . '/images/no_screenshot.png',
...
);

could become:

$current_theme['screenshot'] = array(
  '#theme' => 'image',
  '#uri' => 'module://system/images/no_screenshot.png',
...
);

which has the added benefit of making the value of the #uri property an actual URI.

jhedstrom’s picture

Issue tags: +Needs reroll
mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
35.05 KB

Largely there's a change as function drupal_get_path() moved to bootstrap. Other minor stuff, but basically just a re-roll.

slashrsm’s picture

Issue tags: +D8Media
mbovan’s picture

Rerolled, replaced calls to removed functions, quick fixes, code style updates...
As the previous patch failed, I'm expecting the same with this one.

claudiu.cristea’s picture

No, unfortunately we cannot inject services in a stream wrapper constructor because stream_wrapper_register() doesn't allow that. We can only pass the class name to the registering function. But I still kept the constructors because they can be swapped in a possible unit test.

What I've done:

  1. Reverted some out-of-scope changes from #225.
  2. Reworked the SystemStreamTest to make it more simple to be read. I removed the assertion messages because we don't use anymore messages in assertions

.

Note

I really don't like the idea of having the pseudo-URIs like: profile://current, theme://current, theme://default and theme://admin. I would create separate stream wrappers for such cases:

  • current-profile://
  • current-theme://
  • default-theme://
  • admin-theme://
claudiu.cristea’s picture

Title: Add module://, theme:// and profile:// stream wrappers to access system files » Add stream wrappers to access system files
FileSize
40.03 KB
27.05 KB

Are 'current', 'default' and 'admin' blacklisted on profiles and theme names? No. There's no reason to make such exceptions as profile://current, theme://current, theme://default and theme://admin. That's why I introduced in this patch 4 new schemes and also fixing their names (theme should be active, not current and profile should be installed, not current):

  • installed-profile://
  • active-theme://
  • default-theme://
  • admin-theme://

Improved the tests.

tim.plunkett’s picture

fietserwin’s picture

With the possible name clashes you have a point, but on first sight I don't like that the resource part can now begin after the 1st or 2nd /. and yes that is different from the currently defined ones (public, private, temporary) but they serve different purposes.

We had similar discussions from #98 to #122 and there was a majority for sticking to as it was ...

claudiu.cristea’s picture

I don't like that the resource part can now begin after the 1st or 2nd /. and yes that is different from the currently defined ones (public, private, temporary) but they serve different purposes

I'm totally missing the point here :)

I read the posts between #98 and #122 and they are about a totally different thing. Having an unified drupal:// (or system://) scheme vs. 3 different (module, theme, profile).

stevenlafl’s picture

More than 4 years and this still hasn't been fixed. People must really really love typing drupal_get_path('module', 'theme_test') . '/css/base-override.css' every time they want to reference a file in their theme/module/etc.

Suffice it to say I'm pretty damned disappointed. Can't we backport this to D7?

garphy’s picture

More than 4 years and this still hasn't been fixed. People must really really love typing drupal_get_path('module', 'theme_test') . '/css/base-override.css' every time they want to reference a file in their theme/module/etc.

Well... the reviewers must surely not have gained pretty much confidence that the issue is handled well. I did not see your review of the patch in the thread, though...
So... If you have time, feel free to check out the patch, test it and post your review.

Suffice it to say I'm pretty damned disappointed. Can't we backport this to D7?

Can you ?
So.. again... if your have time, feel free to work on a patch.

Wim Leers’s picture

#233: that specific use case will be a thing of the past in D8. Thanks to asset libraries. See e.g. https://www.drupal.org/theme-guide/8/assets.

twistor queued 228: 1308152-228.patch for re-testing.

pounard’s picture

Just a review of the patch, I think that DIRECTORY_SEPARATOR should not be used. If you build URI/URL with that, if windows tells you that separator is \ it will give you invalid URLs. As far as I know, if I remember correctly, windows gives you '/' but hey, that's not a directory separator if want to build URLs, but it is a whatever they call it in the RFC. Anyway, stricly PHP-speaking, whatever is the OS you're running in, '/' is *always* a valid directory separator. DIRECTORY_SEPARATOR should only be used when parsing filenames coming from the underlaying OS, but not be used in order to build URLs for the external world; in this case, my little finger tells me that the getLocalPath() method will serve URIs in the HTML too.

jonhattan’s picture

Status: Needs review » Needs work

#237 is absolutely right

claudiu.cristea’s picture

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

I still think this can go in 8, in 8.1.x.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
38.41 KB
2.27 KB

Rerolled and applied #237.

Wim Leers’s picture

joelpittet’s picture

There is lots of nice code cleanup in this:)
+1 to getting these stream wrappers!

+++ b/core/lib/Drupal/Core/StreamWrapper/PseudoThemeStreamBase.php
@@ -0,0 +1,23 @@
+abstract class PseudoThemeStreamBase extends PseudoSystemStreamBase {

Does it need 'Pseudo' in the Base class names? We seem to have a convention where our abstract Base classes just end in Base. Just a nitpick but it caught my eye when skimming the patch.

claudiu.cristea’s picture

@Wim Leers, added #2611246: [PP-1] Document the recommended method of creating file URLs to theme assets (e.g. images) as related issue.

@joelpittet, you're right. Fixed.

The problem with issue is that everybody wants this feature but there's no time for a good review and RTBC :)

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +TX (Themer Experience)

So, here's a review then :)

I have serious concerns about most of the stream wrappers besides module:// and theme://. profile:// and installed-profile:// seem pretty useless, and potentially make code less portable.

IMO this should be reduced in scope, to only do module:// and theme://. For those, there are no risks.

Also, the patch does not remotely match the IS.

  1. +++ b/core/core.services.yml
    @@ -1242,6 +1242,34 @@ services:
    +  stream_wrapper.active_theme:
    +    class: Drupal\Core\StreamWrapper\ActiveThemeStream
    +    tags:
    +      - { name: stream_wrapper, scheme: active-theme }
    

    Why do we need this?

    This means the result of this effectively varies by the theme cache context. But none of this code and calling code is designed to have URLs return different values based on the context.

    Therefore I fear this will cause mysterious problems that are tricky to debug.

  2. +++ b/core/core.services.yml
    @@ -1242,6 +1242,34 @@ services:
    +  stream_wrapper.admin_theme:
    +    class: Drupal\Core\StreamWrapper\AdminThemeStream
    +    tags:
    +      - { name: stream_wrapper, scheme: admin-theme }
    +  stream_wrapper.default_theme:
    +    class: Drupal\Core\StreamWrapper\DefaultThemeStream
    +    tags:
    +      - { name: stream_wrapper, scheme: default-theme }
    

    Similarly, these depend on configuration. Hence the generated file URLs will need to have the appropriate cache tag associated. So, this too is brittle.

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/ActiveThemeStream.php
    @@ -0,0 +1,59 @@
    +    return $this->themeNegotiator->determineActiveTheme($this->routeMatch);
    

    Uses theme negotiator, hence needs theme cache context.

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/AdminThemeStream.php
    @@ -0,0 +1,51 @@
    +    return $this->configFactory->get('system.theme')->get('admin');
    

    Uses config, hence needs cache tag.

  5. +++ b/core/lib/Drupal/Core/StreamWrapper/DefaultThemeStream.php
    @@ -0,0 +1,51 @@
    +    return $this->themeHandler->getDefault();
    

    Depends on configuration, hence needs cache tag.

  6. +++ b/core/lib/Drupal/Core/StreamWrapper/InstalledProfileStream.php
    @@ -0,0 +1,44 @@
    +    return drupal_get_profile();
    

    Cannot ever change, hence is fine.

    (Though of questionable utility IMO.)

  7. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,101 @@
    +abstract class SystemStream extends LocalReadOnlyStream {
    +  /**
    

    Missing newline in between.

  8. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStream.php
    @@ -0,0 +1,101 @@
    +  protected function getOwnerName() {
    +    $uri_parts = explode('://', $this->uri, 2);
    +    if (count($uri_parts) === 1) {
    +      // The delimiter ('://') was not found in $uri, malformed $uri passed.
    +      throw new \InvalidArgumentException(sprintf('Malformed uri parameter passed: %s', $this->uri));
    +    }
    +
    +    // Remove the trailing filename from the path.
    +    $length = strpos($uri_parts[1], '/');
    +    return ($length === FALSE) ? $uri_parts[1] : substr($uri_parts[1], 0, $length);
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getTarget($uri = NULL) {
    +    return rtrim(strstr(parent::getTarget($uri), '/') ?: '', '/');
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   *
    +   * @throws \InvalidArgumentException
    +   */
    +  public function getExternalUrl() {
    +    $dir = $this->getDirectoryPath();
    +    if (empty($dir)) {
    +      throw new \InvalidArgumentException(sprintf('Extension directory for %s does not exist.', $this->uri));
    +    }
    +    return $this->request->getUriForPath(base_path() . $dir . $this->getTarget());
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function dirname($uri = NULL) {
    +    if (!isset($uri)) {
    +      $uri = $this->uri;
    +    }
    +    list($scheme) = explode('://', $uri, 2);
    +    $target = $this->getTarget($uri);
    +    $dirname = dirname($target);
    +
    +    if ($dirname === '.' || $dirname === '\\') {
    +      $dirname = '';
    +    }
    +
    +    return rtrim($scheme . '://' . $this->getOwnerName() . $dirname, '/');
    +  }
    

    Woah. This needs good unit test coverage.

  9. +++ b/core/lib/Drupal/Core/StreamWrapper/SystemStreamBase.php
    @@ -0,0 +1,55 @@
    +abstract class SystemStreamBase extends SystemStream {
    

    As does this (need good unit test coverage).

claudiu.cristea’s picture

@Wim Leers,

I think profile:// and installed-profile:// can be merged as profile:// and that should refer to the installed profile. I cannot see use-cases where you want to access files from a non-installed profile. But for the installed profile I can see the need of having such a wrapper. About active-theme://, admin-theme://, default-theme:// —­ I agree that they will overlap as resolved URLs with theme://. Are we OK to keep only module://, theme:// and profile:// (as installed profile)?

catch’s picture

I'm still a bit perplexed by this patch. It doesn't do anything to untangle the mess that is drupal_get_filename(), so that makes it only useful for DX.

But drupal_get_path() is called less and less in 8.x, so the DX benefit is extremely narrow.

For example, we're down to 103 calls to drupal_get_path() in core from 177 in Drupal 7 https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...

Most of the remaining usages are in tests. Many of the core usages could probably be converted to Extension::getFileName() or Extension::getPath() rather than the stream wrapper. Or in some cases just use __DIR__ since they're referencing the module's own files.

103 calls is still quite a lot, but I'd rather see an issue that removes as many calls to those without the stream wrappers being available. I'm concerned that once this gets in, there'd be a mass conversion from drupal_get_path() to the stream wrapper which is not necessarily the right direction.

If we remove all the calls that are already superceeded, and there's still loads left, then the scope of the DX improvement here becomes a lot clearer.

Wim Leers’s picture

I postponed #2611246: [PP-1] Document the recommended method of creating file URLs to theme assets (e.g. images) on this.


#245:

I cannot see use-cases where you want to access files from a non-installed profile

Good point!


#246: See #2611246: [PP-1] Document the recommended method of creating file URLs to theme assets (e.g. images), that IMO is a strong use case. Unless you see another elegant solution there.

almaudoh’s picture

#247 I agree that file URLs may be the one strong use case that justifies this issue. In the theming system we are working around similar limitations using @my_theme/css/...-style dynamic paths, which IMHO is not a very good solution because it requires custom code each time (see #2642122-4: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides for a case that streamwrappers would elegantly fix).

In contrib world, this would be a very useful feature to avoid all sorts of bugs.

OTOH, I agree that parts (mostly PHP code) where drupal_get_path(), etc can be replaced with __DIR__, Extension::getFileName() or Extension::getPath() should be removed from the scope of this issue. There's an old issue #2351919: Replace uses of drupal_get_path() with __DIR__ where possible where we can do that.

Wim Leers’s picture

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -Media Initiative +Needs tests
FileSize
28.3 KB
52.45 KB

Applied #245. Still needs unit tests

claudiu.cristea’s picture

Status: Needs work » Needs review

Bot error.

catch’s picture

Status: Needs review » Postponed

Per #246 and #248 postponing on #2351919: Replace uses of drupal_get_path() with __DIR__ where possible. Once that's done we can see what's left.

almaudoh’s picture

@claudiu.cristea, thanks for keeping this issue alive. Quick review:

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
    @@ -142,6 +116,13 @@ protected function getLocalPath($uri = NULL) {
    +
    +    // Windows accepts either backslash ('\') or slash (UNIX-style '/') as
    +    // directory separator. We'll switch to slash, if the previous realpath()
    +    // ran on Windows platform.
    +    //$realpath = str_replace(DIRECTORY_SEPARATOR, '/', $realpath);
    +    //$directory = str_replace(DIRECTORY_SEPARATOR, '/', $directory);
    +
    

    Do we need this hunk?

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php
    @@ -0,0 +1,45 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getOwnerName() {
    +    return drupal_get_profile();
    +  }
    +
    

    This implementation ignores whatever the user specifies as the profile name in profile://profile_name/path/to/file which would result in wierd behavior. We should use module handler to load the named profile here instead.

  3. +++ b/core/modules/system/tests/src/Kernel/File/ExtensionStreamTest.php
    @@ -0,0 +1,218 @@
    +  /**
    +   * Test Invalid stream uri.
    +   */
    +  public function testInvalidStreamUriException() {
    +    $bad_uris = [
    +      'invalid/uri',
    +      'invalid_uri',
    +      'module/invalid/uri',
    +      'module/invalid_uri',
    +      'module:invalid_uri',
    +      'module::/invalid/uri',
    +      'module::/invalid_uri',
    +      'module//:invalid/uri',
    +      'module//invalid_uri',
    +      'module//invalid/uri',
    +    ];
    ...
    +  public function testStreamWrapperMethods() {
    +    foreach ($this->streamTestCasesProvider() as $uri => $expectation) {
    +      foreach ($expectation as $method => $expected) {
    +        list($scheme, ) = explode('://', $uri);
    +        $this->streamWrappers[$scheme]->setUri($uri);
    +        if ($expected instanceof \InvalidArgumentException) {
    +          /** @var \InvalidArgumentException $expected */
    +          $message = sprintf('Exception thrown: \InvalidArgumentException("%s").', $expected->getMessage());
    +          try {
    +            $this->streamWrappers[$scheme]->$method();
    ...
    +  /**
    +   * Provides test cases for testThemeStream().
    +   *
    +   * @return array
    +   *   Associative array with test cases as values, keyed by uri.
    +   */
    +  protected function streamTestCasesProvider() {
    +    $base_url = $this->container->get('request_stack')->getCurrentRequest()->getUriForPath(base_path());
    +    return [
    

    So now that we're using KernelTestBase(NG), we can use proper PHPUnit @dataProvider in this test.

  4. +++ b/core/modules/system/tests/src/Kernel/File/ExtensionStreamTest.php
    @@ -0,0 +1,218 @@
    +        $this->fail(new FormattableMarkup('Invalid uri %uri not detected.', ['%uri' => $bad_uri]));
    +
    +      }
    

    Nitpick: blank line

  5. +++ b/core/modules/user/src/Entity/User.php
    @@ -573,4 +573,8 @@ public static function getAllowedConfigurableLanguageCodes() {
    +  //public function getCacheTags() {
    +  //  return ['session'] + parent::getCacheContexts(); // TODO: Change the autogenerated stub
    +  //}
    +
    
    +++ b/core/modules/user/src/UserStorage.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Cache\Cache;
     use Drupal\Core\Database\Connection;
    
    @@ -16,6 +17,7 @@
     use Drupal\Core\Language\LanguageManagerInterface;
    +use Drupal\user\Entity\User;
     use Symfony\Component\DependencyInjection\ContainerInterface;
    

    These look out of scope.

claudiu.cristea’s picture

Status: Postponed » Needs review
FileSize
28.09 KB
20.1 KB

@almaudoh, thank you for review.

#255.2

This implementation ignores whatever the user specifies as the profile name in profile://profile_name/path/to/file which would result in wierd behavior. We should use module handler to load the named profile here instead.

profile:// is only for the current profile. There's hard to find a use case for referring files from non-instaleld profiles. See #245 and #247.

Still needs tests.

@catch, I'm gonna set this to "Needs review" only to see the tests. After that I will revert back to Postponed.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
28.02 KB
3.15 KB

Fix the test provider.

almaudoh’s picture

Ha! Was trying to fix the test fails and you beat me to it :). So here are some small nitpicks that were left...

almaudoh’s picture

Status: Needs review » Postponed

Back to postponed...

Wim Leers’s picture

Status: Postponed » Needs review
claudiu.cristea’s picture

Issue tags: +Needs reroll

Yes, nut the reroll is not straight.

alvar0hurtad0’s picture

Status: Needs work » Needs review
FileSize
27.28 KB

Here's the reroll, now the color_test.module file not exists sice this issue #2655580: Dead code: hook_system_theme_info() removed but is still implemented

almaudoh’s picture

Issue tags: -Needs reroll
almaudoh’s picture

Can we make a case for this patch to be committed now on the strength of cases like #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides

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

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

benjy’s picture

I really like this idea, a few points:

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/ExtensionStreamBase.php
    @@ -0,0 +1,113 @@
    +    $uri_parts = explode('://', $this->uri, 2);
    +    if (count($uri_parts) === 1) {
    +      // The delimiter ('://') was not found in $uri, malformed $uri passed.
    +      throw new \InvalidArgumentException("Malformed uri parameter passed: {$this->uri}");
    +    }
    

    This seems like a funny place to validate the uri, it wasn't passed into this method and its a class property, can we validate in the setter?

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStreamTrait.php
    @@ -0,0 +1,72 @@
    +    $uri_parts = explode('://', $uri, 2);
    +    if (count($uri_parts) === 1) {
    +      // The delimiter ('://') was not found in $uri, malformed $uri passed.
    +      throw new \InvalidArgumentException("Malformed uri parameter passed: $uri");
    +    }
    

    Same validation is happening here.

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php
    @@ -0,0 +1,38 @@
    +  protected function getOwnerName() {
    +    return drupal_get_profile();
    +  }
    

    Just checking, so it only works with the "enabled" profile?

deviantintegral’s picture

Agreed with the review comments from #268.

profile:// is explicitly for the enabled profile as per #245.

I'm not sure if LocalStreamTrait should be a trait. Both methods work with a uri property, so would it be better as a LocalStreamUri class? Also, the trait is missing a phpdoc comment.

This code feels like a good amount of it could be covered by unit tests, instead of just functional tests.

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
    @@ -0,0 +1,68 @@
    +      throw new \InvalidArgumentException("Module $name does not exist or is not installed");
    

    Should this be \RuntimeException, or a subclass of it? Same question in the ThemeStream.

  2. +++ b/core/modules/system/tests/src/Kernel/File/ExtensionStreamTest.php
    @@ -0,0 +1,259 @@
    +   *   The uri to be tested,
    

    Period here instead of a comma?

    Also, there's a few places where uri / url is lowercased in the docs text, and they should probably be uppercased.

claudiu.cristea’s picture

I added a use-case to replace the color test removed in #264.

#268.1,2: Moved in setter. But because there's a lot of code duplication between LocalStream and ReadOnlyStream I added a base class for all wrappers: StreamWrapperBase that provides the common properties and methods.

#269: The reason why that is a trait is because those 2 methods are used in LocalStream, then are overridden in ExtensionStreamBase and, finally, used again in ProfileStream. I didn't want to duplicate the code in ProfileStream.

I don't think those are \RuntimeException, but this is not very string belief. Other opinions? Also I see uri/ul with lowercases around, in streams. If it's wrong I would let that to a followup to cover all cases. Thank you for review.

claudiu.cristea’s picture

deviantintegral’s picture

My main thought behind \RuntimeException is that it's dependent on modules being enabled or disabled, which is a runtime state. \InvalidArgumentException makes more sense to me in cases where an argument is never going to be correct.

The trait makes sense to me now, I think it's fine to keep as is.

Here's a small update that fixes calling dirname() with a $uri parameter as in drupal_dirname().

For anyone interested, I've backported this patch (currently #264) over to https://www.drupal.org/project/system_stream_wrapper so we can support a library:// stream wrapper there in existing sites. My aim is to keep that as close to identical to this patch as I can, so that if this lands in 8.2 sites can just uninstall that module and use the core implementation.

juampynr’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStreamTrait.php
    @@ -0,0 +1,75 @@
    + * @file
    

    @file statements were recently removed from core. See #2665992: @file is not required for classes, interfaces and traits

  2. +++ b/core/modules/system/tests/src/Kernel/File/ExtensionStreamTest.php
    @@ -0,0 +1,273 @@
    +  public function providerInvalidUris() {
    

    Is this method name correct? Shouldn't it be providerInvalidStreamUri?

juampynr’s picture

+++ b/core/modules/system/tests/src/Kernel/File/ExtensionStreamTest.php
@@ -0,0 +1,273 @@
+   * @dataProvider providerInvalidUris

Ah, ignore number 2 in my previous comment. I see the annotation now.

juampynr’s picture

Issue summary: View changes
FileSize
225.46 KB

While testing this patch along with Moment.js module, I am getting the following at the Status report:

Warning: file_exists(): Unable to find the wrapper "library" - did you forget to enable it when you configured PHP? in moment_requirements() (line 24 of modules/contrib/moment/moment.install).
moment_requirements('runtime')

Here a screenshot of the debugger. Note that the stream wrapper URL that moment module defines is correct:

Moment requirements

I am debugging why the stream wrappers are not getting registered.

juampynr’s picture

Right, I can see that the StreamWrapperManager recognizes them. They are simply not getting registered:

juampy@Juampy/var/www/drupal8(8.2.x)$ drush core-cli
Psy Shell v0.7.2 (PHP 5.6.11-1ubuntu3.1 — cli) by Justin Hileman
>>> $swm = \Drupal::service('stream_wrapper_manager');
=> Drupal\Core\StreamWrapper\StreamWrapperManager {#83
     +"_serviceId": "stream_wrapper_manager",
   }
>>> $swm->getWrappers();
=> [
     "public" => [
       "type" => 29,
       "class" => "Drupal\Core\StreamWrapper\PublicStream",
     ],
     "temporary" => [
       "type" => 13,
       "class" => "Drupal\Core\StreamWrapper\TemporaryStream",
     ],
     "module" => [
       "type" => 5,
       "class" => "Drupal\Core\StreamWrapper\ModuleStream",
     ],
     "theme" => [
       "type" => 5,
       "class" => "Drupal\Core\StreamWrapper\ThemeStream",
     ],
     "profile" => [
       "type" => 5,
       "class" => "Drupal\Core\StreamWrapper\ProfileStream",
     ],
   ]
>>> 
juampynr’s picture

Oh, now I see, #272 does not implement the LibraryStream. Is that intended @deviantintegral? I see that there has been some discussion about it, but I am not clear on what is the decision.

almaudoh’s picture

@juampynr library: streamwrapper isn't part of the scope of this issue. There is a different issue for that: #1702958: Add libraries:// stream wrappers to access system files .

juampynr’s picture

Status: Needs work » Needs review
FileSize
6.1 KB
36.71 KB

@almaudoh, yes I just saw the issue and I don't understand why it is out of scope. How could then modules share third party libraries in Drupal 8?

I went ahead and ported the library stream wrapper out of https://www.drupal.org/project/system_stream_wrapper into this patch. I tested it against @deviantintegral's Moment.js sandbox and verified that it works (I had to make a small adjustment at the sandbox module, though).

If this does not make sense, we can just continue working on the patch at #272.

juampynr’s picture

Here I have removed the @file docblocks.

deviantintegral’s picture

Adding the library stream wrapper makes sense to me. This patch:

  • Adds test coverage for the library stream wrapper
  • Fixes the compact() call which was actually skipping a bunch of tests
  • Converts \InvalidArgumentException to \RuntimeException for cases where extensions aren't available or installed

Status: Needs review » Needs work

The last submitted patch, 281: add_stream_wrappers_to-1308152-281.patch, failed testing.

Wim Leers’s picture

?!??

I don't understand the recent changes at all. Is this trying to get Libraries API into core in a hidden way? There is no such thing as "library extensions". This feels like massive scope creep.

almaudoh’s picture

I agree with Wim, libraries: is out of scope. This issue is about stream wrappers for extensions. Libraries are not yet extensions (though there's an issue for that somewhere).

Libraries API module can add its own stream wrapper if needed.

Edit: additionally introducing a new ExtensionDiscovery subclass is a totally different direction from current Extension system work #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList

SKAUGHT’s picture

Currently, yes. However could be needed as some point..

creep happens.

deviantintegral’s picture

This update:

  • Removes the library extension code
  • Fixes test fails due to file_test being renamed to file_module_test, along with installing the module so we can actually test submodule resolution.
dawehner’s picture

Just a quick note: #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList could simplify this patch quite a bit.

+++ b/core/lib/Drupal/Core/StreamWrapper/ExtensionStreamBase.php
@@ -0,0 +1,103 @@
+  /**
+   * Returns the current request object.
+   *
+   * @return \Symfony\Component\HttpFoundation\Request
+   *   The current request object.
+   */
+  protected function getRequest() {
+    if (!isset($this->request)) {
+      $this->request = \Drupal::service('request_stack')->getCurrentRequest();
+    }
+    return $this->request;
+  }

Is it possible to inject this via the constructor?

almaudoh’s picture

Just a quick note: #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList could simplify this patch quite a bit.

Just what I've been saying too :) Many other things too would also be unblocked by that patch and this patch. When will they get the attention of the committers? :/

claudiu.cristea’s picture

@dawehner,

Is it possible to inject this via the constructor?

No, unfortunately we cannot inject services in a stream wrapper constructor because stream_wrapper_register() doesn't allow that.

jibran’s picture

Status: Needs review » Needs work

NW for needs tags.

claudiu.cristea’s picture

Issue tags: -Needs tests

@jibran, Yes, except "Needs tests". Tests were provided in the patch. If you feel that we need to test a specific feature of this patch, not included in patch tests, feel free to re-tag and please specify which.

hass’s picture

Cannot find a way how to attach a library pointing to public://

claudiu.cristea’s picture

claudiu.cristea’s picture

dawehner’s picture

Just a saturday borning review :)

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/ExtensionStreamBase.php
    @@ -0,0 +1,103 @@
    +    // Remove the trailing filename from the path.
    +    $length = strpos($uri_parts[1], '/');
    +    return ($length === FALSE) ? $uri_parts[1] : substr($uri_parts[1], 0, $length);
    

    Could we use ltrim here and call it a day?

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/ExtensionStreamBase.php
    @@ -0,0 +1,103 @@
    +  protected function getRequest() {
    +    if (!isset($this->request)) {
    +      $this->request = \Drupal::service('request_stack')->getCurrentRequest();
    +    }
    +    return $this->request;
    +  }
    

    What about storing the request stack on the object. This makes it safe against changing someone pushing/popping request onto the stack.

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalReadOnlyStream.php
    @@ -42,15 +42,7 @@ public function stream_open($uri, $mode, $options, &$opened_path) {
         }
    -
    -    $this->uri = $uri;
    -    $path = $this->getLocalPath();
    -    $this->handle = ($options & STREAM_REPORT_ERRORS) ? fopen($path, $mode) : @fopen($path, $mode);
    -    if ($this->handle !== FALSE && ($options & STREAM_USE_PATH)) {
    -      $opened_path = $path;
    -    }
    -
    -    return (bool) $this->handle;
    +    return parent::stream_open($uri, $mode, $options, $opened_path);
       }
     
    

    This is indeed basically identical to the parent method, but is this needed in this issue? I'm just asking

  4. +++ b/core/modules/system/tests/src/Kernel/File/ExtensionStreamTest.php
    @@ -0,0 +1,271 @@
    +      [
    +        'profile://',
    +        'profile://',
    +        '/core/profiles/minimal',
    +        'core/profiles/minimal',
    +      ],
    

    While reading this line, I'm wondering whether we should think about #2692403: Make info and configuration inheritable in profiles already. Could we maybe use profile://installed or so to indicate the current one, so we can in the future provide parents somehow as well?

  5. +++ b/core/modules/system/tests/src/Kernel/File/ExtensionStreamTest.php
    @@ -0,0 +1,271 @@
    +}
    diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
    
    diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
    index 770a3a7..7f28c29 100644
    
    index 770a3a7..7f28c29 100644
    --- a/sites/default/default.settings.php
    
    --- a/sites/default/default.settings.php
    +++ b/sites/default/default.settings.php
    
    +++ b/sites/default/default.settings.php
    +++ b/sites/default/default.settings.php
    @@ -420,20 +420,6 @@
    
    @@ -420,20 +420,6 @@
      */
     # $settings['omit_vary_cookie'] = TRUE;
     
    -
    -/**
    - * Cache TTL for client error (4xx) responses.
    - *
    - * Items cached per-URL tend to result in a large number of cache items, and
    - * this can be problematic on 404 pages which by their nature are unbounded. A
    - * fixed TTL can be set for these items, defaulting to one hour, so that cache
    - * backends which do not support LRU can purge older entries. To disable caching
    - * of client error responses set the value to 0. Currently applies only to
    - * page_cache module.
    - */
    -# $settings['cache_ttl_4xx'] = 3600;
    -
    -
     /**
    

    This seems to be changed by accident

claudiu.cristea’s picture

Issue summary: View changes
claudiu.cristea’s picture

Title: Add stream wrappers to access extensions files » Add stream wrappers to access extension files
FileSize
29.91 KB
3.27 KB

@dawehner, thank you.

  • #295.1: I don't get it. Next cases should all return 'node':
    module://node
    module://node/
    module://node/node.info.yml
    So, I don't understand where to use ltrim() here. The URI might contain also a subdir or subdir and file.
  • #295.2: Done.
  • #295.3: Indeed. I created #2751351: Remove code duplication from LocalReadOnlyStream. That needs review too :)
  • #295.4: There was a decision taken earlier (see #245 and #247) to keep this patch as simple as we can and to avoid pseudo-stream-wrappers like: theme://active-theme, theme://admin-theme or profile://installed-profile. Extending a profile is a complex task and should be fixed in that issue. Right now, profile:// is only the installed profile as is stated in IS and for complexity reasons invoked in early comments,
  • #295.5: Right.
claudiu.cristea’s picture

Updated also the Draft CR: https://www.drupal.org/node/2352923

dawehner’s picture

#295.4: There was a decision taken earlier (see #245 and #247) to keep this patch as simple as we can and to avoid pseudo-stream-wrappers like: theme://active-theme, theme://admin-theme or profile://installed-profile. Extending a profile is a complex task and should be fixed in that issue. Right now, profile:// is only the installed profile as is stated in IS and for complexity reasons invoked in early comments,

For admin themes its easier. You can add that later. I totally agree with you to keep the patch simple. For profiles though, it would be quite hard to keep BC, don't you think so?

dawehner’s picture

#295.4: There was a decision taken earlier (see #245 and #247) to keep this patch as simple as we can and to avoid pseudo-stream-wrappers like: theme://active-theme, theme://admin-theme or profile://installed-profile. Extending a profile is a complex task and should be fixed in that issue. Right now, profile:// is only the installed profile as is stated in IS and for complexity reasons invoked in early comments,

Okay sure, let's follow that route. We will figure out the install profile inheritance later anyway. Sorry for the inconvenience!

So, I don't understand where to use ltrim() here. The URI might contain also a subdir or subdir and file.

Oh sorry, I talked a bit of BS. strtok is what you want (see https://3v4l.org/V9B14)

<?php

function a($uri) {
    $uri_parts = explode('://', $uri, 2);
    $length = strpos($uri_parts[1], '/');
    return ($length === FALSE) ? $uri_parts[1] : substr($uri_parts[1], 0, $length);
}

function b($uri) {
    $uri_parts = explode('://', $uri, 2);
    return strtok($uri_parts[1], '/');
}

var_dump(a('module://node'));
var_dump(a('module://node/'));
var_dump(a('module://node/node.info.yml'));

var_dump(b('module://node'));
var_dump(b('module://node/'));
var_dump(b('module://node/node.info.yml'));
claudiu.cristea’s picture

@dawehner, I don't believe in profile://installed because "installed" is not blacklisted as extension/profile name. Instead we need dedicated stream wrappers for those pseudo-extensions. See the reasons from #227.

claudiu.cristea’s picture

FileSize
29.77 KB
3.53 KB

Right :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

claudiu.cristea’s picture

FileSize
675 bytes

The latest interdiff is wrong. Sorry

catch’s picture

+++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
@@ -260,7 +260,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', 'theme://test_theme');

Still not keen on changes like this. Why can't this do something like $this->container->get('theme_handler')->getTheme('test_theme')-getPath().

claudiu.cristea’s picture

@catch, yes, we can use that too. But this is the nice thing using stream wrappers —­ it's more simple and intuitive. In fact this is the scope of the issue. #305 is almost the same code as that used by the stream wrapper.

catch’s picture

Status: Reviewed & tested by the community » Needs review

@claudiu.cristea I don't agree that it's more simple and intuitive.

Instead of a method call that clearly shows where the data is coming from and has a relatively clean stack trace (or at least could if we cleaned the extension system up more), we're invoking php stream wrappers which then calls back out to Drupal. It's obfuscating what happens rather than making it clearer.

Also are there really no non-test cases in core this could be applied to?

I said the same thing in #13 (back in 2012) and #202 by the way.

claudiu.cristea’s picture

FileSize
29.67 KB
1.39 KB

OK. I reverted the test case and added a non-test case.

heddn’s picture

Status: Needs review » Needs work

Small nits on docs only.

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
    @@ -0,0 +1,63 @@
    + * Defines the read-only module:// stream wrapper for module files.
    

    The issue summary provides a great example of module://{name}. Let's use it here in the docs.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.php
    @@ -0,0 +1,63 @@
    + * Defines the read-only theme:// stream wrapper for theme files.
    

    Ditto. The issue summary provides a great example of theme://{name}.

This doesn't work with image styles, because the URL is transformed from:
theme://custom_theme/logo.png
into:
theme://styles/issues_landing/theme/custom_theme/logo.png

Which causes getOwnerName to fail:
RuntimeException: Theme styles does not exist or is not installed
Same would apply to modules me thinks.

heddn’s picture

Additional note:
ImageStyle::load('thumbnail')->buildUrl('themes/custom/custom_theme/logo.png');
results in a 404.

Because ImageStyleDownloadController converts the above url into:
public://themes/custom/libre_custom_theme/grey-logo.png

The file doesn't exist in the public file system. It exists in the theme.

Seems like a catch 22.

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.

claudiu.cristea’s picture

Status: Needs work » Needs review

@heddn, the failures from #309 and #310 are caused by a bug in the image.module. The bug was fixed in #2770761: Derivatives should not be created on read-only stream wrappers. You can try again.

claudiu.cristea’s picture

FileSize
1.61 KB
30.06 KB

Fixing #309.

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.

andrewbelcher’s picture

+100 for this. This would allow you to ship images with modules and apply image styles to them. The current workaround is either to basically implement this (although a much simpler approach can be used for a one off) or to store the file as a managed file and track it's ID, manage changes to your file etc.

pwolanin’s picture

needed re-roll for fuzz and search test file moved to core/modules/search/tests/src/Functional/SearchSimplifyTest.php

couldn't use patch for 2 files - needed manual application of changes:

core/lib/Drupal/Core/StreamWrapper/LocalStream.php
core/lib/Drupal/Core/StreamWrapper/ReadOnlyStream.php

so please double check those are right.

===

seems not:

PHP Fatal error: Access level to Drupal\Core\StreamWrapper\ModuleStream::getDirectoryPath() must be public (as in class Drupal\Core\StreamWrapper\LocalStream) in /var/www/html/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php on line 15

Status: Needs review » Needs work

The last submitted patch, 316: 1308152-315.patch, failed testing.

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.

vijaycs85’s picture

dawehner’s picture

Issue tags: +needs profiling

After fighting with #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList for quite a while, I fear we would need some profiling here as well, for example of the installation.

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

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

aspilicious’s picture

Asked for a retest, composer can't apply on drupal 8.5 locally.

aspilicious’s picture

borisson_’s picture

This still needs the profiling requested in #320 by @dawehner.

Attached patch fixes some cs errors.

There was one place in the tests where this was changed, so I ran that test to do some prelimary profiling.

With patch.

$ ./vendor/bin/phpunit -c core/ --filter "SearchSimplifyTest::testSearchSimplifyUnicode" --repeat 10
PHPUnit 6.5.8 by Sebastian Bergmann and contributors.

Testing
..........                                                        10 / 10 (100%)

Time: 1.62 minutes, Memory: 628.00MB

Without patch:

$ ./vendor/bin/phpunit -c core/ --filter "SearchSimplifyTest::testSearchSimplifyUnicode" --repeat 10
PHPUnit 6.5.8 by Sebastian Bergmann and contributors.

Testing
..........                                                        10 / 10 (100%)

Time: 1.59 minutes, Memory: 628.00MB

I ran the test one time before doing the repeated test, to make sure that nothing weird was going on. If this is something to go by, there is a difference, but it is not significant at all.

Wim Leers’s picture

It looks like the only thing that remains here is profiling!

Version: 8.6.x-dev » 8.7.x-dev

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

Status: Needs review » Needs work

The last submitted patch, 324: 1308152-324.patch, failed testing. View results

aspilicious’s picture

Status: Needs work » Needs review
FileSize
8.78 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 328: 1308152-328.patch, failed testing. View results

aspilicious’s picture

Status: Needs work » Needs review
FileSize
29.48 KB

Forgot some files

Status: Needs review » Needs work

The last submitted patch, 330: 1308152-330.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

izus’s picture

hi,
#324 no more applies
here is a reroll of it
Thanks

izus’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 332: 1308152-327.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pingwin4eg’s picture

Hey, there's a movement to allow multiple enabled profiles in one installation - #1356276: Allow profiles to define a base/parent profile. So I suppose, the profile stream wrapper should contain the profile name too?

zipymonkey’s picture

almaudoh’s picture

Status: Needs work » Needs review

Testing the patch in #337

Status: Needs review » Needs work
hanoii’s picture

It's #337 with an attempt to fix tests. Applies to both 8.8.x and 8.7.x.

AaronMcHale’s picture

This issue started in 2011 and the discussion about not using this for referencing PHP files was quite early on, I believe APC was cited as the reason, is that still the case? I ask because it would be really nice if this could be used in any case where you need to reference a file regardless of if it's a PHP file or a static asset. That would create a nicer developer experience and result in less confusion, where you only have to use one approach consistently instead of switching between approaches depending on the context, which could easily result in the wrong use.

Also just wanted to clarify if this will be available in Twig templates? I suspected it will be and so would be a huge bonus, but just wanted to clarify that.

hanoii’s picture

Status: Needs review » Reviewed & tested by the community

And btw, we tested this and it's working properly along with previous tests, so I am marking it as RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

This still needs profiling - that's been an outstanding task for a while.

@AaronMcHale can you clarify how this would be used in Twig templates?

hanoii’s picture

@alexpott what kind of profiling is needed and any guidelines for me to help with that?

AaronMcHale’s picture

@alexpott re Twig: I was thinking if someone needed to reference/include some kind of resource, but then remembered that in most cases you'd still need to use variables like {{ base_path }} or {{ directory }}, even concatenating them together in some cases.

Building on that thought, since Twig is already parsing the Twig HTML files, I wonder how easy (or not easy) it would be to make the stream wrappers usable directly in the HTML, for example <img src="theme://theme/images/image.jpg">, maybe even in CSS files, since I think Drupal already does some scanning and manipulation when caching and aggregating CSS files.

On another note, going back to what I said about including PHP files, now that I think about it there's probably almost no reason contrib needs to actually include other PHP files, since generally PHP files should contain classes and would be already autoloaded.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mdupont’s picture

Re-rolled against 8.9.x since it was no longer applying after a file rename in the Search module.

Status: Needs review » Needs work

The last submitted patch, 348: 1308152-348.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Hardik_Patel_12’s picture

Status: Needs work » Needs review
FileSize
28.6 KB
407 bytes

Solving failed test case, kindly review a new patch.

Status: Needs review » Needs work

The last submitted patch, 350: 1308152-350.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

narendra.rajwar27’s picture

Status: Needs work » Needs review
FileSize
28.57 KB
624 bytes

Status: Needs review » Needs work

The last submitted patch, 352: 1308152-352.patch, failed testing. View results

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

AndyF’s picture

Issue summary: View changes

Add profiling as a remaining task for extra clarity (the issue's already tagged).

mdupont’s picture

Status: Needs work » Needs review
FileSize
28.59 KB
3.57 KB

Re-roll against 9.2-dev since the patch failed to apply. Let's see if it passes the tests.

Status: Needs review » Needs work

The last submitted patch, 356: 1308152-356.patch, failed testing. View results

raman.b’s picture

Status: Needs review » Needs work

The last submitted patch, 358: 1308152-358.patch, failed testing. View results

alexpott’s picture

  1. +++ b/core/core.services.yml
    @@ -1372,6 +1372,18 @@ services:
    +  stream_wrapper.module:
    +    class: Drupal\Core\StreamWrapper\ModuleStream
    +    tags:
    +      - { name: stream_wrapper, scheme: module }
    +  stream_wrapper.theme:
    +    class: Drupal\Core\StreamWrapper\ThemeStream
    +    tags:
    +      - { name: stream_wrapper, scheme: theme }
    +  stream_wrapper.profile:
    +    class: Drupal\Core\StreamWrapper\ProfileStream
    +    tags:
    +      - { name: stream_wrapper, scheme: profile }
    
    +++ b/core/lib/Drupal/Core/StreamWrapper/ExtensionStreamBase.php
    @@ -0,0 +1,101 @@
    +  /**
    +   * Returns the request stack object.
    +   *
    +   * @return \Symfony\Component\HttpFoundation\RequestStack
    +   *   The request stack object.
    +   */
    +  protected function getRequestStack() {
    +    if (!isset($this->requestStack)) {
    +      $this->requestStack = \Drupal::service('request_stack');
    +    }
    +    return $this->requestStack;
    +  }
    
    +++ b/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php
    @@ -0,0 +1,70 @@
    +  /**
    +   * Returns the module handler service.
    +   *
    +   * @return \Drupal\Core\Extension\ModuleHandlerInterface
    +   *   The module handler service.
    +   */
    +  protected function getModuleHandler() {
    +    if (!isset($this->moduleHandler)) {
    +      $this->moduleHandler = \Drupal::moduleHandler();
    +    }
    +    return $this->moduleHandler;
    +  }
    
    +++ b/core/lib/Drupal/Core/StreamWrapper/ThemeStream.php
    @@ -0,0 +1,70 @@
    +  /**
    +   * Returns the theme handler service.
    +   *
    +   * @return \Drupal\Core\Extension\ThemeHandlerInterface
    +   *   The theme handler service.
    +   */
    +  protected function getThemeHandler() {
    +    if (!isset($this->themeHandler)) {
    +      $this->themeHandler = \Drupal::service('theme_handler');
    +    }
    +    return $this->themeHandler;
    +  }
    

    All these dependencies can be injected now.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/ExtensionStreamBase.php
    @@ -0,0 +1,101 @@
    +  // @todo Move this in \Drupal\Core\StreamWrapper\LocalStream in Drupal 9.0.x.
    +  use StringTranslationTrait;
    

    This looks dated... needs to link to an issue too.

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/ExtensionStreamBase.php
    @@ -0,0 +1,101 @@
    +   * @code SystemStream::getOwnerName('module://foo') @endcode and @code
    +   * SystemStream::getOwnerName('module://foo/')@endcode will both return @code
    

    SystemStream? This example looks dated.

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/ExtensionStreamBase.php
    @@ -0,0 +1,101 @@
    +   * @return string
    +   *   The extension name.
    +   */
    +  protected function getOwnerName() {
    

    Can use a return typehint

  5. +++ b/core/lib/Drupal/Core/StreamWrapper/ProfileStream.php
    @@ -0,0 +1,39 @@
    +    return \Drupal::installProfile();
    

    This should be injected too.

  6. +++ b/core/modules/search/tests/src/Kernel/SearchSimplifyTest.php
    @@ -28,7 +28,7 @@ public function testSearchSimplifyUnicode() {
    -    $input = file_get_contents($this->root . '/core/modules/search/tests/UnicodeTest.txt');
    +    $input = file_get_contents('module://search/tests/UnicodeTest.txt');
    

    This change is actually a regression. The test is a core search test. It should be able to make assumptions about its path - invoking the extension system that means that search module can be relocated and changed makes the behaviour of this unpredictable.

adamzimmermann’s picture

FileSize
30.57 KB
7.02 KB

360.1:
DI added.

360:2
I'm leaving this for someone who has more context.

360.3
I'm leaving this for someone who has more context.

360.4
Return typehint added.

360.5
DI added.

360.6
Do we just want to revert this change? I'm leaving that for others.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ricovandevin’s picture

is_file('module://path/to/file.jpg') leads to an ArgumentCountError with the patch in #361:

ArgumentCountError: Too few arguments to function Drupal\Core\StreamWrapper\ModuleStream::__construct(), 0 passed and exactly 2 expected in Drupal\Core\StreamWrapper\ModuleStream->__construct() (line 35 of core/lib/Drupal/Core/StreamWrapper/ModuleStream.php).

bradjones1 made their first commit to this issue’s fork.

bradjones1’s picture

Status: Needs work » Needs review

Migrated this to an MR. Re-rolled with some fuzz but no rewriting necessary.

Addressed #360 points 2, 3 and 6 which were outstanding from changes made in #361. Specifically, I reverted the change to the search test, which seems out of scope. It could theoretically use __DIR__ in the line that was changed, but so could other things be improved in that test :-)

Let's see what testbot yields.

bradjones1’s picture

@ricovandevin are you observing that we aren't fuzzing the tests sufficiently? In your example the module would be path, and the file inside would be to/file.jpg?

bradjones1’s picture

Is there a standard procedure for profiling? I'm not familiar and can't find anything in docs. This page isn't particularly clear about a standard procedure.

ricovandevin’s picture

In your example the module would be path, and the file inside would be to/file.jpg?

@bradjones1 That is correct. In our specific use case we have an images folder inside the module folder and we want to point to an image in that images folder using the module:// stream wrapper.

Debugging shows that in case of is_file() the getWrapper() method of the StreamWrapperManager is never called.

It works when is_file($path) is replaces with is_file(\Drupal::service('file_system')->realpath($path)). But we cannot rely on all occurrences of is_file() being updated.

ricovandevin’s picture

As is_file() seems to lead to the wrapper classes being instantiated directly I guess we need to have a fallback for the injected services.

Not sure if the approach I added in the MR is ok and what a better alternative would be. But I'm happy to learn from whoever has a better solution. :-)

Berdir’s picture

Status: Needs review » Needs work

Added some comments to the MR, contains some .orig files that should be removed.

mondrake made their first commit to this issue’s fork.

mondrake’s picture

Rerolled and addressed @Berdir's points.

mondrake’s picture

I reverted the change to the color.module file because there are several other places in that file that would require the same change for consistency, but I am not sure we want this when it is code requiring to access a file. AFAICS it's preferrable to use the extension.list.module service in that case.

Rather, I am trying here to change the image.settings.preview_image which is possibly closer to the use case here i.e. user input in config.

EDIT - Also see #2774449-2: Edge case image derivative URIs collision

mondrake’s picture

MR push above xposted with @alexpott's comments in the MR.

I cleaned up dependency injection as all injected args in constructors should be mandatory - we're adding a new API here, no need to play the BC dance we're doing with existing API.

For ProfileStream, I can't manage to get the installed profile available at constructor's time to inject, in the context of a kernel test. Calling setInstalledProfile() works, but only for sebsequent access to the container. For the moment I am expliciting getting it from the container in getOwnerName(), till a better solution can be found.

Berdir’s picture

I commented on the DI aspect, I'm not so sure that works.

> I reverted the change to the color.module file because there are several other places in that file that would require the same change for consistency, but I am not sure we want this when it is code requiring to access a file. AFAICS it's preferrable to use the extension.list.module service in that case.

Not sure about this, but I'm happy to push that to a follow-up issue, but that's IMHO like saying modules shouldn't use public://.

mondrake’s picture

@Berdir is right about DI, it does not work. We need to remove dependency injection then, in favor of \Drupal::service calls instead.

catch’s picture

Per #1308152-13: Add stream wrappers to access extension files, more than nine years ago... I'm still struggling with the use case for this - it's hundreds of lines of code to support (unless I missed something), this single change in core:

preview_image: core/modules/image/sample.png
preview_image: module://image/sample.png

Previous use-cases discussed in this issue, like CSS and JS, have evaporated as we've improved our APIs elsewhere (i.e. module.libraries.yml references relative paths to assets, and generally modules deal with each other's libraries instead of specific files).

The examples in the issue summary aren't examples of things that modules actually need to do often, and include referencing PHP files which was ruled out a long time ago for stream wrappers due to opcode caching, as well as also being something that has no use-case as the surface of procedural code continues to shrink.

Berdir’s picture

Yeah, that being the only example now isn't really showing this in a great light.

However, that only works for a core module, a contrib module might be in multiple places and even a core module could be as an override in different place. Edge case, but we do technically support that. For a contrib module, doing what image module does is simply not possible. It would need hook_install() do dynamically set the path or provide an empty default and that is then hardcode the default in code. Or do what media_install() does, copy them into public://, which also uses the API and not just hardcode the path.

And if we include examples in code, for example ckeditor plugins like \Drupal\ckeditor\Plugin\CKEditorPlugin\DrupalImage (which can't use the library system).

\Drupal::service('extension.list.module')->getPath('image') . '/sample.png'; (and in many cases DI for that service, so contstructor, property, maybe create() method, ..
module://image/sample.png

Based on my reroll of the drupal_get_path() issue, batch include files is still a fairly common use case in core to reference PHP files, but that's just because we didn't convert those to classes/services yet (batch API does currently not support services).

In my install profile on 9.2, I'm seeing ~100 calls to drupal_get_path() with a hardcoded module name (drupal_get_path('module', '), sure, a ton of that is tests. And another 150 in our contrib modules.

mondrake’s picture

mondrake’s picture

The way I see this issue is mainly an attempt to get the System Stream Wrapper module into core. There's a clear statement in this direction in the project page, and looking now at the code I see a lot of what's there ported here.
Agree this is mostly relevant in contrib (I would be certainly using these wrappers it in Image Effect for instance), and the number of installs of System Stream Wrapper shows there's a need for this.
Unfortunately there's no stable release for that module, and stable modules likely won't be keen on requiring it as a dependency.

So the options here IMHO are either won't fix this and get a stable release for SSW, or get a core supported set of wrappers and make SSW redundant.

Last commit removes DI and addresses some of @alexpott's points.

mondrake’s picture

Cannit figure out the PHP 8 test failure - it’s not the test failing, rather PHPUnit or the bridge on DrupalCI. Locally tests pass, as do in DrupalCI on PHP 7.3

mondrake’s picture

Status: Needs work » Needs review
mondrake’s picture

Hm, test failures may be because of this https://github.com/sebastianbergmann/phpunit/issues/4347

mondrake’s picture

Assigned: Unassigned » mondrake
Category: Task » Feature request
Priority: Major » Normal

I’m working on simplifying the patch.

mondrake’s picture

Assigned: mondrake » Unassigned

Significantly simplified the patch, now it only adds the new stream wrappers without touching preexisting ones, and most of the work is done in the ExtensionStreamBase class.

mondrake’s picture

Rerolled

DuaelFr’s picture

This looks good! Nice work here!

@catch #379: I found this issue while trying to get an image from my theme in a responsive image style process. I could not find any other way to acheive that without placing my source picture in the public files (which is not ideal). Now I can just use this trick in my template (or do the same in a preprocess):

{% set picture = {
  '#theme': 'responsive_image',
  '#responsive_image_style_id': 'my_image_style',
  '#uri': 'theme://my_theme/dist/images/my_picture.png'
} %}
{{ picture }}
clayfreeman’s picture

I know the discussion around library:// is largely over(?), but I thought it pertinent to mention this change record, which seems like a natural fit for a stream wrapper so that contrib doesn't need to bother with using \Drupal::service('library.libraries_directory_file_finder')->find(). Instead, contrib could just refer to library://name/path/to/resource.

Also, libraries don't have to be purely CSS or JS - in fact, sometimes external libraries are required for other things (e.g., server-side compilation of Sass or TypeScript resources). The use cases could be infinite, so having a stream wrapper would be extremely convenient. At the end of the day, libraries are just a folder on disk, so they should be treated as such. The difference is that the folder could exist in any one of the directories listed in the change record.

mdupont’s picture

All threads on the Merge Request were addressed, and it can still be merged into 9.3.x, what is missing to move it to RTBC?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

aspilicious’s picture

Mondrake could you rebase your branch? It is failing to apply at the moment.

mondrake’s picture

@aspilicious no I cannot, what needs to be done here is:

a) @bradjones1 should change the target branch to 9.4.x or 10.0.x, only the MR creator can
b) someone to pull from the MR branch
c) that someone to merge locally with the 9.4.x fresh HEAD, resolve conflicts, git add+commit
d) that someone to push the result to the MR branch

or

a) someone to create a new MR from 9.4
b) apply locally the patch from the current MR, possibly with 3way merge
c) resolve any conflict, git add+commit
d) push the result to the new MR branch

aspilicious’s picture

I thought the merge request stuff would make it easier... :D
Creating a good old regular patch file.
I think it contains everything from the MR?

bradjones1’s picture

Target changed on the MR.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

Rerolled

geek-merlin’s picture

Status: Needs review » Needs work

Now it needs reroll onto 9.5.x. Maybe a new MR.

mondrake’s picture

Being a feature request, honestly I think this is for no sooner than 10.1. See ya in a year or so!

mondrake’s picture

Version: 9.5.x-dev » 10.0.x-dev
geek-merlin’s picture

Bump...

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.