Problem/Motivation
The core/misc directory is a mess. Lots of random scripts, images, stylesheets, etc. Completely unorganized.
Proposed resolution
- Move the assets that are defined in core.libraries.yml that are in core/misc/js to core/assets/js (see #57-#63).
Remaining tasks
- Make a new patch for this issue
- Review patch
- Commit patch
- Open the box
User interface changes
No UI changes.
API changes
No API changes since it's handled by hook_library_info().
Original report by _nod
Reusable scripts are currently in /core/misc
next to random images and other things.
We need to have a proper structure for putting javascript sources, minified files and third party scripts.
I'm proposing the following structure in core/js
src/
src/lib/
reusable components like tabledragdist/
same structure assrc/
with minified/uglyfied filesvendor
third party scripts used in core (like jquery.forms.js, jquery.ba-bbq.js)
this need some more thought, I just want to put it out there. Contrib should also follow the convention.
Comment | File | Size | Author |
---|---|---|---|
#110 | 1663622-nr-bot.txt | 154 bytes | needs-review-queue-bot |
#87 | modules-js-files.txt | 3.1 KB | Chi |
#86 | core-js-proper-folder-1663622-86.patch | 31.91 KB | stefan.r |
Comments
Comment #1
RobLoachRegarding third-party vendor code, I'd like to see:
Then it would really discourage us from touching those files. At the very least, we should most definitely work more closely with npm and grunt. Those tools will allow us to automate a lot of what we currently do manually.
Comment #2
nod_Move all the core/misc JS files, still need to take care of all the modules files.
This will make test fails, just don't know how many :p
Comment #4
nod_oups, missed a couple of /js/vendor
Comment #5
pascalduez CreditAttribution: pascalduez commentedThat's definitely a good move, and the vendor structure/naming is pretty neat. Thanks for this.
+1 for considering the use of grunt in managing core js.
Comment #6
nod_haha it passed. need to be verified though
Missed a core/misc,
Comment #7
RobLoachThis is a great move and helps clean up the core/misc folder greatly. Since the System module currently handles all of its assets in
core/misc
, what are your thoughts on moving those defined in system_library_info() tocore/modules/system/assets
or something similar? Moving some of core/misc over to modules/system might help decouple it a bit... I'd be willing to do the re-roll.The discussion of where third-party vendor code should go can continue over at #1473066: [Meta] Move third-party assets out of core/misc folder. The proposed location of it here seems appropriate for this issue.
Comment #8
nod_oh yeah moving that to system would make sense, that way we only have 1 rule about the location of js files.
I have no time to take care of the reroll, please go ahead if you can, thanks.
Comment #9
jhodgdonAn issue summary describing which directory structure the patch is trying to do would be helpful. :)
Comment #10
nod_All right, not a serious or very helpful comment, but a funny one: http://xkcd.com/1077/ :)
Comment #11
RobLoachSounds like we want to move them to core/modules/system/assets then. There are some pathings in #1341792: [meta] Ship minified versions of external JavaScript libraries too. We should probably get that one in first so that updating the locations arn't too too crazy.
Comment #12
jhodgdonSo what's the current proposal, in total -- could someone update the issue summary?
Comment #12.0
jhodgdoncontrib
Comment #13
RobLoachThanks... Updated the summary.
Comment #14
jhodgdonTHANK YOU! I love the first line: "The core/misc directory is a mess. ". Well put!
So I have a couple of questions about this proposal:
a) It seems like many contrib projects have been using the convention that JavaScript files go into a "js" sub-directory. This makes more sense to me than inventing a new directory name "assets" that isn't being used anywhere else (to my knowledge). Is there any particular reason why "assets" is a better idea?
b) This proposal doesn't address other JS files that are in core/misc not covered by system_library_info(). There are a few, I think?
c) In general, because of coding standards not applying to third-party code, it is really helpful to have Drupal vs. third-party code separated into separate directories. I guess this is covered by other issues, but I just mention it here because the proposal in the summary doesn't say that third-party libraries are to be handled differently:
#1473066: [Meta] Move third-party assets out of core/misc folder
#1298304: [Meta] Move all externally-developed files to core/vendor/{vendorname}/ directories
Comment #15
RobLoachProjects like jQuery UI are not just limited to JavaScript. They have both JavaScript and stylesheets, hence assets. We could go with another name though. It'll likely be moved out of there via #1473066: [Meta] Move third-party assets out of core/misc folder though. "public" in the Resources namespace is what Symfony uses. Might be good to go with that then.
Likely to be a lot more.
Correct. I'll be working on some packaging for external assets that will help with this.
Comment #16
jhodgdonRegarding 'assets' -- OK... Most contrib projects seem to have a separate js and css directory, but I can see the wisdom of combining related JS and CSS so they are together.
Comment #17
sxnc CreditAttribution: sxnc commentedOkay, i made a patch that moves all the .js files from core/misc to core/modules/system/assets. I also modified the JavaScriptTest.php, system.module and core/includes.common.inc to reflect the new paths.
Comment #18
sxnc CreditAttribution: sxnc commentedre-submitting patch, previous version did not contain the moved files (it only deleted them).
Comment #19
tim.plunkettRerolled using the configuration detailed here http://drupal.org/node/1542048, specifically
git config diff.renames copies
Certainly shrinks the patch size :)
Comment #20
sxnc CreditAttribution: sxnc commentedRe-submitt full patch, there were 2 additional files missing: "vertical-tabs-rtl.css" and "vertical-tabs.css".
Comment #21
sxnc CreditAttribution: sxnc commented@tim.plunkett dang it, i didn't refresh when i submitted my patch again... But thanks A LOT for that git rename config thing, we were looking for that! Here comes a much smaller and hopefully issueless patch~
Comment #22
nod_in common.inc there is still a
core/misc/drupal.js
that should be renamed as well :)Once that is fixed and the testbot is happy that's RTBC :)
Comment #23
sxnc CreditAttribution: sxnc commented@nod_ should be fixed now :) dang it, that was a sloppy mistake...
Comment #24
nod_Great, I'll rtbc after the testbot finishes.
Comment #26
nod_All right, so core/misc is used all over core since it's not only in library definitions.
So either we wait for the explicit dependencies patch to get in and we can just use this patch. Or we have to replace every call to drupal_add_js that adds a file from this directory.
Comment #27
nod_Actually there is a lot more depending on this dependencies patch. Postponing as it will be easier afterwards.
Comment #28
nod_Patch is in, it's on!
So now the only path that needs changing should be in the
system.module
insystem_library_info()
.Comment #29
RobLoachJam is a package manager for JavaScript. It is an interesting system, but still quite young. It handles downloading the correct versions of packages that you want.
I've talked briefly with Scott González about having jQuery UI support for this. He's open to the idea, but it's not on the immediate radar. Kat Bailey, Crell and many others helped with switching up to Composer to handle our PHP dependencies, would be neat to have something like it handle our JavaScript dependencies too.
Not saying it's something we should do, just something to consider.
Comment #30
klonosI'd open a new issue and set it to postponed if I were you. Just so to make sure that the idea and the specific proposal won't be forgotten.
Comment #31
nod_nice and easy
Comment #33
nod_All right, forgot the tests :-°
Comment #35
nod_Quite a lot of hardcoded paths in tests.
Comment #37
nod_Comment #38
jhodgdonI don't think that 3rd-party files, such as Farbtastic and jQuery UI, should be going into core/modules/system:
These really need to be in their own space. Wasn't that the plan?
Comment #39
nod_Well, they don't belong in /misc either and the PHP guys don't want assets in /vendor
So I'm open to putting it anywhere else, just need to know where. We're replication the misc structure in assets, which is not great but way better than the current structure.
Comment #40
jhodgdonHow about system/assets/vendor, so that we at least know they're third-party?
Comment #42
nod_reroll. This patch is not using assets/vendor as I'm still unsure if people would OK it.
Comment #43
nod_was missing one replace in a test.
Comment #44
jessebeach CreditAttribution: jessebeach commentedre #39, I think we need to push back a bit on /core/vendor. Looking ahead to inclusion of libraries like Backbone and Underscore, I'm hesitant to bury them in core/system/assets. We should have all third-party code under a single directory instead of making an arbitrary PHP / non-PHP distinction.
Who objected to /core/vendor? We just need to sit down and chat with them.
Comment #45
nod_Let's open that back up december 2nd.
Comment #46
RobLoach#43: core-js-proper-folder-1663622-42.patch queued for re-testing.
Comment #47
RobLoachDidn't want to "needs review". Just was curious if the file locations have changed. Ignore me :-) .
Comment #48
jhodgdonBy the way, there's another issue open that is related to this effort:
#1298304: [Meta] Move all externally-developed files to core/vendor/{vendorname}/ directories
(actually it lists this issue in its summary). And this one should have been long ago tagged with "coding standards", so doing that now.
Comment #49
boruis CreditAttribution: boruis commented#4: core-js-proper-folder-1663622-4.patch queued for re-testing.
Comment #50
boruis CreditAttribution: boruis commentedsorry, wrong click. ignore me.
Comment #51
nod_see #44
Comment #52
JohnAlbinIt was sun and pounard. #1298304-12: [Meta] Move all externally-developed files to core/vendor/{vendorname}/ directories
But the “full discussion” is over at #1473066: [Meta] Move third-party assets out of core/misc folder
Why not core/libraries and core/libraries/vendor? All of our 3rd-party JS and CSS code are defined in hook_library_info(). I agree with Jesse; burying stuff in core/modules/system/assets is not a good plan.
I think the underlying issue is that it would be very nice if our 3rd party code was easily packaged and bundled with Drupal. Currently, composer only works with PHP code and it is extremely unlikely that it will be adopted for wide-use in front-end-oriented projects. IMO, Bower looks to have the most wide-spread support. What experience do front-end developers have for auto-building projects with 3rd party assets? Do these tools affect our choice of directory structure?
Comment #53
nod_There is already a lib/ directory so libraries/ would look silly. vendor would still be the best. Here is a couple of bower files that gets most of our dependencies.
Just apply, install bower and run
bower install
. Really neat.An on the hook_library_info() topic (I'll open a new issue, it's just that there are more people following this one for the moment). How about declaring assets in an MYMODULE.assets.yml file instead of a hook_library_info()? I got scoled by sun a while back because there shouldn't be anything dynamic in the library_info hook, so it'd make sense. Would address #1969916-5: Remove drupal_add_js/css from seven.theme too.
( edit ) and here is the issue #1996238: Replace hook_library_info() by *.libraries.yml file.
Comment #54
ry5n CreditAttribution: ry5n commentedForgive me if I missed something, but have we thought about just /core/assets? It’s not buried, and we could choose (sensibly) to put 3rd-party code in either core/assets/vendor or core/vendor/assets :)
Here is a sketch of what this might look like:
Comment #55
JohnAlbincore/vendor/assets has been vehemently argued against. *sigh* I say let's just go with core/assets and core/assets/vendor
I'm playing with nod_'s patch in #53 to see if I can get the 3rd party vendor assets to organize themselves into the correct directory. :-)
Comment #56
JohnAlbinI've started a branch moving stuff over in the mobile sandbox. http://drupalcode.org/sandbox/johnalbin/1488942.git/shortlog/refs/heads/...
All of the vendor libraries I've looked so far seem to have Bower registration (even if they aren't working), so I'm using those Bower registrations to determine which directory under core/assets/vendor they should go under. e.g. normalize is going under core/assets/vendor/normalize-css
Comment #57
JohnAlbinI realized I was about to upload a patch to the wrong issue. I've uploaded a patch to #1473066-24: [Meta] Move third-party assets out of core/misc folder that moves all the 3rd party vendor code from core/misc to core/assets/vendor.
I think the remaining core/misc/*.js files should be moved to core/assets/js/*.js.
Comment #58
jhodgdonDo we still need this issue now that #1473066: [Meta] Move third-party assets out of core/misc folder has been taken care of?
Comment #59
JohnAlbinAs I said before: I think the remaining core/misc/*.js files should be moved to core/assets/js/*.js.
I'd like to move everything out of core/misc and into more structured folders.
Having the 3rd party libraries in core/assets/vendor, I believe, is the best separation we can get from other CSS/JS stuff given the rest of our directory structure.
Comment #60
nod_umm if you look at drop button you have CSS and js files. would be good to keep that grouped.
Comment #61
ry5n CreditAttribution: ry5n commentedRE: #60 Agree. In #54 I suggested core/assets/components for this purpose. Here’s a more readable version:
Comment #62
mavimo CreditAttribution: mavimo commentedPatch to add bower as manager to external libraries.
NOTE:
If this solution is approached I can work on required changes in PHP code.
Comment #63
jessebeach CreditAttribution: jessebeach commented+1 to the structure proposed by ry5n in #61.
Comment #63.0
jessebeach CreditAttribution: jessebeach commentedUpdated issue summary.
Comment #64
LewisNymanComment #65
stefan.r CreditAttribution: stefan.r commentedComment #66
stefan.r CreditAttribution: stefan.r commentedComment #67
stefan.r CreditAttribution: stefan.r commentedThis patch moves:
misc/*.js => assets/js
misc/dropbutton/*.js => assets/js/dropbutton
misc/dialog/*.js => assets/js/dialog
As there is also some CSS in dropbutton and dialog, it moves that to assets/css/dropbutton and assets/css/dialog.
Comment #69
stefan.r CreditAttribution: stefan.r commentedComment #71
stefan.r CreditAttribution: stefan.r commentedThis patch moves misc/print.css and misc/vertical-tabs.css as well, while we're moving CSS files for dialog and dropbutton anyway.
Comment #73
stefan.r CreditAttribution: stefan.r commentedComment #75
stefan.r CreditAttribution: stefan.r commentedSorry for 3 failing tests in a row, it seems drush really wants drupal.js to be in core/misc/drupal.js, or it will refuse to install drupal at all :)
I'll post a new patch that leaves drupal.js in core/misc. I'll also move the dropbutton and dialog files to core/assets/components, as suggested by @nod_, @ry5n and @jessebeach in #54, #60, #61.
Comment #76
stefan.r CreditAttribution: stefan.r commentedComment #77
hass CreditAttribution: hass commentedWhy are you not changing the file path in file_exists?
Comment #78
stefan.r CreditAttribution: stefan.r commented@hass that is drush code and I don't think I can modify that with just a core patch ;)
Ideally Drupal 8 core itself provides a function that does this root check for drush, so we don't run into this issue every time we change paths of files.
Moving drupal.js will break a whole lot of drush installs (and tests will be red as it can't pass the root check in drush bootstrap level 1), so whether to move that file or not should probably be discussed in another issue.
I've created a Drush issue in any case: https://github.com/drush-ops/drush/issues/1105
Comment #79
hass CreditAttribution: hass commentedHuh... sorry thought it is core that is causing errors. That is really a problem if drush get's broken that easy. But drush for D8 is DEV only, so it should be ok to break it and fix in follow up. Core often change things... nothing to worry until final version is out.
Comment #80
stefan.r CreditAttribution: stefan.r commentedThe only problem with moving this file is that as soon as in core drupal.js is not in core/misc anymore, everyone using drush on D8 will get a "Drupal installation could not be found" error, and may lose some time before figuring out it's because
drupal.js
has moved and they need to update drush.In any case, I've submitted a pull request to drush to remove that check (which isn't actually needed), so as soon as it gets merged and the drush install on the test server includes the fix we can move drupal.js without making tests go red. But that shouldn't block this issue, we can always do a follow-up to move that one last file.
Perhaps as a compromise we can move
drupal.js
tocore/assets/js/drupal.js
and temporarily create an emptycore/misc/drupal.js
placeholder file so as to not break drush (to be taken out before the final 8.0.0 release, when hopefully more people have upgraded to a more recent version of drush).Comment #81
stefan.r CreditAttribution: stefan.r commentedThis patch moves
drupal.js
as well, leaving a placeholder file atcore/misc/drupal.js
so as to not break drush and the testbot.Comment #82
Manuel Garcia CreditAttribution: Manuel Garcia commentedComment #83
RobLoachCouldn't we make Drush use a different method to find Drupal's root? Why hold ourselves back for Drush to catch up?
Also, since the assets are called "drupal.batch", "drupal.collapse", etc, shouldn't we rename the files as such too? "drupal.batch.js", "drupal.collapse.js", etc.
Comment #84
stefan.r CreditAttribution: stefan.r commented@RobLoach the renaming of assets makes sense, we should probably create another issue for that?
As to the Drupal root finding, other than looking for drupal.js in the new location, I can't really think of any other method without parsing text files. From Greg Anderson @ https://github.com/drush-ops/drush/issues/1105 :
A fix has been committed to Drush, just waiting for testbots to use the new version now (#2422915: Upgrade Drush on qa.d.o to alpha9) so that removing the placeholder won't give a test failure.
Comment #85
stefan.r CreditAttribution: stefan.r commentedLooks like the testbot doesn't depend on Drush anymore, so the drupal.js placeholder can be safely taken out now.
Comment #86
stefan.r CreditAttribution: stefan.r commentedre-roll
Comment #87
Chi CreditAttribution: Chi commentedCan we also change directory structure for javascript files situated inside core/modules directory? Some modules keep those files inside js directory while others keep them inside module top level directory. I propose moving all javascript files to respective js directory for the sake of consistency.
Note that all modules css file are situated with the same pattern.
Comment #88
nod_@Chi, see related issue #2484623: Move all JS in modules to a js/ folder
Comment #89
stefan.r CreditAttribution: stefan.r commented@nod_ what do we want to do with this issue? Have you decided on a final directory structure yet?
Comment #90
droplet CreditAttribution: droplet commentedas simple as #2484623: Move all JS in modules to a js/ folder get rejected, has no hope for this one :(
I'm really sad and don't understand why Drupal end up with this pattern. we have standard libs and promote to use it, but we can't do internal refactoring because we think everyone would linking these files directly..
Comment #91
nod_Especially since we use libraries, we have library override and extending from the info file, the actual path don't matter much. and if they do it's a trivial fix.
Comment #92
SKAUGHTI think from here there's two sides of this ticket.
[#12]i think of what remains TODO about the core/misc folder is about putting which every js/css/img (by their used module) into the core module that uses them. Off hand, i think most would go into System Module.. but that's just off hand. of course, backtracking needs to be done.
I would hope this would help with the individual core module maintainers -- rather than just keeping a blank pile of asset/js | asset/img | asset/css. Making it clearer for those working with feature branches, etc. of what connects to where..
Comment #93
SKAUGHTComment #94
SKAUGHTremove +#1440628 -- not related. that issue was poorly titled.
Comment #95
SKAUGHTComment #96
catchThis could be done in a minor release.
Additionally even with library_override, it would be possible to provide a bc layer mapping the old paths to the new ones.
Comment #102
andypostComment #110
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.