Just like CSS, we need to move all javascript files to a js subfolder inside modules and update the library information for those modules.

A module that does it well and can be used as reference is the toolbar module.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we're improving standards, not fixing a problem
Issue priority Normal because we moving, not changing, code to be more consistent
Unfrozen changes Not Unfrozen because this is not a bug fix or a change with security, performance, or user experience improvements
Files: 

Comments

nod_’s picture

Issue tags: +JavaScript
anavarre’s picture

Status: Active » Needs review
FileSize
15.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,228 pass(es), 54 fail(s), and 27 exception(s). View

Here's a start.

Status: Needs review » Needs work

The last submitted patch, 2: move-all-js-files.patch, failed testing.

anavarre’s picture

Status: Needs work » Needs review
FileSize
16.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,277 pass(es), 8 fail(s), and 27 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 4: 2484623-4.patch, failed testing.

nod_’s picture

Looks like this one is missing (I know it's a testing module but let's move it too for consistency).
core/modules/system/tests/modules/twig_theme_test/twig_theme_test.js

The last one I can find not in a js subfolder is core/modules/locale/tests/locale_test.js but it's not exactly a module so I say it's fine like that.

Before:

$ find core/modules -name '*.js' | grep -v '/js/' | wc -l
31

After:

$ find core/modules -name '*.js' | grep -v '/js/' | wc -l
2

After the change to twig_theme_test.js and fixes to the tests we're RTBC :)

nod_’s picture

All test failures seems to be related to the locale module js file. The magic in locale_js_alter doesn't have the right path anymore.

anavarre’s picture

Status: Needs work » Needs review
FileSize
18.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,279 pass(es), 7 fail(s), and 26 exception(s). View
1.77 KB

Yeh I didn't really know what to do with twig_theme_test.js but I've added it for consistency now. Trying to fix more failing tests for now. Likely not yet ready.

nod_’s picture

Status: Needs review » Needs work

One more to replace in the local alter hook, a few lines below there is $placeholder_file = 'core/modules/locale/locale.translation.js';

Let's see what the tests say. Should be pretty close.

anavarre’s picture

Status: Needs work » Needs review
FileSize
18.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,284 pass(es), 3 fail(s), and 2 exception(s). View
570 bytes

Oops, thanks. Missed this one.

The last submitted patch, 8: 2484623-8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2484623-10.patch, failed testing.

anavarre’s picture

Working on the failures. I already found one more path to fix.

anavarre’s picture

Status: Needs work » Needs review
FileSize
19.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,286 pass(es), 1 fail(s), and 0 exception(s). View
508 bytes

Status: Needs review » Needs work

The last submitted patch, 14: 2484623-14.patch, failed testing.

anavarre’s picture

Status: Needs work » Needs review
FileSize
20.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
770 bytes

That should be it.

Status: Needs review » Needs work

The last submitted patch, 16: 2484623-16.patch, failed testing.

anavarre’s picture

Not sure I know what's happening with that last fail.

$this->assertTrue(strpos($rendered_js, 'simpletest.js') < strpos($rendered_js, 'core/misc/tableselect.js'), 'Altering JavaScript weight through the alter hook.');
var_dump(strpos($rendered_js, 'simpletest.js')); // returns int(812)
var_dump(strpos($rendered_js, 'core/misc/tableselect.js')); //returns int(634)
die();

So, the integer returned for each strpos() is not what I was expecting. To make the test pass, we'd have to change the comparison operator to >

Tried to dump the strpos() without this patch and here's what I'm getting:

$this->assertTrue(strpos($rendered_js, 'simpletest.js') < strpos($rendered_js, 'core/misc/tableselect.js'), 'Altering JavaScript weight through the alter hook.');
var_dump(strpos($rendered_js, 'simpletest.js')); // returns int(658)
var_dump(strpos($rendered_js, 'core/misc/tableselect.js')); //returns int(724)
die();
anavarre’s picture

Status: Needs work » Needs review
FileSize
20.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,274 pass(es). View
852 bytes

isntall queued 16: 2484623-16.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 19: 2484623-19.patch, failed testing.

isntall queued 19: 2484623-19.patch for re-testing.

The last submitted patch, 16: 2484623-16.patch, failed testing.

rteijeiro’s picture

