Updated: Comment #130

Problem/Motivation

drupal_get_path() infiltrates almost every module and isn't very intuitive for new developers. Stream wrappers make much more sense.

Proposed resolution

Replace drupal_get_path() with stream wrappers in strings that are evaluated for a module, theme or profile path.

Before patch examples:

<?php
drupal_add_css
(drupal_get_path('module', 'theme_test') . '/css/base-override.css');
$render_array['#attached']['css'][] = drupal_get_path('module', 'theme_test') . '/css/base-override.css';
?>

After patch examples:
<?php
drupal_add_css
('module://theme_test/css/base-override.css');
$render_array['#attached']['css'][] = 'module://theme_test/css/base-override.css';
?>

Remaining tasks

Consensus is to go with the suggested module:// theme:// and profile:// suggested.
Needs a reroll
Needs to pass tests
Would be nice, but not a requirement to be able to script replacement of 90% of drupal_get_path calls to not increase amount of novice issue tasks

API changes

Simplifies getting paths to modules, themes and profiles by using a stream wrapper.

#1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend

Original report by @Dave Reid

I wrote http://drupal.org/project/system_stream_wrapper and now let's merge it into core! It's super helpful to be able to access files via module://modulename/path/to/file.txt which nicely resolves to the actual file location.

Related: #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend

Files: 
CommentFileSizeAuthor
#134 drupal8.system_stream_wrappers.1308152-134.patch30.2 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 48,360 pass(es), 3,319 fail(s), and 1,913 exception(s).
[ View ]
#134 interdiff.txt1.14 KBdisasm
#131 drupal8.system_stream_wrappers.1308152-131.patch31.33 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#95 drupal-wrappers-1308152-95.patch31.62 KBPancho
FAILED: [[SimpleTest]]: [MySQL] 47,056 pass(es), 3,294 fail(s), and 1,893 exception(s).
[ View ]
#95 interdiff-85-95.txt32.81 KBPancho
#86 interdiff.txt5.13 KBjthorson
#85 drupal-wrappers-1308152-85.patch15.04 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] 56,858 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#84 drupal-wrappers-1308152-83.patch15.04 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#82 drupal-wrappers-1308152-82.patch15.45 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/File/SystemStreamTest.php.
[ View ]
#82 interdiff.txt5.54 KBjthorson
#80 drupal-wrappers-1308152-80.patch9.91 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,395 pass(es).
[ View ]
#80 interdiff.txt2.58 KBParisLiakos
#71 drupal-wrappers-1308152-71.patch9.76 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,804 pass(es).
[ View ]
#68 drupal-wrappers-1308152-68.patch9.19 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#61 1308152-add-module-theme-profile-stream-wrappers-rev7.patch9.18 KBbrianV
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1308152-add-module-theme-profile-stream-wrappers-rev7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#59 1308152-add-module-theme-profile-stream-wrappers-rev6.patch8.18 KBbrianV
FAILED: [[SimpleTest]]: [MySQL] 50,017 pass(es), 268 fail(s), and 8,809 exception(s).
[ View ]
#50 1308152-add-module-theme-profile-stream-wrappers-rev5.patch8.17 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1308152-add-module-theme-profile-stream-wrappers-rev5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#43 1308152-add-module-theme-profile-stream-wrappers-rev4.patch11.77 KBbrianV
PASSED: [[SimpleTest]]: [MySQL] 48,527 pass(es).
[ View ]
#28 1308152-add-module-theme-profile-stream-wrappers-rev3.patch12.9 KBbrianV
None
[ View ]
#8 1308152-add-module-theme-profile-stream-wrappers-rev2.patch9.71 KBbrianV
PASSED: [[SimpleTest]]: [MySQL] 35,730 pass(es).
[ View ]
#6 1308152-add-module-theme-profile-stream-wrappers.patch9.67 KBbrianV
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

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.

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

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

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.

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

Status:Active» Needs review
StatusFileSize
new9.67 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1308152-add-module-theme-profile-stream-wrappers.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.71 KB
PASSED: [[SimpleTest]]: [MySQL] 35,730 pass(es).
[ View ]

Updated patch, should pass testing.

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

And theme://current

Ow yes, brilliant!

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)

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

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.

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

@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.

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

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

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.

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.

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.

Issue tags:+sprint, +Media Initiative

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?

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.

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

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?

Forgot to attach patch.

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.

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

