Problem/Motivation
The core/vendor directory is for third-party vendor code kept in its original shipped form. Since its contents are auto-generated using Composer and it contains only PHP code, some Drupal core would like for it to only contain that PHP; additionally, they'd like the ability to make the entire folder inaccessible from the webroot.
However, there are 3rd party vendor could that must be web accessible, such as jQuery, Normalize.css, etc. And we are frequently seeing patches that try to update that code to Drupal coding standards.
We need to separate those 3rd party asset files from the rest of Drupal's code base.
Proposed resolution
Move 3rd party vendor code from core/misc to core/assets/vendor.
The rationale for using "core/assets/vendor" is currently summarized in comment #24.
Remaining tasks
Move the reset of core/misc into various core/assets/css, core/assets/js, and core/assets/images folders.
User interface changes
No user interfaces are changed.
API changes
No API changes are made as the paths are handled by system_library_info().
Comment | File | Size | Author |
---|---|---|---|
#32 | 1473066-32-misc-vendor.patch | 138.91 KB | echoz |
#29 | 1473066-29-misc-vendor.patch | 138.91 KB | JohnAlbin |
#29 | 1473066-interdiff-26-29.txt | 1.33 KB | JohnAlbin |
#26 | 1473066-26-misc-vendor.patch | 139.3 KB | JohnAlbin |
#24 | 1473066-misc-vendor.patch | 138.96 KB | JohnAlbin |
Comments
Comment #1
sunOne thing is clear:
If we're going to do this, then we will remove the additional
lib/
directory "separator" in modules, too.#1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch] decided that we have to have a
lib/
directory to separate the namespaced PHP class code from any other code.--
Personally, I don't think it's a good idea to intermix PHP code with front-end assets within
/core/vendor
.OTOH, I'd love to see that
lib/
in modules to go away, so I'd be fine either way, but definitely not both at the same time.Comment #2
RobLoachlib/
is for the given project's PSR-0 PHP classesvendor/
is for third party code (not limited to PHP)They're two different things. Let's keep the
lib
discussion over at #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch] and #1431804: Bikeshed 'lib' vs. 'libraries' directory. This is to move jQuery UI intovendor
since it is third party code. See Crell's post about it.Comment #3
pounardI agree with sun, mixing frontend code and PHP code sounds bad.
Comment #4
nod_Please deceide if we can use
core/vendor/
for JS, otherwise close this and it will leave incore/js/vendor/
#1663622: Change directory structure for JavaScript files.We're not mixing front-end and back-end code, we're grouping third party dependencies, that's different.
Comment #5
nod_meh
Comment #6
pounard@nod_ vendor/ is a PHP library dir, ideally it would be outside the webroot. I like to consider that all public assets should live somewhere in sites/ instead, away from PHP code.
Comment #7
RobLoach@pounard Web assets can be hosted in vendor packages in
Resources/public
directories. I wrote up how we could accomplish something like this at https://github.com/mattfarina/Lootils/issues/1 .In the mean time, #1663622: Change directory structure for JavaScript files seems like a good first step.
Effectively, we'd end up with a hook_library_info() 2.0 which would handle all the integration for us.
Comment #8
pounardThe Resources/public dir sounds like a Symfony convention where to place public assets into components or bundles. We're not speaking about bundles but about self-contained JS scripts. Putting PHP and public JS assets side by side is missing the point, they should not live together. Looking at some other CMS (other technos but still) they actually copy all the assets on module enabling into a common public folder, which makes much more sense since it frees the code of any contraints of where to live (webroot or not), and it makes all the public assets for the client live in a self-contained minimal public webroot folder. I think third party JS libraries must not be mixed with PHP code in the vendor dir, we could use another folder somewhere else to put those, it would make much more sense.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe way I see it, jquery, jquery-ui, jquery-form, etc. are all part of the core javascript framework and owned by the system module (which implement
hook_library()
for them). As a consequence they should live undercore/modules/system
(in apublic
orassets
folder).Comment #10
nod_That's what Rob proposed in the other issue (that can probably be closed), seems like we agree about that.
As for the name seems like symfony uses
public
so, let's go with that?Comment #11
nod_Comment #12
jhodgdon#1663622: Change directory structure for JavaScript files seems to be leaning in the direction of lumping Drupal-sourced and third-party code together under modules/system.
I think that is a bad idea, because of the reasons stated in the issue summary above:
Comment #13
RobLoachNeeds the build system up first. I'll hopefully have some time very soon to work that out.
Comment #14
webchickI'm not sure why this is postponed, but it's exceedingly dumb that we moosh together our JS and vendor JS in the same folder. IMO all third-party code should be in core/vendor. It makes it much easier to exclude it from searches and find/replace stuff.
Comment #15
jhodgdon+1000000 on #14. PLEASE.
Comment #16
nod_Was postponed since we wanted to avoid the tedious folder-bikeshed before feature freeze since that's not holding anything back.
Comment #17
RobLoachI sent a pull request up to components/jquery to add Composer support. This is the shim/built repository that's used by the Bower Package Manager. Would be nice to use the same repository as the JavaScript peoples. Then, whenever they put out an update, we'd also get it.
We could do the same for components/jqueryui, and any other packages we need.
Comment #18
jhodgdonUmmm. How is #17 related to this issue? This issue is about making *directory structure* for the third-party JS libraries, not about how to maintain them...
Comment #19
RobLoach@jhodgdon That would give us the directory structure in core/vendor, as webchick pointed out in #14.
Comment #20
nod_Ok I'm ready to deal with it. What RobLoach says. We put our deps in composer.json and let composer handle it, would make updating libraries easier since that'll happen often in D8.
As for the system module I'm pretty sure we want a system.library.php or whatever with only system_library_info(), the function is currently 1012 lines long and sucks to read.
On this topic, I'd be open to generate some of the library definition with composer and put that somewhere kind of like what's done for autoload_classmap.php.
Anyway, it's time to get all this js out of core/misc.
Comment #21
jhodgdonThe issue summary needs an update. What is the plan here?
Comment #22
webchickThis is just annoying, it doesn't actually break anything. Demoting back to normal.
Comment #23
nod_Most of the action is happening in #1663622: Change directory structure for JavaScript files. I guess we can leave that one open to deal with non-third party scripts that are in core/misc.
Comment #24
JohnAlbinGiven that some people want to move the PHP files in core/vendor out of a webroot (seems reasonable to me), let's put 3rd party assets that need to be web accessible in a directory besides core/vendor.
Over in #1663622: Change directory structure for JavaScript files, we had a round-and-round discussion which I'll summarize here:
core/vendor
core/libraries
(andcore/libraries/vendor
) because we already have a similarly-namedcore/lib
directory and it is way too late to changecore/lib
tocore/src
or whatever.core/js
,core/css
, andcore/images
directories and also usecore/js/vendor
,core/css/vendor
, andcore/images/vendor
directories. But that's a mess.core/js
,core/css
, andcore/images
directories and also usecore/assets-vendor
orcore/vendor-assets
directories. And when people seecore/vendor-assets
orcore/assets-vendor
sitting next tocore/vendor
and look inside to see normalize and jquery, etc., they'll think “wtf? why is this separated fromcore/vendor
?".core/assets/vendor
,core/assets/js
,core/assets/css
, andcore/assets/images
. And since I don't see a better option…Here's the patch that implements option #5.
Looking at the number of minus signs in .jshintignore makes me happy.
Comment #24.0
JohnAlbinUpdated issue summary.
Comment #25
JohnAlbinI should note that the directory names I used under core/assets/vendor were either taken from the Bower registry, which the majority of the code was already registered with. The 1 or 2 libraries that weren't registered with Bower all had GitHub repositories, so I took the project part of their URL as the directory name. In other words, I didn't come up with the names of those directories arbitrarily; they are the names chosen by the webdev community at-large.
Comment #26
JohnAlbinWhoops. Missed one reference to a moved misc/ file in ckeditor_library_info(). New patch with 1 line change from previous patch.
Comment #26.0
JohnAlbinUpdated issue summary.
Comment #27
nod_+1 +1 +1 +1…
Tested the patch, couldn't break it and i see no references to core/misc for vendor libs.
Comment #28
Wim Leers+1 of course :)
But:
I'm pretty sure this is unrelated?
Comment #29
JohnAlbinGood catch! I was playing around with using Bower to package the libraries and the GitHub version of Farbtastic doesn't have a 1.2 tag. I've reverted that change in the latest patch.
Incidentally, the change right after that I purposefully put in; that's the new jQuery dependency for Farbtastic. The dependency exists (you can see the jQuery variable in the last line of the farbastic.js) but was undeclared in the info hook.
Comment #30
Wim LeersIndeed, I saw that, and that's good :)
Back to RTBC!
Comment #31
alexpottNeeds a reroll
Comment #32
echoz CreditAttribution: echoz commentedRe-roll!
Comment #33
JohnAlbinConfirmed the patch in #32 is a proper re-roll.
Comment #34
JohnAlbinRemoving "needs reroll" tag.
Comment #35
catchDrives me mad having everything in /misc, committed/pushed to 8.x.
Comment #36
JohnAlbin*Fist pump*
Ok! We'll start working on patches for the rest of core/misc. :-D
Comment #37.0
(not verified) CreditAttribution: commentedAdded link to rationale of using core/assets/vendor