Cancelled test in #16. We should try to test only latest patch.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
20.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
229 bytes

Just a nit I found with a js file in a test folder. I found that in other modules the js files for tests are also placed in a js folder.

rteijeiro’s picture

FileSize
20.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,275 pass(es). View
969 bytes

Sorry, forgot to add a file :(

The last submitted patch, 25: 2484623-25.patch, failed testing.

anavarre’s picture

In #6, _nod said:

The last one I can find not in a js subfolder is core/modules/locale/tests/locale_test.js but it's not exactly a module so I say it's fine like that.

Thus why it's been left as is. I guess it's fine to have both approaches so that we can pick the one we eventually prefer.

nod_’s picture

FileSize
3.08 KB
21.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-jsfolder-2484623-29.patch. Unable to apply patch. See the log in the details link for more information. View

The change in #18 is not correct, we're making the test useless. What broke the test is we forgot on more path change in simpletest js alter hook.

I'm ok with putting that file in a js subfolder. Doesn't matter much :)

nod_’s picture

Status: Needs review » Reviewed & tested by the community

This is needed for consistency and best practices.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: core-jsfolder-2484623-29.patch, failed testing.

Status: Needs work » Needs review
droplet’s picture

Status: Needs review » Reviewed & tested by the community

+1 RTBC

cilefen’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs beta evaluation

Like all issues hoping to be committed right now, this one needs a beta evaluation. Also, I had to be a nudge but if you mark an issue RTBC it will help the community and the committers if you explain how you came to that conclusion.

tbradbury’s picture

Issue summary: View changes

I took a crack at a beta evaluation and I don't think this change is unfrozen or prioritized so I suggest postponing it. Please share if you disagree.

cilefen’s picture

@tbradbury I agree. This can be done in the next release.

nod_’s picture

I would like it for 8.0.x it's all about setting a good example for contrib and it's just moving files around.

droplet’s picture

Drupal tracking All JS via PHP in CORE. And this is the only and must way to use Drupal libs via Drupal API. We should consider these as private and protected scope members. We're doing internal improvement now.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 29: core-jsfolder-2484623-29.patch, failed testing.

The last submitted patch, 29: core-jsfolder-2484623-29.patch, failed testing.

cilefen’s picture

@nod_ and @droplet According to the beta policy the only way this issue can go in now is if it reduces fragility. The decision on fragility is a core maintainer decision (in other words, your decision) and is on a case-by-case basis. This fragility examples from https://www.drupal.org/core/beta-changes do not seem to apply here:

Additionally, certain changes that reduce fragility are also prioritized. For example: Narrowing an API, removing unused and untested functionality, correcting misspelled or wholly inaccurate method names. Whether or not an issue reduces fragility is at core maintainer discretion and should be discussed on a case-by-case basis.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
20.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,226 pass(es). View

Created reroll.

Patch attached.

tbradbury’s picture

Status: Needs review » Reviewed & tested by the community

8.0.x:

$ find core/modules -name '*.js' | grep -v '/js/' | wc -l
29

After #44:

$ find core/modules -name '*.js' | grep -v '/js/' | wc -l
0

Looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 44: move_all_js_in_modules-2484623-44.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 44: move_all_js_in_modules-2484623-44.patch, failed testing.

Status: Needs work » Needs review
JeroenT’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as per #45.

xjm’s picture

Version: 8.0.x-dev » 9.x-dev
Status: Reviewed & tested by the community » Postponed
Issue tags: -Needs beta evaluation

Thanks @cilefen and @tbradbury. I agree that this does not really pass the beta evaluation. Even if it is just moving files around internally, there is not any specific reason this change is prioritized during the beta. Unfortunately consistency/best practice/DX are not by themselves enough to justify the change. There isn't really a case for it reducing fragility.

I asked @Wim Leers if this would be reasonable to change in 8.1.x, and he pointed out that since other code can alter these libraries, the paths would be wrong and the alters would break. So there is disruption from this change, and it would need to be postponed to 9.x. It also could break an existing site or module's custom JS for one of these libraries now.

So unfortunately I'm postponing this issue to 9.x. Thanks all for your work on the patch nonetheless. I can see why this would be good to clean up eventually.

droplet’s picture

We should reconsider once for this changes. D8 is the release with big changes in structures. Let's do it together.

nod_’s picture

