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

  1. Make a new patch for this issue
  2. Review patch
  3. Commit patch
  4. 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 tabledrag
  • dist/ same structure as src/ with minified/uglyfied files
  • vendor 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.

Files: 
CommentFileSizeAuthor
#87 modules-js-files.txt3.1 KBChi
#86 core-js-proper-folder-1663622-86.patch31.91 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,453 pass(es). View
#81 core-js-proper-folder-1663622-81.patch47.13 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,132 pass(es). View
#76 core-js-proper-folder-1663622-76.patch29.88 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,139 pass(es). View
#73 core-js-proper-folder-1663622-73.patch31.96 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#71 core-js-proper-folder-1663622-71.patch26.53 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,101 pass(es), 4 fail(s), and 1,093 exception(s). View
#69 core-js-proper-folder-1663622-69.patch29.88 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#67 core-js-proper-folder-1663622-67.patch29.88 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Tests/Ajax/DialogTest.php. View
#62 bower-in-core-1663622-62.diff854 bytesmavimo
#53 core-bower-1663622-53.patch586 bytesnod_
#43 core-js-proper-folder-1663622-42.patch99.87 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 41,980 pass(es). View
#42 core-js-proper-folder-1663622-41.patch99.85 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 41,897 pass(es), 1 fail(s), and 0 exception(s). View
#37 core-js-proper-folder-1663622-37.patch100.22 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 40,694 pass(es). View
#35 core-js-proper-folder-1663622-35.patch100.77 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,702 pass(es), 3 fail(s), and 0 exception(s). View
#33 core-js-proper-folder-1663622-33.patch89.47 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,525 pass(es), 11 fail(s), and 0 exception(s). View
#31 core-js-proper-folder-1663622-32.patch71.55 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,526 pass(es), 12 fail(s), and 8 exception(s). View
#23 1663622-23.patch29.18 KBsxnc
FAILED: [[SimpleTest]]: [MySQL] 40,235 pass(es), 4 fail(s), and 46 exception(s). View
#21 1663622-21.patch29.1 KBsxnc
FAILED: [[SimpleTest]]: [MySQL] 40,241 pass(es), 5 fail(s), and 43 exception(s). View
#20 1663622-19.patch895.24 KBsxnc
FAILED: [[SimpleTest]]: [MySQL] 40,241 pass(es), 5 fail(s), and 43 exception(s). View
#19 drupal-1663622-19.patch28.68 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 40,239 pass(es), 5 fail(s), and 45 exception(s). View
#18 1663622-18.patch890.31 KBsxnc
FAILED: [[SimpleTest]]: [MySQL] 40,236 pass(es), 5 fail(s), and 45 exception(s). View
#17 1663622-17.patch457.31 KBsxnc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1663622-17.patch. Unable to apply patch. See the log in the details link for more information. View
#6 core-js-proper-folder-1663622-6.patch113.62 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 36,995 pass(es). View
#4 core-js-proper-folder-1663622-4.patch113.1 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-proper-folder-1663622-4.patch. Unable to apply patch. See the log in the details link for more information. View
#2 core-js-proper-folder-1663622-2.patch113.08 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 36,983 pass(es), 3 fail(s), and 66 exception(s). View

Comments

RobLoach’s picture

Issue tags: +JavaScript

Regarding third-party vendor code, I'd like to see:

core/vendor
├── cowboy
│   └── jquery-bbq
├── jquery
│   ├── jquery
│   └── jquery-ui
├── malsup
│   └── form
├── symfony
└── twig
    └── twig

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.

nod_’s picture

Status: Active » Needs review
FileSize
113.08 KB
FAILED: [[SimpleTest]]: [MySQL] 36,983 pass(es), 3 fail(s), and 66 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, core-js-proper-folder-1663622-2.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
113.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-proper-folder-1663622-4.patch. Unable to apply patch. See the log in the details link for more information. View

oups, missed a couple of /js/vendor

pascalduez’s picture

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

nod_’s picture

