Updated: Comment #179

Problem/Motivation

drupal_get_path() infiltrates almost every module and isn't very intuitive for new developers.
Stream wrappers make much more sense and simplify the API for developers trying to refer to image, JS, CSS and other assets files in code.
Stream wrappers will make it easier to reference JS and CSS files from yml files in modules, e.g. *.libraries.yml and will support the API to be introduced by #1762204: Introduce Assetic compatibility layer for core's internal handling of assets Assigned to: sdboyer

Proposed resolution

Consensus is to go with the module:// theme:// and profile:// streamwrappers. Replace drupal_get_path() with stream wrappers in strings that are evaluated for a module, theme or profile path. Documentation will specify that this is *not* to be used to include PHP files.

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';
?>

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.

Remaining tasks

Reviews.
Commit.
Replacement of most / all uses of drupal_get_path calls (separate issue). This can be scripted.

API changes

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

#2028109: Convert hook_stream_wrappers() to tagged services.
#1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend
#1762204: Introduce Assetic compatibility layer for core's internal handling of assets Assigned to: sdboyer
#1702958: Add libraries:// stream wrappers to access system files

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.

Files: 
CommentFileSizeAuthor
#222 add_module_theme-1308152-222.patch35.05 KBmgifford
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,415 pass(es), 1 fail(s), and 5 exception(s).
[ View ]
#218 add_module_theme-1308152-218.patch35.25 KBalmaudoh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,850 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
#205 interdiff.txt2.16 KBalmaudoh
#205 add_module_theme-1308152-205.patch32.47 KBalmaudoh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add_module_theme-1308152-205.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#193 interdiff.txt1.27 KBgarphy
#193 add_module_theme-1308152-193.patch32.4 KBgarphy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,130 pass(es).
[ View ]
#190 add_module_theme-1308152-190.patch32.23 KBJeroenT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,851 pass(es).
[ View ]
#190 interdiff.txt1.56 KBJeroenT
#186 add_module_theme-1308152-186.patch32.01 KBalmaudoh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,885 pass(es).
[ View ]
#170 add_module_theme-1308152-170.patch38.46 KBArla
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,682 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#170 add_module_theme-1308152-170.interdiff.txt2.75 KBArla
#176 interdiff.txt38.98 KBalmaudoh
#176 add_module_theme-1308152-176.patch30.38 KBalmaudoh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,820 pass(es).
[ View ]

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

brianV’s picture

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

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

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

brantwynn’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

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.

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

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.

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.

Status:Needs review» Needs work

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

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

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.

brianV’s picture

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.

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

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?

xjm’s picture

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.

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

ParisLiakos’s picture

ParisLiakos’s picture

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

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

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

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

jthorson’s picture

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.

jthorson’s picture

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

jthorson’s picture

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.

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

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

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 an 'empty' libraries folder (or similar) 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...

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

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

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

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.

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:

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

Status:Needs work» Needs review
StatusFileSize
new28.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1308152-139-system-streamwrappers.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll + #138

Status:Needs review» Needs work

The last submitted patch, 139: 1308152-139-system-streamwrappers.patch, failed testing.

t0xicCode’s picture

Issue tags:+Needs reroll
almaudoh’s picture

Assigned:BTMash» Unassigned
Issue tags:-Needs reroll
StatusFileSize
new32.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,046 pass(es), 19 fail(s), and 0 exception(s).
[ View ]

Re-rolled patch with test fixes.

almaudoh’s picture

Status:Needs work» Needs review

Oops! :p

Status:Needs review» Needs work

The last submitted patch, 143: 1308152-143-system-streamwrappers.patch, failed testing.

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
StatusFileSize
new33.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,118 pass(es), 1,265 fail(s), and 1,079 exception(s).
[ View ]
new15.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?

Status:Needs review» Needs work

The last submitted patch, 148: add_module_theme-1308152-148.patch, failed testing.

The last submitted patch, 148: add_module_theme-1308152-148.patch, failed testing.

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.

    <?php
    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
StatusFileSize
new36.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,474 pass(es), 30 fail(s), and 0 exception(s).
[ View ]
new22.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.

Status:Needs review» Needs work

The last submitted patch, 153: add_module_theme-1308152-153.patch, failed testing.

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
StatusFileSize
new35.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,043 pass(es), 30 fail(s), and 0 exception(s).
[ View ]
new3.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.

Status:Needs review» Needs work

The last submitted patch, 158: add_module_theme-1308152-158.patch, failed testing.

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
StatusFileSize
new35.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,440 pass(es).
[ View ]
new728 bytes

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

almaudoh’s picture

StatusFileSize
new36.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,426 pass(es).
[ View ]
new8.33 KB

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
StatusFileSize
new38.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,103 pass(es).
[ View ]
new44.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
StatusFileSize
new38.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,610 pass(es).
[ View ]
new4.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
StatusFileSize
new38.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,682 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.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.

The last submitted patch, 170: add_module_theme-1308152-170.patch, failed testing.

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
StatusFileSize
new38.98 KB
new30.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,820 pass(es).
[ View ]

#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
StatusFileSize
new1.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
StatusFileSize
new32.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,885 pass(es).
[ View ]
new8.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()

<?php
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
StatusFileSize
new1.56 KB
new32.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,851 pass(es).
[ View ]

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
StatusFileSize
new32.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,130 pass(es).
[ View ]
new1.27 KB

Took care of the comment issues mentioned in #191.

Status:Needs review» Needs work

The last submitted patch, 193: add_module_theme-1308152-193.patch, failed testing.

The last submitted patch, 193: add_module_theme-1308152-193.patch, failed testing.

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: Remove drupal_get_path() as public facing API and replace with module_load_include() for including PHP files 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
StatusFileSize
new32.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add_module_theme-1308152-205.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.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?

Status:Needs review» Needs work

The last submitted patch, 205: add_module_theme-1308152-205.patch, failed testing.

almaudoh’s picture

Status:Needs work» Needs review
StatusFileSize
new35.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,850 pass(es), 1 fail(s), and 4 exception(s).
[ View ]

#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: Remove drupal_get_path() as public facing API and replace with module_load_include() for including PHP files):

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

Status:Needs review» Needs work

The last submitted patch, 218: add_module_theme-1308152-218.patch, failed testing.

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
StatusFileSize
new35.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,415 pass(es), 1 fail(s), and 5 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 222: add_module_theme-1308152-222.patch, failed testing.