I agree, it's not a big deal to change that during beta, and postponing to D9 means contrib doesn't have a best practice from core to rely on.

nod_’s picture

Version: 9.x-dev » 8.1.x-dev
Status: Postponed » Needs work

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.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
21.03 KB

I agree, rerolling against 8.1.x
Fixed conflicts on:

  • EngineTwigTest.php
  • StatisticsLoggingTest.php
  • LocaleJavascriptTranslationTest.php

Status: Needs review » Needs work

The last submitted patch, 55: move_all_js_in_modules-2484623-55.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
631 bytes
21.64 KB

This should fix the failing test.

dawehner’s picture

Should we add an empty update function to cause a cache rebuild?

Manuel Garcia’s picture

@dawehner: that is probably a good idea.

dawehner’s picture

Cool, afaik we add those doc groups for each updates, so we probably want one for 8.1.x

hass’s picture

@_nod #54: I think you are wrong. See #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides and #1308152: Add stream wrappers to access extension files will not really solve this if you change subfolders. libraries-override can have paths and this will all break if the folder structure is changed. I'm not sure whether these makes this a D9 issue.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

You're right, the path does matter but I think it's a small price to pay for consistency, especially because the files moved are not really the ones I see overriden in contrib or in projects:

book/js/book.js
color/js/color.js
color/js/preview.js
comment/js/comment-entity-form.js
content_translation/js/content_translation.admin.js
field_ui/js/field_ui.js
file/js/file.js
filter/js/filter.admin.js
filter/js/filter.filter_html.admin.js
filter/js/filter.js
language/js/language.admin.js
locale/js/locale.admin.js
locale/js/locale.bulk.js
locale/js/locale.datepicker.js
locale/tests/js/locale_test.js
menu_ui/js/menu_ui.admin.js
menu_ui/js/menu_ui.js
node/js/content_types.js
node/js/node.js
node/js/node.preview.js
path/js/path.js
statistics/js/statistics.js
system/tests/modules/twig_theme_test/js/twig_theme_test.js
taxonomy/js/taxonomy.js
text/js/text.js
user/js/user.js
user/js/user.permissions.js
views/tests/modules/views_test_data/js/views_cache.test.js

We're not touching misc/ making this patch much less disruptive than assumed.

Whatever is decided, this patch is ready to go.

Manuel Garcia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
22.09 KB
378 bytes

Attempting to address comment #60. Not entirely sure on the docblock, I couldn't find documentation about this on the coding standards, so I just tried to follow what was there previously. Have a look.

nandasoftware’s picture

Assigned: Unassigned » nandasoftware
Issue tags: +drupalconasia2016
nandasoftware’s picture

nandasoftware’s picture

Assigned: nandasoftware » Unassigned
lokesh.soni’s picture

Assigned: Unassigned » lokesh.soni
lokesh.soni’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -JavaScript clean-up, -JavaScript, -Novice, -drupalconasia2016 ++drupalconasia2016
Cottser’s picture

Issue tags: -+drupalconasia2016 +drupalconasia2016

This seems a bit problematic for backwards compatibility because of libraries-override and such…

nod_’s picture

Issue tags: +JavaScript clean-up

Even RTBC it's still clean-up :)

@Cottser: It looks sloppy to keep it like that until 9.x whenever that is. We can just add a comment in the release notes and/or a change notice. From what I'm aware, file paths are not part of the 'Public API'.

alexpott’s picture

I think I agree with @Cottser - if someone has done the following in their theme...

libraries-override:
  drupal.book:
    js:
      book.js: false

We'd be breaking this no?

Cottser’s picture

Status: Reviewed & tested by the community » Needs review

Yeah we would be breaking it… I'm a bit torn but overall it feels a bit wrong to potentially break BC for cleanup.

I talked to @catch the other day about this and he mentioned we could maybe do a mapping of old paths to new to preserve the BC. This method could probably also make hook_library_info_alter() done by modules still work which is perhaps a more common use case than from the theme, i.e. replacing a core-provided JS file with a custom one.

Sorry if it seems like overkill. Setting back to needs review for more consideration.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

SKAUGHT’s picture

cross-posting:

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

xjm’s picture