FileSize
113.62 KB
PASSED: [[SimpleTest]]: [MySQL] 36,995 pass(es). View

haha it passed. need to be verified though

Missed a core/misc,

RobLoach’s picture

This 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() to core/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.

nod_’s picture

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.

jhodgdon’s picture

An issue summary describing which directory structure the patch is trying to do would be helpful. :)

nod_’s picture

All right, not a serious or very helpful comment, but a funny one: http://xkcd.com/1077/ :)

RobLoach’s picture

Status: Needs review » Needs work

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

jhodgdon’s picture

Title: Give JS a real folder of his own » Change directory structure for JavaScript files

So what's the current proposal, in total -- could someone update the issue summary?

jhodgdon’s picture

Issue summary: View changes

contrib

RobLoach’s picture

Thanks... Updated the summary.

jhodgdon’s picture

THANK 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

RobLoach’s picture

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?

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

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?

Likely to be a lot more.

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

Correct. I'll be working on some packaging for external assets that will help with this.

jhodgdon’s picture

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

sxnc’s picture

Status: Needs work » Needs review
FileSize
457.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1663622-17.patch. Unable to apply patch. See the log in the details link for more information. View

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

sxnc’s picture

FileSize
890.31 KB
FAILED: [[SimpleTest]]: [MySQL] 40,236 pass(es), 5 fail(s), and 45 exception(s). View

re-submitting patch, previous version did not contain the moved files (it only deleted them).

tim.plunkett’s picture

FileSize
28.68 KB
FAILED: [[SimpleTest]]: [MySQL] 40,239 pass(es), 5 fail(s), and 45 exception(s). View

Rerolled using the configuration detailed here http://drupal.org/node/1542048, specifically git config diff.renames copies
Certainly shrinks the patch size :)

sxnc’s picture

FileSize
895.24 KB
FAILED: [[SimpleTest]]: [MySQL] 40,241 pass(es), 5 fail(s), and 43 exception(s). View

Re-submitt full patch, there were 2 additional files missing: "vertical-tabs-rtl.css" and "vertical-tabs.css".

sxnc’s picture

FileSize
29.1 KB
FAILED: [[SimpleTest]]: [MySQL] 40,241 pass(es), 5 fail(s), and 43 exception(s). View

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

nod_’s picture

Status: Needs review » Needs work

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

sxnc’s picture

Status: Needs work » Needs review
FileSize
29.18 KB
FAILED: [[SimpleTest]]: [MySQL] 40,235 pass(es), 4 fail(s), and 46 exception(s). View

@nod_ should be fixed now :) dang it, that was a sloppy mistake...

nod_’s picture

Great, I'll rtbc after the testbot finishes.

Status: Needs review » Needs work

The last submitted patch, 1663622-23.patch, failed testing.

nod_’s picture

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.

nod_’s picture

Status: Needs work » Postponed

Actually there is a lot more depending on this dependencies patch. Postponing as it will be easier afterwards.

nod_’s picture

Status: Postponed » Needs work

Patch is in, it's on!

So now the only path that needs changing should be in the system.module in system_library_info().

RobLoach’s picture

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

klonos’s picture

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

nod_’s picture

Status: Needs work » Needs review
FileSize
71.55 KB
FAILED: [[SimpleTest]]: [MySQL] 40,526 pass(es), 12 fail(s), and 8 exception(s). View

nice and easy

Status: Needs review » Needs work

The last submitted patch, core-js-proper-folder-1663622-32.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
89.47 KB
FAILED: [[SimpleTest]]: [MySQL] 40,525 pass(es), 11 fail(s), and 0 exception(s). View

All right, forgot the tests :-°

Status: Needs review » Needs work

The last submitted patch, core-js-proper-folder-1663622-33.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
100.77 KB
FAILED: [[SimpleTest]]: [MySQL] 40,702 pass(es), 3 fail(s), and 0 exception(s). View

Quite a lot of hardcoded paths in tests.

Status: Needs review » Needs work

