Problem/Motivation
Starting from #2351919: Replace uses of drupal_get_path() with __DIR__ where possible, when the PHP code needs to include or parse files inside the module or theme directory space, will simply use _DIR_
instead of drupal_get_path()
. This is sufficient, intuitive and more performant than calling drupal_get_path()
.
However, drupal_get_path()
is widely used to refer files (.js, .css, images, assets) outside a module or theme. This isn't very intuitive for new developers. Stream wrappers make much more sense and simplify the API for developers trying to refer directories or files from outside the current extension in code.
Proposed resolution
Introduce extension stream wrappers:
Scheme | Description |
---|---|
profile:// |
Points to the installed profile root directory. |
module://{name} |
Points to the module {name} root directory. Only enabled modules can be referred. |
theme://{name} |
Points to the theme {name} root directory. Only installed themes can be referred. |
Examples
Profile: profile://
Assuming the standard
profile is installed:
- Referring a directory:
- Actual:
drupal_get_path('profile', 'standard') . '/config'
- Proposed:
profile://config
- Path:
core/profiles/standard/config
- Actual:
- Referring a file:
- Actual:
drupal_get_path('profile', 'standard') . '/config/install/automated_cron.settings.yml'
- Proposed:
profile://config/install/automated_cron.settings.yml
- Path:
core/profiles/standard/config/install/automated_cron.settings.yml
- Actual:
Module: module://
Assuming the node
module is enabled but color
module is not:
- Referring a directory:
- Actual:
drupal_get_path('module', 'node') . '/config'
- Proposed:
module://node/config
- Path:
core/modules/node/config
- Actual:
- Referring a file:
- Actual:
drupal_get_path('module', 'node') . '/config/install/node.settings.yml'
- Proposed:
module://node/config/install/node.settings.yml
- Path:
core/modules/node/config/install/node.settings.yml
- Actual:
- Referring a resource in a uninstalled or inexistent module:
- Actual:
drupal_get_path('module', 'color') . '/config'
- Proposed:
module://color/config
- Path: Throws
\RuntimeException
- Actual:
Theme: theme://
Assuming the bartik
theme is installed but seven
theme is not:
- Referring a directory
- Actual:
drupal_get_path('theme', 'bartik') . '/config'
- Proposed:
theme://bartik/config
- Path:
core/themes/bartik/config
- Actual:
- Referring a file
- Actual:
drupal_get_path('theme', 'bartik') . '/color/color.inc'
- Proposed:
theme://bartik/color/color.inc
- Path:
core/themes/bartik/color/color.inc
- Actual:
- Referring a resource in a uninstalled or inexistent theme:
- Actual:
drupal_get_path('theme', 'seven') . '/config'
- Proposed:
theme://seven/config
- Path: Throws
\RuntimeException
- Actual:
Remaining tasks
- Profiling to compare performance of the new code with the old.
API changes
New abstract class for extensions stream wrappers:
\Drupal\Core\StreamWrapper\ExtensionStreamBase
New stream wrapper classes:
\Drupal\Core\StreamWrapper\ModuleStream
\Drupal\Core\StreamWrapper\ThemeStream
\Drupal\Core\StreamWrapper\ProfileStream
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#395 | 1308152-395.patch | 19.64 KB | aspilicious |
Issue fork drupal-1308152
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
Dave ReidAlso 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.
Comment #2
Gábor HojtsyI 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)?
Comment #3
Dave ReidI'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).
Comment #4
Crell CreditAttribution: Crell commentedLet'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.
Comment #5
brianV CreditAttribution: brianV commented+1 to this. I also think that library:// would be very helpful.
Comment #6
brianV CreditAttribution: brianV commentedAttached 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.
Comment #8
brianV CreditAttribution: brianV commentedUpdated patch, should pass testing.
Comment #9
Dave ReidOoh, we should probably add theme://default and theme://admin support for the default and admin themes respectively.
Comment #10
Dave ReidAnd theme://current
Comment #11
betz CreditAttribution: betz commentedOw yes, brilliant!
Comment #12
chx CreditAttribution: chx commentedThis 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)
Comment #13
catchdrupal_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).
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentednot 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.
Comment #15
dman CreditAttribution: dman commentedThis is so sexy.
Will clear out all those drupal_get_path() clunky calls.
Comment #16
Dave Reid@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.
Comment #17
stevectorI just closed a duplicate: #908874: Provide module://, theme://, drupal:// schemes for files That issue also suggested libraries://
Comment #18
xjmTagging.
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://).
Comment #19
saltednutJust 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.
Comment #20
Robin Millette CreditAttribution: Robin Millette commentedAfter reading #17 I posted a followup #1702958: Add libraries:// stream wrappers to access system files .
Comment #21
chx CreditAttribution: chx commentedSorry 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.
Comment #22
pounardAgree 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.
Comment #23
Dave ReidComment #24
brianV CreditAttribution: brianV commentedBumping. 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?
Comment #25
chx CreditAttribution: chx commentedThat 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.
Comment #26
Crell CreditAttribution: Crell commentedThat makes sense to me. It still simplifies the API for people trying to refer to JS, CSS, etc. files.
Comment #27
brianV CreditAttribution: brianV commentedAttached 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?
Comment #28
brianV CreditAttribution: brianV commentedForgot to attach patch.
Comment #29
Lars Toomre CreditAttribution: Lars Toomre commentedAttached are some nit-picking review items from a documentation perspective:
'Definition of' should be 'Contains'. Changed in last few months.
Exceeds 80 characters and needs to rewrapped.
Throughout class docblocks, need to start one line summaries with an active verb. In this case, perhaps 'Provides support for ...'?
Suggest that this variable $options be renamed to $mask, as many in Drupal community think of $options as an array.
Ibid.
Needs a new line at end of the file.
Should have a blank line after '<?php' and before @file docblock. Same error reoccurs several times in this patch.
ProfileStream.php needs a blank line at end of that file.
Needs to be active verb.
Missing docblock.
Overrides from what class getExternalUrl()?
Missing newline at end of ThemeStream.php.
Comment #30
Xano"Supports..."
Comment #31
Crell CreditAttribution: Crell commentedLars: 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.
Comment #32
pounardWhile I really like the use of stream wrappers, a sum of details bothers me:
Aside of that that sounds cool.
Comment #33
fietserwinWhy 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?!
Comment #34
pounardActually 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.
Comment #35
dman CreditAttribution: dman commented@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.
Comment #36
pounardStream 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.
Comment #37
chx CreditAttribution: chx commentedNot 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)
Comment #38
Lars Toomre CreditAttribution: Lars Toomre commentedSorry @crell... I did not mean to change status.
Comment #39
fietserwinHaving 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.
Comment #40
brianV CreditAttribution: brianV commented@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.
Comment #41
fietserwinWhat 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.
Comment #42
Dave ReidI would strongly disagree with adding logic to 'disallow' certain extensions rather than documenting our best practices for including PHP files like normal...
Comment #43
brianV CreditAttribution: brianV commented@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.
Comment #44
Crell CreditAttribution: Crell commentedDoesn'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.)
Comment #45
brianV CreditAttribution: brianV commented@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.
Comment #46
pounard@#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.
Comment #47
fietserwinThere 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.
Comment #48
jthorson CreditAttribution: jthorson commented#1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend is now potentially complete, awaiting reviews and an RTBC.
Comment #49
jthorson CreditAttribution: jthorson commentedMarking back to needs review, as #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend has been marked as RTBC.
Comment #50
jthorson CreditAttribution: jthorson commentedHere's a re-roll with the duplicate LocalReadOnlyStream.php file from #1308054: Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend pulled out.
Comment #52
jthorson CreditAttribution: jthorson commentedRequeued on bot. Suspect the failure was due to broken HEAD in #347988: Move $user->data into own table.
Comment #54
fietserwinEven 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.
The new files in the patch don't contain a new line at the end of the file.
The global can be moved to the first if block. BTW: Is this still a global?
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).
Comment #55
jthorson CreditAttribution: jthorson commentedThanks, fietserwin ... realized the patch issue shortly after re-posting.
Comment #56
fietserwin#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.
Comment #57
fietserwin#50: 1308152-add-module-theme-profile-stream-wrappers-rev5.patch queued for re-testing.
Comment #59
brianV CreditAttribution: brianV commentedRe-rolled patch against latest HEAD. Let's see how it handles the tests.
Comment #60
chx CreditAttribution: chx commentedLet'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.
Comment #61
brianV CreditAttribution: brianV commentedHere'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:
It passes the
file_exists()
check, but fails on thefile_get_contents()
call. Possiblyfile_get_contents()
is passing a flag other than 'r' through toLocalReadOnlyStream::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?
Comment #62
xjmSending to the bot.
Comment #64
brianV CreditAttribution: brianV commentedLooks like it needs another reroll for HEAD. Will fail testing regardless as it doesn't fix the problems from #59
Comment #65
nod_Need this for #1996238: Replace hook_library_info() by *.libraries.yml file to reference JS and CSS files from yml files in modules.
Comment #66
sdboyer CreditAttribution: sdboyer commentedseconding @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.Comment #67
PanchoIt'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.
Comment #68
ParisLiakos CreditAttribution: ParisLiakos commentedlets start with a reroll
Comment #70
ParisLiakos CreditAttribution: ParisLiakos commentedthis fails because of #1954180: LocalReadOnlyStream::stream_open refuses to open a file when 'rb' flag is passed in
Comment #71
ParisLiakos CreditAttribution: ParisLiakos commentedshould be tested, after the issue above is committed
Comment #72
hass CreditAttribution: hass commentedCan you make sure we also have base theme support, please? I'm including a lot of files from the base theme in my subthemes.
Comment #73
Pancho@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.
Comment #74
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, in my system file_get_contents passes 'rb' flag..probably its same for bot.
this issue definitely needs this tag
Comment #75
ParisLiakos CreditAttribution: ParisLiakos commentedblocker is in!
Comment #76
sdboyer CreditAttribution: sdboyer commented@hass #72 - file a followup. don't derail this.
Comment #77
chx CreditAttribution: chx commentedGo 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.
Comment #78
fietserwin@param and @return missing
Comparing an array with a string? From the PHP manual : Arrays are always converted to the string"Array";
documentation missing
Again comparing an array with a string
Comment #79
Pancho#71: drupal-wrappers-1308152-71.patch queued for re-testing.
Comment #80
ParisLiakos CreditAttribution: ParisLiakos commentedthat should fix #78
Comment #81
PanchoLooks 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... :/
Comment #82
jthorson CreditAttribution: jthorson commentedFirst cut at adding some tests.
Comment #84
jthorson CreditAttribution: jthorson commentedOoops ... a bit of junk snuck in that last patch.
Comment #85
jthorson CreditAttribution: jthorson commentedCorrecting a cut and paste error (renaming the test class).
Comment #86
jthorson CreditAttribution: jthorson commentedUpdated interdiff (#80 to #85).
Comment #88
fietserwinI think it returns the name of the module, theme or profile.
Comment #89
Crell CreditAttribution: Crell commentedI don't think $GLOBALS['base_url'] is reliable anymore. We'll have to get this value from the request object.
Comment #90
PanchoWorking on this some more minutes.
Comment #91
PanchoSome 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.)
This should be for all streams not only system streams, so moving up to LocalStream::getTarget().
5.)
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...
Comment #92
fietserwin#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.
Comment #93
PanchoRe #92)
"Extension" is ambiguous, and while it works fine for contrib modules/themes, it's less obvious for modules like system or node.
"Component" is used in many contexts either and I don't find it obvious either. If we wanted to settle for "component", this needed to be enforced throughout core IMHO.
A "project" might consist of several modules, so would be the wrong concept here.
"Owner" is very descriptive from the stream perspective. The module/theme/profile owns a filespace, that might wherever be located, so this part is mapped (think of 'current' or 'default' or the different possible locations). Whatever is following is the hierarchical filesystem part that finally leads us to the owned file to be streamed.
Re 6.)
No more sure if we should add a theme engine wrapper. While it would be there for completeness, it probably wouldn't be used at all and could return only a PHP file which we didn't want to encourage. I'm rolling it in but we can throw it out again, if the tendency is to throw it out.
Regarding non-system libraries, we still don't know where they are located, if and how they are registered. We really should leave libraries to #1702958: Add libraries:// stream wrappers to access system files as a followup and postpone that one to #667058: Add a libraries folder with a README.txt in it to DRUPAL_ROOT.
7.)
We're still missing stream:://default. Added that.
8.)
The test really should actually be based on
UnitTestBaseDrupalUnitTestBase, not WebTestBase. Not sure if that works out, though...Even more and a new patch to come...
Comment #94
fietserwin#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.
Comment #95
PanchoThere'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.
Comment #97
PanchoForgot 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.
Comment #98
Pancho11.)
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?
Comment #99
Pancho"Needs review" in order to discuss #98.
Comment #100
fietserwinI 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;\\.
Comment #101
jibranRelated #2028109: Convert hook_stream_wrappers() to tagged services.
Comment #102
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #103
pwolanin CreditAttribution: pwolanin commentedLike 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).
Comment #104
effulgentsia CreditAttribution: effulgentsia commentedNo, the suggestion in #98 would be
drupal://module:node/css/node.admin.css
anddrupal://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.Comment #105
pwolanin CreditAttribution: pwolanin commented@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.
Comment #106
ParisLiakos CreditAttribution: ParisLiakos commentedwe dont enforce unique names among themes/modules at least not in d7
i suggest we stick to the issue title
Comment #107
jibran+1 to #106
Comment #108
thedavidmeister CreditAttribution: thedavidmeister commented+1 to #106
Comment #109
jcisio CreditAttribution: jcisio commented#106 even that, why not #98 or #102 for single scheme? KISS.
Comment #110
effulgentsia CreditAttribution: effulgentsia commentedHow is
system://module:node/
more KISS thanmodule://node/
?Comment #111
jcisio CreditAttribution: jcisio commented#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?
Comment #112
fietserwinI see a majority for sticking to module://, theme:// and profile://. Back to NW as per #96 (and #97). Pancho? (you assigned it to yourself)
Comment #113
ParisLiakos CreditAttribution: ParisLiakos commentedthats also true for
private://
andpublic://
and i think i like it the way it is
Comment #114
jcisio CreditAttribution: jcisio commented#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.
Comment #115
jcisio CreditAttribution: jcisio commentedOk, 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.
Comment #116
tstoecklerI 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.
Comment #117
ParisLiakos CreditAttribution: ParisLiakos commentedactually 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
Comment #118
ParisLiakos CreditAttribution: ParisLiakos commentedx-post
agree with #116
Comment #119
tim.plunkettPlease leave the title as is. +1 for module:// theme:// profile:// and actually just getting this done.
Comment #120
jcisio CreditAttribution: jcisio commented@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.
Comment #121
ParisLiakos CreditAttribution: ParisLiakos commentedjust 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+1000
Comment #122
sdboyer CreditAttribution: sdboyer commentedechoing #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://
andprofile://
. 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:essentially self-explanatory, assuming you know what a stream wrapper is.
now, here's that explanation for
system://
:now, here's that explanation for
drupal://
: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://
andprivate://
.Comment #123
fietserwinI 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)
Comment #124
sdboyer CreditAttribution: sdboyer commentedthough 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?
Comment #125
webchickI 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.
Comment #126
jthorson CreditAttribution: jthorson commentedI'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.
Comment #127
webchickSo 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.
Comment #128
jthorson CreditAttribution: jthorson commentedD.o tag monster
Comment #129
thedavidmeister CreditAttribution: thedavidmeister commentedIt might, but they'd be novice issues, right?
Comment #130
webchickProbably. 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.
Comment #130.0
disasm CreditAttribution: disasm commentedApplying template
Comment #131
disasm CreditAttribution: disasm commentedIssue 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...
Comment #133
Crell CreditAttribution: Crell commentedTo 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.
Comment #134
disasm CreditAttribution: disasm commentedI think I went to deep when I tried to fix a merge conflict. reverted ModuleHandler class, and it installs now.
Comment #135.0
(not verified) CreditAttribution: commentedforgot problem
Comment #136
BTMash CreditAttribution: BTMash commentedReassigning 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.
Comment #137
BTMash CreditAttribution: BTMash commentedComment #138
almaudoh CreditAttribution: almaudoh commented$start
will never beFALSE
because of the addition to 1.Better:
Comment #139
almaudoh CreditAttribution: almaudoh commentedRe-roll + #138
Comment #142
t0xicCode CreditAttribution: t0xicCode commentedComment #143
almaudoh CreditAttribution: almaudoh commentedRe-rolled patch with test fixes.
Comment #144
almaudoh CreditAttribution: almaudoh commentedOops! :p
Comment #146
ArlaComment #147
ArlaAs far as I can see, we cannot test ProfileStream::getOwnerName() for profile://current, because there is no current profile during a unit test.
Comment #148
ArlaThe 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?
Comment #152
almaudoh CreditAttribution: almaudoh commentedThanks for taking this up. Some quick reviews:
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?
Nice!! Shouldn't we restore the original settings in the tearDown() method? e.g.
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.
Comment #153
almaudoh CreditAttribution: almaudoh commentedThe main problem is that
\Drupal::container
is not the same as theKernelTestBase::container
and so the themes list is not carried over into\Drupal::container
whenKernelTestBase
issetUp()
. 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.
Comment #155
Arla#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...)
Comment #156
neclimdul2) The run method cleans up the environment directly by calling the restoreEnvironment method.
http://cgit.drupalcode.org/drupal/tree/core/modules/simpletest/src/TestB...
Comment #157
neclimdulScanned through some of the code. Wouldn't call it thorough but these things stood out.
unrelated but seems reasonable.
Should this get tested though?
originalSettings is for internal use. Sun will not be happy with you using it in your test. Mock what you need explicitly.
??
As noted, this is handled as part of the runner.
We generally put documentation before the code.
Comment #158
almaudoh CreditAttribution: almaudoh commented#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.
Comment #160
almaudoh CreditAttribution: almaudoh commentedI 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.
Comment #161
pwolanin CreditAttribution: pwolanin commentedFollowing Sun's suggesiton on the other issue seems to let it pass.
Comment #162
almaudoh CreditAttribution: almaudoh commentedWow!! 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 totestThemeStream()
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 :)
Comment #163
fietserwinSame error you already found elsewhere.
After throwing an exception this code does not need to be in an else branch.
$target = $uri_parts[1]; ???
We can use {@inheritdoc}, FALSE is not a string and should not be returned: throw an exception instead (like in the parent).
We can use {@inheritdoc},unless we want to document a @throws.
Should we document a @throws?
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.
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.
we can remove the else around this code.
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.
Same story here: FALSE is not a string. => use throw
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)
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
Comment #164
almaudoh CreditAttribution: almaudoh commented#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 uset()
, some useformat_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()
.Comment #165
fietserwinThanks for reworking the patch. As the interdiff is larger than the patch itself, it must have been some work.
(Seems to be in SystemStreamUnitTest.php)
Should be false in lowercase: https://www.drupal.org/coding-standards/docs#types
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)
This will always be true, so we can remove the if around the code.
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.
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?
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
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.
Comment #166
ArlaFixed 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()?
Comment #167
fietserwinI thought that we can no longer disable modules and themes, only uninstall them?
Why do we need to call the parent implementation here? This is a code smell.
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('');
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:
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:
Stream wrapper as an externally accessible resource:
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:
Stream wrapper as an externally accessible resource:
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?
Comment #168
ArlaI 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?
Comment #169
almaudoh CreditAttribution: almaudoh commentedI 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.
Comment #170
ArlaRerolled 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.
Comment #172
slashrsm CreditAttribution: slashrsm commentedFew minor comment. Otherwise looks OK and almost ready.
Could we use \Drupal::moduleHandler()->moduleExists()?
Also, no need to is_null() as NULL casts to FALSE.
themeExists()?
Can be removed if only calls parent.
Debug code.
Comment #174
fietserwinenable() does not exist, I think it should be install(), same for disable() below, replace with uninstall().
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.
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])));
Comment #175
almaudoh CreditAttribution: almaudoh commentedI 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:
getTarget()
,getDirectoryPath()
andgetOwnerName()
are all internal methods that help in the implementation of the external streamwrapper interfaces and should be protected methods.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.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.Additionally, even though
getTarget()
forSystemStream
wrappers is a slightly different implementation fromgetTarget()
inLocalStream
, its purpose is the same - to return the internal path relative to the scheme base path. The implementation is different becauseSystemStream
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'
, butmodule://
is not sufficient, you needmodule://{module_name}
. For this reason, I don't think we have to renameSystemStream::getTarget()
.I will rework the patch with above as well as other review comments shortly.
Comment #176
almaudoh CreditAttribution: almaudoh commented#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()
andgetOwnerName()
to protected methods. Removedfile_exists()
check ingetTarget()
and also removed all the optional $url
parameters except inLocalStream::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()
andgetOwnerName()
. I've removed tests on the protected methods and introduced tests for the public interface methods ofStreamWrapperInterface
, i.e.dirname()
andrealpath()
. 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()
andgetOwnerName()
, we can write PHPUnit tests for those in a separate issue.Comment #177
almaudoh CreditAttribution: almaudoh commentedRaised #2343491: Review and Redefine the Drupal 8 StreamWrapper API including error handling
Comment #179
almaudoh CreditAttribution: almaudoh commentedComment #180
almaudoh CreditAttribution: almaudoh commentedComment #182
pwolanin CreditAttribution: pwolanin commentedI 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?
Comment #183
almaudoh CreditAttribution: almaudoh commented#182: Raised #2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname()
Comment #184
fietserwinEmpty comment line before @param.
{@inheritdoc}
Defines the read-only profile:// stream wrapper for profile files.
{@inheritdoc}
Not used: can be removed.
Not correct english (at the semantic level).
{@inheritdoc} (will remove the non existing @param error)
{@inheritdoc}
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).
Comment #185
fietserwinTo 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.
Comment #186
almaudoh CreditAttribution: almaudoh commented@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'sdirname()
method which leaves a leading slash, so we didn't use the path separator inSystemstream::dirname()
We can properly clean-up the code in
LocalStream
andLocalReadOnlyStream
in the follow-ups.This should be RTBC now.
Comment #187
fietserwinThanks 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.
Comment #188
garphy CreditAttribution: garphy commented@fietserwin, regarding #187, which @return are still not properly documented in proposed patch ?
Comment #189
fietserwin1
2
3
4
Comment #190
JeroenTI documented the @returns as suggested by fietserwin in comment 189.
Patch attached.
Comment #191
fietserwinThanks JeroenT, but
@return string|null (according to the documentation on \Drupal\Core\Theme\ThemeNegotiatorInterface::determineActiveTheme())
Needs an empty line before the @return line as per the documentation standards.
Comment #192
garphy CreditAttribution: garphy commentedI'm taking care of it.
Comment #193
garphy CreditAttribution: garphy commentedTook care of the comment issues mentioned in #191.
Comment #197
garphy CreditAttribution: garphy commentedLet's trigger it one more time. Drupalcon folks are all either gone drinking or sleeping, right now.
Comment #199
fietserwinGood to go!
Comment #200
slashrsm CreditAttribution: slashrsm commentedYay!
Comment #201
almaudoh CreditAttribution: almaudoh commentedAdded #2350553: Replace uses of drupal_get_path() for CSS and JS files with module:// theme:// and profile://streamwrappers as followup.
Comment #202
catchI'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.
Comment #203
almaudoh CreditAttribution: almaudoh commented#202.1: This issue and #2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname() will help in limiting the usage of drupal_get_path(), so that a possible follow-up to address drupal_get_path() brittleness would be less disruptive to commit.
#202.2: A related issue #2028109: Convert hook_stream_wrappers() to tagged services. will turn stream_wrappers into services which would allow dependency injection. I'm not sure if that should go in before this one...
Is
Extension::getPath()
a suitable replacement for drupal_get_path()? That would fix #202.1 easily. What do others think?#202:3: Raised new issue #2351919: Replace uses of drupal_get_path() with __DIR__ where possible
. Thiswhile current issue will focus on the use cases where __DIR__ won't work (mostly modules that use other modules' resources).Will update issue summary.
Edit: update for more clarity.
Comment #204
fietserwinI can agree with the responses in #203 to the valid objections/concerns in #202.
I guess that would be as easy as to replace
with (and, if necessary, add proper exception handling to meet the method specification):
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.
Comment #205
almaudoh CreditAttribution: almaudoh commentedProfiles are still using old procedural code :(
Comment #206
fietserwinFor 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)?
Comment #207
catchThe 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.
This can also use the module handler no?
And here?
Is there ever a valid use case for this?
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.
Comment #208
fietserwinRE #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?
Comment #209
almaudoh CreditAttribution: almaudoh commented@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
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.
Comment #210
almaudoh CreditAttribution: almaudoh commentedUpdated issue summary
Comment #211
almaudoh CreditAttribution: almaudoh commentedRaised 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)
Comment #212
fietserwin@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.
Comment #213
catchI actually prefer
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:
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.
Comment #214
alexpottI 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?
Comment #215
Wim LeersYes!
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?
Comment #218
almaudoh CreditAttribution: almaudoh commented#2028109: Convert hook_stream_wrappers() to tagged services. just went in, attached patch was already in my repo.
Following #213, #214 and #215, the other use-cases not yet discussed (#2347783-8: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname()):
Comment #220
effulgentsia CreditAttribution: effulgentsia commentedI 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():
could become:
which has the added benefit of making the value of the #uri property an actual URI.
Comment #221
jhedstromComment #222
mgiffordLargely there's a change as function drupal_get_path() moved to bootstrap. Other minor stuff, but basically just a re-roll.
Comment #224
slashrsm CreditAttribution: slashrsm commentedComment #225
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedRerolled, replaced calls to removed functions, quick fixes, code style updates...
As the previous patch failed, I'm expecting the same with this one.
Comment #227
claudiu.cristeaNo, unfortunately we cannot inject services in a stream wrapper constructor because stream_wrapper_register() doesn't allow that. We can only pass the class name to the registering function. But I still kept the constructors because they can be swapped in a possible unit test.
What I've done:
SystemStreamTest
to make it more simple to be read. I removed the assertion messages because we don't use anymore messages in assertions.
Note
I really don't like the idea of having the pseudo-URIs like:
profile://current
,theme://current
,theme://default
andtheme://admin
. I would create separate stream wrappers for such cases:current-profile://
current-theme://
default-theme://
admin-theme://
Comment #228
claudiu.cristeaAre 'current', 'default' and 'admin' blacklisted on profiles and theme names? No. There's no reason to make such exceptions as
profile://current
,theme://current
,theme://default
andtheme://admin
. That's why I introduced in this patch 4 new schemes and also fixing their names (theme should be active, not current and profile should be installed, not current):installed-profile://
active-theme://
default-theme://
admin-theme://
Improved the tests.
Comment #229
tim.plunkettComment #230
fietserwinWith the possible name clashes you have a point, but on first sight I don't like that the resource part can now begin after the 1st or 2nd /. and yes that is different from the currently defined ones (public, private, temporary) but they serve different purposes.
We had similar discussions from #98 to #122 and there was a majority for sticking to as it was ...
Comment #231
claudiu.cristeaI'm totally missing the point here :)
I read the posts between #98 and #122 and they are about a totally different thing. Having an unified drupal:// (or system://) scheme vs. 3 different (module, theme, profile).
Comment #233
stevenlafl CreditAttribution: stevenlafl commentedMore than 4 years and this still hasn't been fixed. People must really really love typing
drupal_get_path('module', 'theme_test') . '/css/base-override.css'
every time they want to reference a file in their theme/module/etc.Suffice it to say I'm pretty damned disappointed. Can't we backport this to D7?
Comment #234
garphy CreditAttribution: garphy at ICI LA LUNE commentedWell... the reviewers must surely not have gained pretty much confidence that the issue is handled well. I did not see your review of the patch in the thread, though...
So... If you have time, feel free to check out the patch, test it and post your review.
Can you ?
So.. again... if your have time, feel free to work on a patch.
Comment #235
Wim Leers#233: that specific use case will be a thing of the past in D8. Thanks to asset libraries. See e.g. https://www.drupal.org/theme-guide/8/assets.
Comment #237
pounardJust a review of the patch, I think that DIRECTORY_SEPARATOR should not be used. If you build URI/URL with that, if windows tells you that separator is \ it will give you invalid URLs. As far as I know, if I remember correctly, windows gives you '/' but hey, that's not a directory separator if want to build URLs, but it is a whatever they call it in the RFC. Anyway, stricly PHP-speaking, whatever is the OS you're running in, '/' is *always* a valid directory separator. DIRECTORY_SEPARATOR should only be used when parsing filenames coming from the underlaying OS, but not be used in order to build URLs for the external world; in this case, my little finger tells me that the getLocalPath() method will serve URIs in the HTML too.
Comment #238
jonhattan#237 is absolutely right
Comment #239
claudiu.cristeaI still think this can go in 8, in 8.1.x.
Comment #240
claudiu.cristeaRerolled and applied #237.
Comment #241
Wim LeersThis would also address #2611246: [PP-1] Document the recommended method of creating file URLs to theme assets (e.g. images).
Comment #242
joelpittetThere is lots of nice code cleanup in this:)
+1 to getting these stream wrappers!
Does it need 'Pseudo' in the Base class names? We seem to have a convention where our abstract Base classes just end in Base. Just a nitpick but it caught my eye when skimming the patch.
Comment #243
claudiu.cristea@Wim Leers, added #2611246: [PP-1] Document the recommended method of creating file URLs to theme assets (e.g. images) as related issue.
@joelpittet, you're right. Fixed.
The problem with issue is that everybody wants this feature but there's no time for a good review and RTBC :)
Comment #244
Wim LeersSo, here's a review then :)
I have serious concerns about most of the stream wrappers besides
module://
andtheme://
.profile://
andinstalled-profile://
seem pretty useless, and potentially make code less portable.IMO this should be reduced in scope, to only do
module://
andtheme://
. For those, there are no risks.Also, the patch does not remotely match the IS.
Why do we need this?
This means the result of this effectively varies by the
theme
cache context. But none of this code and calling code is designed to have URLs return different values based on the context.Therefore I fear this will cause mysterious problems that are tricky to debug.
Similarly, these depend on configuration. Hence the generated file URLs will need to have the appropriate cache tag associated. So, this too is brittle.
Uses theme negotiator, hence needs
theme
cache context.Uses config, hence needs cache tag.
Depends on configuration, hence needs cache tag.
Cannot ever change, hence is fine.
(Though of questionable utility IMO.)
Missing newline in between.
Woah. This needs good unit test coverage.
As does this (need good unit test coverage).
Comment #245
claudiu.cristea@Wim Leers,
I think
profile://
andinstalled-profile://
can be merged asprofile://
and that should refer to the installed profile. I cannot see use-cases where you want to access files from a non-installed profile. But for the installed profile I can see the need of having such a wrapper. Aboutactive-theme://
,admin-theme://
,default-theme://
— I agree that they will overlap as resolved URLs withtheme://
. Are we OK to keep onlymodule://
,theme://
andprofile://
(as installed profile)?Comment #246
catchI'm still a bit perplexed by this patch. It doesn't do anything to untangle the mess that is drupal_get_filename(), so that makes it only useful for DX.
But drupal_get_path() is called less and less in 8.x, so the DX benefit is extremely narrow.
For example, we're down to 103 calls to drupal_get_path() in core from 177 in Drupal 7 https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...
Most of the remaining usages are in tests. Many of the core usages could probably be converted to Extension::getFileName() or Extension::getPath() rather than the stream wrapper. Or in some cases just use __DIR__ since they're referencing the module's own files.
103 calls is still quite a lot, but I'd rather see an issue that removes as many calls to those without the stream wrappers being available. I'm concerned that once this gets in, there'd be a mass conversion from drupal_get_path() to the stream wrapper which is not necessarily the right direction.
If we remove all the calls that are already superceeded, and there's still loads left, then the scope of the DX improvement here becomes a lot clearer.
Comment #247
Wim LeersI postponed #2611246: [PP-1] Document the recommended method of creating file URLs to theme assets (e.g. images) on this.
#245:
Good point!
#246: See #2611246: [PP-1] Document the recommended method of creating file URLs to theme assets (e.g. images), that IMO is a strong use case. Unless you see another elegant solution there.
Comment #248
almaudoh CreditAttribution: almaudoh commented#247 I agree that file URLs may be the one strong use case that justifies this issue. In the theming system we are working around similar limitations using
@my_theme/css/...
-style dynamic paths, which IMHO is not a very good solution because it requires custom code each time (see #2642122-4: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides for a case that streamwrappers would elegantly fix).In contrib world, this would be a very useful feature to avoid all sorts of bugs.
OTOH, I agree that parts (mostly PHP code) where
drupal_get_path()
, etc can be replaced with__DIR__
,Extension::getFileName()
orExtension::getPath()
should be removed from the scope of this issue. There's an old issue #2351919: Replace uses of drupal_get_path() with __DIR__ where possible where we can do that.Comment #249
Wim Leers#248: Thank you for your excellent comment!
I agree that #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides would also be elegantly solved by this.
Given #248, I wonder if we should postpone this issue on #2351919: Replace uses of drupal_get_path() with __DIR__ where possible.
Comment #250
claudiu.cristeaApplied #245. Still needs unit tests
Comment #253
claudiu.cristeaBot error.
Comment #254
catchPer #246 and #248 postponing on #2351919: Replace uses of drupal_get_path() with __DIR__ where possible. Once that's done we can see what's left.
Comment #255
almaudoh CreditAttribution: almaudoh commented@claudiu.cristea, thanks for keeping this issue alive. Quick review:
Do we need this hunk?
This implementation ignores whatever the user specifies as the profile name in
profile://profile_name/path/to/file
which would result in wierd behavior. We should use module handler to load the named profile here instead.So now that we're using KernelTestBase(NG), we can use proper PHPUnit
@dataProvider
in this test.Nitpick: blank line
These look out of scope.
Comment #256
claudiu.cristea@almaudoh, thank you for review.
#255.2
profile:// is only for the current profile. There's hard to find a use case for referring files from non-instaleld profiles. See #245 and #247.
Still needs tests.
@catch, I'm gonna set this to "Needs review" only to see the tests. After that I will revert back to Postponed.
Comment #258
claudiu.cristeaFix the test provider.
Comment #259
almaudoh CreditAttribution: almaudoh commentedHa! Was trying to fix the test fails and you beat me to it :). So here are some small nitpicks that were left...
Comment #260
almaudoh CreditAttribution: almaudoh commentedBack to postponed...
Comment #261
Wim Leers#2351919: Replace uses of drupal_get_path() with __DIR__ where possible has been committed, unpostponing.
Comment #263
claudiu.cristeaYes, nut the reroll is not straight.
Comment #264
alvar0hurtad0Here's the reroll, now the color_test.module file not exists sice this issue #2655580: Dead code: hook_system_theme_info() removed but is still implemented
Comment #265
almaudoh CreditAttribution: almaudoh commentedComment #266
almaudoh CreditAttribution: almaudoh commentedCan we make a case for this patch to be committed now on the strength of cases like #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides
Comment #268
benjy CreditAttribution: benjy at PreviousNext commentedI really like this idea, a few points:
This seems like a funny place to validate the uri, it wasn't passed into this method and its a class property, can we validate in the setter?
Same validation is happening here.
Just checking, so it only works with the "enabled" profile?
Comment #269
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedAgreed with the review comments from #268.
profile:// is explicitly for the enabled profile as per #245.
I'm not sure if LocalStreamTrait should be a trait. Both methods work with a uri property, so would it be better as a LocalStreamUri class? Also, the trait is missing a phpdoc comment.
This code feels like a good amount of it could be covered by unit tests, instead of just functional tests.
Should this be \RuntimeException, or a subclass of it? Same question in the ThemeStream.
Period here instead of a comma?
Also, there's a few places where uri / url is lowercased in the docs text, and they should probably be uppercased.
Comment #270
claudiu.cristeaI added a use-case to replace the color test removed in #264.
#268.1,2: Moved in setter. But because there's a lot of code duplication between
LocalStream
andReadOnlyStream
I added a base class for all wrappers:StreamWrapperBase
that provides the common properties and methods.#269: The reason why that is a trait is because those 2 methods are used in LocalStream, then are overridden in ExtensionStreamBase and, finally, used again in ProfileStream. I didn't want to duplicate the code in ProfileStream.
I don't think those are \RuntimeException, but this is not very string belief. Other opinions? Also I see uri/ul with lowercases around, in streams. If it's wrong I would let that to a followup to cover all cases. Thank you for review.
Comment #271
claudiu.cristeaAdded related issue #2700509: Remove code duplication in read-only stream wrappers.
Comment #272
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedMy main thought behind \RuntimeException is that it's dependent on modules being enabled or disabled, which is a runtime state. \InvalidArgumentException makes more sense to me in cases where an argument is never going to be correct.
The trait makes sense to me now, I think it's fine to keep as is.
Here's a small update that fixes calling dirname() with a $uri parameter as in drupal_dirname().
For anyone interested, I've backported this patch (currently #264) over to https://www.drupal.org/project/system_stream_wrapper so we can support a library:// stream wrapper there in existing sites. My aim is to keep that as close to identical to this patch as I can, so that if this lands in 8.2 sites can just uninstall that module and use the core implementation.
Comment #273
juampynr CreditAttribution: juampynr at Lullabot commented@file statements were recently removed from core. See #2665992: @file is not required for classes, interfaces and traits
Is this method name correct? Shouldn't it be providerInvalidStreamUri?
Comment #274
juampynr CreditAttribution: juampynr at Lullabot commentedAh, ignore number 2 in my previous comment. I see the annotation now.
Comment #275
juampynr CreditAttribution: juampynr at Lullabot commentedWhile testing this patch along with Moment.js module, I am getting the following at the Status report:
Here a screenshot of the debugger. Note that the stream wrapper URL that moment module defines is correct:
I am debugging why the stream wrappers are not getting registered.
Comment #276
juampynr CreditAttribution: juampynr at Lullabot commentedRight, I can see that the StreamWrapperManager recognizes them. They are simply not getting registered:
Comment #277
juampynr CreditAttribution: juampynr at Lullabot commentedOh, now I see, #272 does not implement the LibraryStream. Is that intended @deviantintegral? I see that there has been some discussion about it, but I am not clear on what is the decision.
Comment #278
almaudoh CreditAttribution: almaudoh commented@juampynr
library:
streamwrapper isn't part of the scope of this issue. There is a different issue for that: #1702958: Add libraries:// stream wrappers to access system files .Comment #279
juampynr CreditAttribution: juampynr at Lullabot commented@almaudoh, yes I just saw the issue and I don't understand why it is out of scope. How could then modules share third party libraries in Drupal 8?
I went ahead and ported the library stream wrapper out of https://www.drupal.org/project/system_stream_wrapper into this patch. I tested it against @deviantintegral's Moment.js sandbox and verified that it works (I had to make a small adjustment at the sandbox module, though).
If this does not make sense, we can just continue working on the patch at #272.
Comment #280
juampynr CreditAttribution: juampynr at Lullabot commentedHere I have removed the
@file
docblocks.Comment #281
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedAdding the library stream wrapper makes sense to me. This patch:
Comment #283
Wim Leers?!??
I don't understand the recent changes at all. Is this trying to get Libraries API into core in a hidden way? There is no such thing as "library extensions". This feels like massive scope creep.
Comment #284
almaudoh CreditAttribution: almaudoh commentedI agree with Wim,
libraries:
is out of scope. This issue is about stream wrappers for extensions. Libraries are not yet extensions (though there's an issue for that somewhere).Libraries API module can add its own stream wrapper if needed.
Edit: additionally introducing a new ExtensionDiscovery subclass is a totally different direction from current Extension system work #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList
Comment #285
SKAUGHTCurrently, yes. However could be needed as some point..
creep happens.
Comment #286
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedThis update:
Comment #287
dawehnerJust a quick note: #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList could simplify this patch quite a bit.
Is it possible to inject this via the constructor?
Comment #288
almaudoh CreditAttribution: almaudoh commentedJust what I've been saying too :) Many other things too would also be unblocked by that patch and this patch. When will they get the attention of the committers? :/
Comment #289
claudiu.cristea@dawehner,
No, unfortunately we cannot inject services in a stream wrapper constructor because stream_wrapper_register() doesn't allow that.
Comment #290
jibranNW for needs tags.
Comment #291
claudiu.cristea@jibran, Yes, except "Needs tests". Tests were provided in the patch. If you feel that we need to test a specific feature of this patch, not included in patch tests, feel free to re-tag and please specify which.
Comment #292
hass CreditAttribution: hass commentedCannot find a way how to attach a library pointing to public://
Comment #293
claudiu.cristeaWill come tomorrow with IS update. Setting to NR to see the tests.
Comment #294
claudiu.cristeaFixed the IS.
Comment #295
dawehnerJust a saturday borning review :)
Could we use ltrim here and call it a day?
What about storing the request stack on the object. This makes it safe against changing someone pushing/popping request onto the stack.
This is indeed basically identical to the parent method, but is this needed in this issue? I'm just asking
While reading this line, I'm wondering whether we should think about #2692403: Make info and configuration inheritable in profiles already. Could we maybe use
profile://installed
or so to indicate the current one, so we can in the future provide parents somehow as well?This seems to be changed by accident
Comment #296
claudiu.cristeaComment #297
claudiu.cristea@dawehner, thank you.
module://node
module://node/
module://node/node.info.yml
So, I don't understand where to use
ltrim()
here. The URI might contain also a subdir or subdir and file.Comment #298
claudiu.cristeaUpdated also the Draft CR: https://www.drupal.org/node/2352923
Comment #299
dawehnerFor admin themes its easier. You can add that later. I totally agree with you to keep the patch simple. For profiles though, it would be quite hard to keep BC, don't you think so?
Comment #300
dawehnerOkay sure, let's follow that route. We will figure out the install profile inheritance later anyway. Sorry for the inconvenience!
Oh sorry, I talked a bit of BS. strtok is what you want (see https://3v4l.org/V9B14)
Comment #301
claudiu.cristea@dawehner, I don't believe in profile://installed because "installed" is not blacklisted as extension/profile name. Instead we need dedicated stream wrappers for those pseudo-extensions. See the reasons from #227.
Comment #302
claudiu.cristeaRight :)
Comment #303
dawehnerThank you!
Comment #304
claudiu.cristeaThe latest interdiff is wrong. Sorry
Comment #305
catchStill not keen on changes like this. Why can't this do something like $this->container->get('theme_handler')->getTheme('test_theme')-getPath().
Comment #306
claudiu.cristea@catch, yes, we can use that too. But this is the nice thing using stream wrappers — it's more simple and intuitive. In fact this is the scope of the issue. #305 is almost the same code as that used by the stream wrapper.
Comment #307
catch@claudiu.cristea I don't agree that it's more simple and intuitive.
Instead of a method call that clearly shows where the data is coming from and has a relatively clean stack trace (or at least could if we cleaned the extension system up more), we're invoking php stream wrappers which then calls back out to Drupal. It's obfuscating what happens rather than making it clearer.
Also are there really no non-test cases in core this could be applied to?
I said the same thing in #13 (back in 2012) and #202 by the way.
Comment #308
claudiu.cristeaOK. I reverted the test case and added a non-test case.
Comment #309
heddnSmall nits on docs only.
The issue summary provides a great example of module://{name}. Let's use it here in the docs.
Ditto. The issue summary provides a great example of theme://{name}.
This doesn't work with image styles, because the URL is transformed from:
theme://custom_theme/logo.png
into:
theme://styles/issues_landing/theme/custom_theme/logo.png
Which causes getOwnerName to fail:
RuntimeException: Theme styles does not exist or is not installed
Same would apply to modules me thinks.
Comment #310
heddnAdditional note:
ImageStyle::load('thumbnail')->buildUrl('themes/custom/custom_theme/logo.png');
results in a 404.
Because ImageStyleDownloadController converts the above url into:
public://themes/custom/libre_custom_theme/grey-logo.png
The file doesn't exist in the public file system. It exists in the theme.
Seems like a catch 22.
Comment #312
claudiu.cristea@heddn, the failures from #309 and #310 are caused by a bug in the image.module. The bug was fixed in #2770761: Derivatives should not be created on read-only stream wrappers. You can try again.
Comment #313
claudiu.cristeaFixing #309.
Comment #315
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commented+100 for this. This would allow you to ship images with modules and apply image styles to them. The current workaround is either to basically implement this (although a much simpler approach can be used for a one off) or to store the file as a managed file and track it's ID, manage changes to your file etc.
Comment #316
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedneeded re-roll for fuzz and search test file moved to core/modules/search/tests/src/Functional/SearchSimplifyTest.php
couldn't use patch for 2 files - needed manual application of changes:
core/lib/Drupal/Core/StreamWrapper/LocalStream.php
core/lib/Drupal/Core/StreamWrapper/ReadOnlyStream.php
so please double check those are right.
===
seems not:
PHP Fatal error: Access level to Drupal\Core\StreamWrapper\ModuleStream::getDirectoryPath() must be public (as in class Drupal\Core\StreamWrapper\LocalStream) in /var/www/html/core/lib/Drupal/Core/StreamWrapper/ModuleStream.php on line 15
Comment #319
vijaycs85Comment #320
dawehnerAfter fighting with #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList for quite a while, I fear we would need some profiling here as well, for example of the installation.
Comment #322
aspilicious CreditAttribution: aspilicious commentedAsked for a retest, composer can't apply on drupal 8.5 locally.
Comment #323
aspilicious CreditAttribution: aspilicious commentedComment #324
borisson_This still needs the profiling requested in #320 by @dawehner.
Attached patch fixes some cs errors.
There was one place in the tests where this was changed, so I ran that test to do some prelimary profiling.
With patch.
Without patch:
I ran the test one time before doing the repeated test, to make sure that nothing weird was going on. If this is something to go by, there is a difference, but it is not significant at all.
Comment #325
Wim LeersIt looks like the only thing that remains here is profiling!
Comment #328
aspilicious CreditAttribution: aspilicious commentedRerolled
Comment #330
aspilicious CreditAttribution: aspilicious commentedForgot some files
Comment #332
izus CreditAttribution: izus commentedhi,
#324 no more applies
here is a reroll of it
Thanks
Comment #333
izus CreditAttribution: izus commentedComment #336
pingwin4egHey, there's a movement to allow multiple enabled profiles in one installation - #1356276: Allow profiles to define a base/parent profile. So I suppose, the profile stream wrapper should contain the profile name too?
Comment #337
zipymonkey CreditAttribution: zipymonkey commentedRerolling #332 so it applies to 8.8.x.
Comment #338
almaudoh CreditAttribution: almaudoh commentedTesting the patch in #337
Comment #340
hanoiiIt's #337 with an attempt to fix tests. Applies to both 8.8.x and 8.7.x.
Comment #341
AaronMcHaleThis issue started in 2011 and the discussion about not using this for referencing PHP files was quite early on, I believe APC was cited as the reason, is that still the case? I ask because it would be really nice if this could be used in any case where you need to reference a file regardless of if it's a PHP file or a static asset. That would create a nicer developer experience and result in less confusion, where you only have to use one approach consistently instead of switching between approaches depending on the context, which could easily result in the wrong use.
Also just wanted to clarify if this will be available in Twig templates? I suspected it will be and so would be a huge bonus, but just wanted to clarify that.
Comment #342
hanoiiAnd btw, we tested this and it's working properly along with previous tests, so I am marking it as RTBC.
Comment #343
alexpottThis still needs profiling - that's been an outstanding task for a while.
@AaronMcHale can you clarify how this would be used in Twig templates?
Comment #344
hanoii@alexpott what kind of profiling is needed and any guidelines for me to help with that?
Comment #345
AaronMcHale@alexpott re Twig: I was thinking if someone needed to reference/include some kind of resource, but then remembered that in most cases you'd still need to use variables like
{{ base_path }}
or{{ directory }}
, even concatenating them together in some cases.Building on that thought, since Twig is already parsing the Twig HTML files, I wonder how easy (or not easy) it would be to make the stream wrappers usable directly in the HTML, for example
<img src="theme://theme/images/image.jpg">
, maybe even in CSS files, since I think Drupal already does some scanning and manipulation when caching and aggregating CSS files.On another note, going back to what I said about including PHP files, now that I think about it there's probably almost no reason contrib needs to actually include other PHP files, since generally PHP files should contain classes and would be already autoloaded.
Comment #348
mdupontRe-rolled against 8.9.x since it was no longer applying after a file rename in the Search module.
Comment #350
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commentedSolving failed test case, kindly review a new patch.
Comment #352
narendra.rajwar27Comment #355
AndyF CreditAttribution: AndyF at Fabb for FRUITION commentedAdd profiling as a remaining task for extra clarity (the issue's already tagged).
Comment #356
mdupontRe-roll against 9.2-dev since the patch failed to apply. Let's see if it passes the tests.
Comment #358
raman.b CreditAttribution: raman.b at OpenSense Labs commentedRequired another re-roll after #3177541: stream_open() needs to cope with a failure in \Drupal\Core\StreamWrapper\LocalStream::getLocalPath() better
Comment #360
alexpottAll these dependencies can be injected now.
This looks dated... needs to link to an issue too.
SystemStream? This example looks dated.
Can use a return typehint
This should be injected too.
This change is actually a regression. The test is a core search test. It should be able to make assumptions about its path - invoking the extension system that means that search module can be relocated and changed makes the behaviour of this unpredictable.
Comment #361
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented360.1:
DI added.
360:2
I'm leaving this for someone who has more context.
360.3
I'm leaving this for someone who has more context.
360.4
Return typehint added.
360.5
DI added.
360.6
Do we just want to revert this change? I'm leaving that for others.
Comment #363
ricovandevin CreditAttribution: ricovandevin at Finlet for Carwebs commentedis_file('module://path/to/file.jpg')
leads to an ArgumentCountError with the patch in #361:Comment #366
bradjones1Migrated this to an MR. Re-rolled with some fuzz but no rewriting necessary.
Addressed #360 points 2, 3 and 6 which were outstanding from changes made in #361. Specifically, I reverted the change to the search test, which seems out of scope. It could theoretically use
__DIR__
in the line that was changed, but so could other things be improved in that test :-)Let's see what testbot yields.
Comment #367
bradjones1Comment #368
bradjones1@ricovandevin are you observing that we aren't fuzzing the tests sufficiently? In your example the module would be
path
, and the file inside would beto/file.jpg
?Comment #369
bradjones1Is there a standard procedure for profiling? I'm not familiar and can't find anything in docs. This page isn't particularly clear about a standard procedure.
Comment #370
ricovandevin CreditAttribution: ricovandevin at Finlet for Carwebs commented@bradjones1 That is correct. In our specific use case we have an images folder inside the module folder and we want to point to an image in that images folder using the module:// stream wrapper.
Debugging shows that in case of
is_file()
thegetWrapper()
method of theStreamWrapperManager
is never called.It works when
is_file($path)
is replaces withis_file(\Drupal::service('file_system')->realpath($path))
. But we cannot rely on all occurrences ofis_file()
being updated.Comment #371
ricovandevin CreditAttribution: ricovandevin at Finlet for Carwebs commentedAs
is_file()
seems to lead to the wrapper classes being instantiated directly I guess we need to have a fallback for the injected services.Not sure if the approach I added in the MR is ok and what a better alternative would be. But I'm happy to learn from whoever has a better solution. :-)
Comment #372
BerdirAdded some comments to the MR, contains some .orig files that should be removed.
Comment #374
mondrakeRerolled and addressed @Berdir's points.
Comment #375
mondrakeI reverted the change to the color.module file because there are several other places in that file that would require the same change for consistency, but I am not sure we want this when it is code requiring to access a file. AFAICS it's preferrable to use the extension.list.module service in that case.
Rather, I am trying here to change the
image.settings.preview_image
which is possibly closer to the use case here i.e. user input in config.EDIT - Also see #2774449-2: Edge case image derivative URIs collision
Comment #376
mondrakeMR push above xposted with @alexpott's comments in the MR.
I cleaned up dependency injection as all injected args in constructors should be mandatory - we're adding a new API here, no need to play the BC dance we're doing with existing API.
For ProfileStream, I can't manage to get the installed profile available at constructor's time to inject, in the context of a kernel test. Calling setInstalledProfile() works, but only for sebsequent access to the container. For the moment I am expliciting getting it from the container in getOwnerName(), till a better solution can be found.
Comment #377
BerdirI commented on the DI aspect, I'm not so sure that works.
> I reverted the change to the color.module file because there are several other places in that file that would require the same change for consistency, but I am not sure we want this when it is code requiring to access a file. AFAICS it's preferrable to use the extension.list.module service in that case.
Not sure about this, but I'm happy to push that to a follow-up issue, but that's IMHO like saying modules shouldn't use public://.
Comment #378
mondrake@Berdir is right about DI, it does not work. We need to remove dependency injection then, in favor of \Drupal::service calls instead.
Comment #379
catchPer #1308152-13: Add stream wrappers to access extension files, more than nine years ago... I'm still struggling with the use case for this - it's hundreds of lines of code to support (unless I missed something), this single change in core:
Previous use-cases discussed in this issue, like CSS and JS, have evaporated as we've improved our APIs elsewhere (i.e. module.libraries.yml references relative paths to assets, and generally modules deal with each other's libraries instead of specific files).
The examples in the issue summary aren't examples of things that modules actually need to do often, and include referencing PHP files which was ruled out a long time ago for stream wrappers due to opcode caching, as well as also being something that has no use-case as the surface of procedural code continues to shrink.
Comment #380
BerdirYeah, that being the only example now isn't really showing this in a great light.
However, that only works for a core module, a contrib module might be in multiple places and even a core module could be as an override in different place. Edge case, but we do technically support that. For a contrib module, doing what image module does is simply not possible. It would need hook_install() do dynamically set the path or provide an empty default and that is then hardcode the default in code. Or do what media_install() does, copy them into public://, which also uses the API and not just hardcode the path.
And if we include examples in code, for example ckeditor plugins like \Drupal\ckeditor\Plugin\CKEditorPlugin\DrupalImage (which can't use the library system).
Based on my reroll of the drupal_get_path() issue, batch include files is still a fairly common use case in core to reference PHP files, but that's just because we didn't convert those to classes/services yet (batch API does currently not support services).
In my install profile on 9.2, I'm seeing ~100 calls to drupal_get_path() with a hardcoded module name (
drupal_get_path('module', '
), sure, a ton of that is tests. And another 150 in our contrib modules.Comment #381
mondrakeThis is also a blocker to #3218514: [PP-1] Deprecate passing path (instead of true URI) to ImageStyleInterface::buildUri($uri) which would be good tightening.
Comment #382
mondrakeThe way I see this issue is mainly an attempt to get the System Stream Wrapper module into core. There's a clear statement in this direction in the project page, and looking now at the code I see a lot of what's there ported here.
Agree this is mostly relevant in contrib (I would be certainly using these wrappers it in Image Effect for instance), and the number of installs of System Stream Wrapper shows there's a need for this.
Unfortunately there's no stable release for that module, and stable modules likely won't be keen on requiring it as a dependency.
So the options here IMHO are either won't fix this and get a stable release for SSW, or get a core supported set of wrappers and make SSW redundant.
Last commit removes DI and addresses some of @alexpott's points.
Comment #383
mondrakeCannit figure out the PHP 8 test failure - it’s not the test failing, rather PHPUnit or the bridge on DrupalCI. Locally tests pass, as do in DrupalCI on PHP 7.3
Comment #384
mondrakeComment #385
mondrakeHm, test failures may be because of this https://github.com/sebastianbergmann/phpunit/issues/4347
Comment #386
mondrakeI’m working on simplifying the patch.
Comment #387
mondrakeSignificantly simplified the patch, now it only adds the new stream wrappers without touching preexisting ones, and most of the work is done in the ExtensionStreamBase class.
Comment #388
mondrakeRerolled
Comment #389
DuaelFrThis looks good! Nice work here!
@catch #379: I found this issue while trying to get an image from my theme in a responsive image style process. I could not find any other way to acheive that without placing my source picture in the public files (which is not ideal). Now I can just use this trick in my template (or do the same in a preprocess):
Comment #390
clayfreemanI know the discussion around
library://
is largely over(?), but I thought it pertinent to mention this change record, which seems like a natural fit for a stream wrapper so that contrib doesn't need to bother with using\Drupal::service('library.libraries_directory_file_finder')->find()
. Instead, contrib could just refer tolibrary://name/path/to/resource
.Also, libraries don't have to be purely CSS or JS - in fact, sometimes external libraries are required for other things (e.g., server-side compilation of Sass or TypeScript resources). The use cases could be infinite, so having a stream wrapper would be extremely convenient. At the end of the day, libraries are just a folder on disk, so they should be treated as such. The difference is that the folder could exist in any one of the directories listed in the change record.
Comment #391
mdupontAll threads on the Merge Request were addressed, and it can still be merged into 9.3.x, what is missing to move it to RTBC?
Comment #393
aspilicious CreditAttribution: aspilicious commentedMondrake could you rebase your branch? It is failing to apply at the moment.
Comment #394
mondrake@aspilicious no I cannot, what needs to be done here is:
a) @bradjones1 should change the target branch to 9.4.x or 10.0.x, only the MR creator can
b) someone to pull from the MR branch
c) that someone to merge locally with the 9.4.x fresh HEAD, resolve conflicts, git add+commit
d) that someone to push the result to the MR branch
or
a) someone to create a new MR from 9.4
b) apply locally the patch from the current MR, possibly with 3way merge
c) resolve any conflict, git add+commit
d) push the result to the new MR branch
Comment #395
aspilicious CreditAttribution: aspilicious commentedI thought the merge request stuff would make it easier... :D
Creating a good old regular patch file.
I think it contains everything from the MR?
Comment #396
bradjones1Target changed on the MR.
Comment #398
mondrakeRerolled
Comment #399
geek-merlinNow it needs reroll onto 9.5.x. Maybe a new MR.
Comment #400
mondrakeBeing a feature request, honestly I think this is for no sooner than 10.1. See ya in a year or so!
Comment #401
mondrakeComment #402
geek-merlinBump...