"Supports..."

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.

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.

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

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.

@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.

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.

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)

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

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.

@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.

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.

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

StatusFileSize
new11.77 KB
PASSED: [[SimpleTest]]: [MySQL] 48,527 pass(es).
[ View ]

@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.

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

@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.

@#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.

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.

Status:Postponed» Needs review

StatusFileSize
new8.17 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1308152-add-module-theme-profile-stream-wrappers-rev5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1308152-add-module-theme-profile-stream-wrappers-rev5.patch, failed testing.

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 1308152-add-module-theme-profile-stream-wrappers-rev5.patch, failed testing.

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

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

#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.

Status:Needs work» Needs review
Issue tags:-sprint, -Media Initiative, -VDC

Status:Needs review» Needs work
Issue tags:+sprint, +Media Initiative, +VDC

The last submitted patch, 1308152-add-module-theme-profile-stream-wrappers-rev5.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.18 KB
FAILED: [[SimpleTest]]: [MySQL] 50,017 pass(es), 268 fail(s), and 8,809 exception(s).
[ View ]

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

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.

StatusFileSize
new9.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1308152-add-module-theme-profile-stream-wrappers-rev7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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:

<?php
$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?

Status:Needs work» Needs review

Sending to the bot.

Status:Needs review» Needs work

The last submitted patch, 1308152-add-module-theme-profile-stream-wrappers-rev7.patch, failed testing.

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

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new9.19 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

lets start with a reroll

Status:Needs review» Needs work

The last submitted patch, drupal-wrappers-1308152-68.patch, failed testing.

StatusFileSize
new9.76 KB
PASSED: [[SimpleTest]]: [MySQL] 56,804 pass(es).
[ View ]

should be tested, after the issue above is committed

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.

@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.

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

Status:Needs work» Needs review

blocker is in!

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

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.

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

Status:Needs work» Needs review

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

StatusFileSize
new2.58 KB
new9.91 KB
PASSED: [[SimpleTest]]: [MySQL] 56,395 pass(es).
[ View ]

that should fix #78

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... :/

Status:Needs work» Needs review
StatusFileSize
new5.54 KB
new15.45 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/File/SystemStreamTest.php.
[ View ]

First cut at adding some tests.

Status:Needs review» Needs work

The last submitted patch, drupal-wrappers-1308152-82.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new15.04 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new15.04 KB
FAILED: [[SimpleTest]]: [MySQL] 56,858 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new5.13 KB

Updated interdiff (#80 to #85).

Status:Needs review» Needs work

The last submitted patch, drupal-wrappers-1308152-85.patch, failed testing.

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

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

Assigned:Dave Reid» Pancho

Working on this some more minutes.

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

<?php
   
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.)

<?php
   
// 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...

#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.

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 an 'empty' libraries folder and encourage people to use it properly.

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

#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.

Status:Needs work» Needs review
StatusFileSize
new32.81 KB
new31.62 KB
FAILED: [[SimpleTest]]: [MySQL] 47,056 pass(es), 3,294 fail(s), and 1,893 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal-wrappers-1308152-95.patch, failed testing.

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.

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?

Status:Needs work» Needs review

"Needs review" in order to discuss #98.

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;\\.

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.

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

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.

@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.

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

+1 to #106

+1 to #106

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

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

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

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)

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

#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.

Title:Add module://, theme:// and profile:// stream wrappers to access system filesAdd 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.

Title:Add drupal:// stream wrapper to access system filesAdd 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.

Title:Add module://, theme:// and profile:// stream wrappers to access system filesAdd 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

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

x-post
agree with #116

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

@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.

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

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://.

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.

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?

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.

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

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.

D.o tag monster

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?

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.

Issue summary:View changes

Applying template

Status:Needs work» Needs review
Issue tags:-Needs issue summary update
StatusFileSize
new31.33 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

Status:Needs review» Needs work
Issue tags:+Needs issue summary update

The last submitted patch, drupal8.system_stream_wrappers.1308152-131.patch, failed testing.

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.

StatusFileSize
new1.14 KB
new30.2 KB
FAILED: [[SimpleTest]]: [MySQL] 48,360 pass(es), 3,319 fail(s), and 1,913 exception(s).
[ View ]

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

The last submitted patch, drupal8.system_stream_wrappers.1308152-134.patch, failed testing.

Issue summary:View changes

forgot problem

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.