The last submitted patch, core-js-proper-folder-1663622-35.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
100.22 KB
PASSED: [[SimpleTest]]: [MySQL] 40,694 pass(es). View
jhodgdon’s picture

I don't think that 3rd-party files, such as Farbtastic and jQuery UI, should be going into core/modules/system:

rename from core/misc/farbtastic/farbtastic.css
rename to core/modules/system/assets/farbtastic/farbtastic.css

These really need to be in their own space. Wasn't that the plan?

nod_’s picture

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.

jhodgdon’s picture

How about system/assets/vendor, so that we at least know they're third-party?

nod_’s picture

FileSize
99.85 KB
FAILED: [[SimpleTest]]: [MySQL] 41,897 pass(es), 1 fail(s), and 0 exception(s). View

reroll. This patch is not using assets/vendor as I'm still unsure if people would OK it.

nod_’s picture

FileSize
99.87 KB
PASSED: [[SimpleTest]]: [MySQL] 41,980 pass(es). View

was missing one replace in a test.

jessebeach’s picture

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

nod_’s picture

Status: Needs review » Postponed

Let's open that back up december 2nd.

RobLoach’s picture

Status: Postponed » Needs review
RobLoach’s picture

Status: Needs review » Postponed

Didn't want to "needs review". Just was curious if the file locations have changed. Ignore me :-) .

jhodgdon’s picture

Issue tags: +coding standards

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

boruis’s picture

Status: Postponed » Needs review
boruis’s picture

Status: Needs review » Postponed

sorry, wrong click. ignore me.

nod_’s picture

Status: Postponed » Needs work

see #44

JohnAlbin’s picture

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.

It was sun and pounard. #1298304-12: [Meta] Move all externally-developed files to core/vendor/{vendorname}/ directories

My stance on this issue is:

/vendor is for PSR-0 namespaced code. If you have PSR-0 namespaced code, put it there. If your code isn't, then you don't.

But the “full discussion” is over at #1473066: [Meta] Move third-party assets out of core/misc folder

Move the assets that are defined in system_library_info() that are in core/misc to core/modules/system/assets

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?

nod_’s picture

FileSize
586 bytes

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.

ry5n’s picture

Forgive 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:

core/
  assets/
    vendor/
      backbone/
      ckeditor/
      jquery-ui/
      modernizr/
      normalize/
      underscore/
    css/
      base.css <- SMACSS base styles for core
    js/
      ajax.js
      announce.js
    images/
    fonts/
    components/ <- components define complex ui objects & bundle js, css, images…
      collapse
        collapse.js
      utilities/
        utilities.layout.css <- .container-inline, etc.
        utilities.accessibility.css <- .element-invisible, etc.
        utilities.js.css <- JS util classes like .js-show
      buttons/
        css/
          buttons.css <- only essential styles; themes implement look-and-feel
      progress/
        css/
          progress.css
        js/
          jquery.progress.js
JohnAlbin’s picture

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

JohnAlbin’s picture

I'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

JohnAlbin’s picture

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

jhodgdon’s picture

Do we still need this issue now that #1473066: [Meta] Move third-party assets out of core/misc folder has been taken care of?

JohnAlbin’s picture

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

nod_’s picture

umm if you look at drop button you have CSS and js files. would be good to keep that grouped.

ry5n’s picture

RE: #60 Agree. In #54 I suggested core/assets/components for this purpose. Here’s a more readable version:

core/
  assets/
    vendor/
    css/
    js/
    components/
      dropbutton/
      …
    fonts/
    images/
mavimo’s picture

FileSize
854 bytes

Patch to add bower as manager to external libraries.

NOTE:

  • Some libraries are unmantained (jquery-bbq last commit is 3years ago, come PR are available on GithHub but are ignored, ..)
  • Some libraries require change in path (eg: jquery.ui need to be jquery-ui, html5shiv need to be html5shiv/src, ..)

If this solution is approached I can work on required changes in PHP code.

jessebeach’s picture

+1 to the structure proposed by ry5n in #61.

jessebeach’s picture

Issue summary: View changes