So there are two questions here:

  1. Whether the file location is public API or internal API. We should discuss that in #2550249: [policy, then meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation.
  2. Whether the impact of the change outweighs the disruption, even if we do decide asset locations are only internal API. If it turns out this is internal API, then we should discuss. At the moment it looks like the subsystem maintainers consider this worth the disruption, but the committers are hesitant. So, let's explicitly document the impacts and disruptions in the summary once we decide #1.
xjm’s picture

Status: Needs review » Postponed

Postponing on #75 to make sure we don't fragment the discussion. Thanks everyone.

SKAUGHT’s picture

Related issues:

Change directory structure for JavaScript files this was what i was cross posting from.

catch’s picture

#72 is still my position on this - we should look at how feasible it is to do a backwards compatibility layer for the old paths given it's a fixed number.

We should also document that file locations for assets are @internal - i.e. we let you mess with things via override stuff in .info.yml and alter hooks, but that doesn't mean we promise that the data structures you're modifying don't change, especially given other modules or base themes could be altering the same things anyway.

The bc layer would allow modules to work on both 8.1.x and 8.2.x until 8.2.x is released. We could then even remove that bc layer in 8.3.x or 8.4.x given it's an internal API change since it's going to be an ugly hardcoded mapping at best.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nod_’s picture

Assigned: lokesh.soni » Unassigned
Status: Postponed » Needs work
FileSize
21.65 KB

Updated patch. Need BC layer, here are the files concerned.

core/modules/text/text.js
core/modules/file/file.js
core/modules/color/preview.js
core/modules/color/color.js
core/modules/path/path.js
core/modules/menu_ui/menu_ui.js
core/modules/menu_ui/menu_ui.admin.js
core/modules/content_translation/content_translation.admin.js
core/modules/taxonomy/taxonomy.js
core/modules/comment/comment-entity-form.js
core/modules/locale/locale.datepicker.js
core/modules/locale/locale.admin.js
core/modules/locale/locale.bulk.js
core/modules/locale/tests/locale_test.js
core/modules/filter/filter.filter_html.admin.js
core/modules/filter/filter.js
core/modules/filter/filter.admin.js
core/modules/node/node.js
core/modules/node/content_types.js
core/modules/node/node.preview.js
core/modules/simpletest/simpletest.js
core/modules/book/book.js
core/modules/user/user.permissions.js
core/modules/user/user.js
core/modules/statistics/statistics.js
core/modules/field_ui/field_ui.js
core/modules/views/tests/modules/views_test_data/views_cache.test.js
core/modules/system/tests/modules/twig_theme_test/twig_theme_test.js
core/modules/language/language.admin.js

The new paths:

core/modules/text/js/text.js
core/modules/file/js/file.js
core/modules/color/js/preview.js
core/modules/color/js/color.js
core/modules/path/js/path.js
core/modules/menu_ui/js/menu_ui.js
core/modules/menu_ui/js/menu_ui.admin.js
core/modules/content_translation/js/content_translation.admin.js
core/modules/taxonomy/js/taxonomy.js
core/modules/comment/js/comment-entity-form.js
core/modules/locale/js/locale.datepicker.js
core/modules/locale/js/locale.admin.js
core/modules/locale/js/locale.bulk.js
core/modules/locale/tests/js/locale_test.js
core/modules/filter/js/filter.filter_html.admin.js
core/modules/filter/js/filter.js
core/modules/filter/js/filter.admin.js
core/modules/node/js/node.js
core/modules/node/js/content_types.js
core/modules/node/js/node.preview.js
core/modules/simpletest/js/simpletest.js
core/modules/book/js/book.js
core/modules/user/js/user.permissions.js
core/modules/user/js/user.js
core/modules/statistics/js/statistics.js
core/modules/field_ui/js/field_ui.js
core/modules/views/tests/modules/views_test_data/js/views_cache.test.js
core/modules/system/tests/modules/twig_theme_test/js/twig_theme_test.js
core/modules/language/js/language.admin.js
nod_’s picture

testbot

nod_’s picture

Status: Needs work » Needs review
joelpittet’s picture

We could check if an alter/override is using the old path and hot swap it for the correct override/alter as a BC layer but may want to wait on catch's changes to the aggregation to get in?

#1014086: Race conditions, stampedes and cold cache performance issues with css/js aggregation Since they may conflict if that BC is implemented, thoughts? maybe postpone on that?

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.