Updated issue summary.

LewisNyman’s picture

Issue tags: +frontend
stefan.r’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
stefan.r’s picture

Issue summary: View changes
stefan.r’s picture

Status: Needs work » Needs review
FileSize
29.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Tests/Ajax/DialogTest.php. View

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

Status: Needs review » Needs work

The last submitted patch, 67: core-js-proper-folder-1663622-67.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
29.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 69: core-js-proper-folder-1663622-69.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
26.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,101 pass(es), 4 fail(s), and 1,093 exception(s). View

This patch moves misc/print.css and misc/vertical-tabs.css as well, while we're moving CSS files for dialog and dropbutton anyway.

Status: Needs review » Needs work

The last submitted patch, 71: core-js-proper-folder-1663622-71.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
31.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 73: core-js-proper-folder-1663622-73.patch, failed testing.

stefan.r’s picture

Sorry 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 :)

function drush_valid_drupal_root($path) {
  if (!empty($path) && is_dir($path) && file_exists($path . '/index.php')) {
    // Drupal 8 root. Additional check for the presence of core/composer.json to
    // grant it is not a Drupal 7 site with a base folder named "core".
    $candidate = 'core/includes/common.inc';
    if (file_exists($path . '/' . $candidate) && file_exists($path . '/core/misc/drupal.js') && file_exists($path . '/core/core.services.yml')) {
      return $candidate;
    }

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.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
29.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,139 pass(es). View
hass’s picture

Why are you not changing the file path in file_exists?

stefan.r’s picture

@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

hass’s picture

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

stefan.r’s picture

The 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 to core/assets/js/drupal.js and temporarily create an empty core/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).

stefan.r’s picture

FileSize
47.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,132 pass(es). View

This patch moves drupal.js as well, leaving a placeholder file at core/misc/drupal.js so as to not break drush and the testbot.

Manuel Garcia’s picture

RobLoach’s picture

+++ b/core/misc/drupal.js
@@ -1,432 +1,2 @@
+// Placeholder file to prevent the root check in drush 7.x-dev from failing.
+// This file has been moved to core/assets/js/drupal.js

Couldn't we make Drush use a different method to find Drupal's root? Why hold ourselves back for Drush to catch up?

+++ b/core/core.libraries.yml
@@ -106,7 +106,7 @@ drupal.autocomplete:
+    assets/js/batch.js: { cache: false }

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.

stefan.r’s picture

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

So, we have a basic problem that there is no official way to detect a Drupal 8 root directory, so we try to do the best we can by checking for "fingerprint" files, especially those that contain the name "drupal". We want to avoid false positives and false negatives to the greatest extent possible. We also want to avoid parsing text files during the bootstrap, if possible.

If anyone could provide of a good, stable and reliable way to detect the Drupal root, then we could improve this code.

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.

stefan.r’s picture

Issue tags: +Needs reroll

Looks like the testbot doesn't depend on Drush anymore, so the drupal.js placeholder can be safely taken out now.

stefan.r’s picture

Issue tags: -Needs reroll
FileSize
31.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,453 pass(es). View

re-roll

Chi’s picture

FileSize
3.1 KB

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

nod_’s picture

stefan.r’s picture

@nod_ what do we want to do with this issue? Have you decided on a final directory structure yet?

droplet’s picture

Version: 8.0.x-dev » 9.x-dev

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

nod_’s picture

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.

SKAUGHT’s picture

I think from here there's two sides of this ticket.

  1. the 3rd party 'vendor' plugins. [i've created a child ticket relative to /libraries -- coming from contrib need for the libraries API in hopes to make the use of 'plugins' libraries to /libraries. To keep apart from /vender folder which is composer/PHP based stuff.]
  2. the drupal behaviours that require the plugin. loaded by an actual core/module.

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

SKAUGHT’s picture

SKAUGHT’s picture

remove +#1440628 -- not related. that issue was poorly titled.

SKAUGHT’s picture

catch’s picture

Version: 9.x-dev » 8.3.x-dev

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.