Objective
hook_library_info()
is one of the last remaining info hooks in D8.- All manual asset file inclusions via former
drupal_add_js()
anddrupal_add_css()
have been removed in D8 in favor of library definitions that properly declare their dependencies. - Consequently, almost all modules (and also themes) need to declare their libraries in D8. However, that is currently done in PHP and the format is an arbitrary array of declarative data.
- Already starting with its original introduction in D7,
hook_library_info()
was architecturally designed to supply static declarations only; i.e., the declarations must not depend or rely on any request or context information. Since they are currently declared in PHP, that architectural design aspect is not guaranteed.
Proposed solution
-
hook_library_info()
implementations are converted from PHP into$extension.libraries.yml
files. -
Drupal core gets it own
/core/core.libraries.yml
file, which lives right next tocore.services.yml
.→ Removing another dependency on System module.
-
All external libraries that are shipped with Drupal core (the entire Drupal core thing) are centrally declared by
core.libraries.yml
and should be located in/core/assets/vendor
. -
Slightly change the declaration format (1) to make more sense in YAML and (2) to help resolve some architectural asset system problems:
- Every external library declares a new
'remote'
property whose value is the originating repository URL (typically GitHub, but any git remote URL is valid). - For external libraries, the
'version'
property denotes a tagged release (git tag) in the originating repository. - The list of
'dependencies'
is changed from the formerarray('provider', 'library-name')
PHP syntax into a namespaced string:provider/library-name
. - For JavaScript asset files, all former custom
'group'
options (fordrupal_add_js()
) are removed, since all JavaScript should rely on declared dependencies only. - For CSS asset files, the definition structure is slightly changed to be declared in nested SMACSS category names, following our Drupal 8 CSS architecture documentation, and to aid in resolving some hard CSS asset file ordering problems that currently exist.
→ The additional classification info will likely be consumed by #1762204: Introduce Assetic compatibility layer for core's internal handling of assets
- Every external library declares a new
API changes
-
hook_library_info()
is replaced by$extension.libraries.yml
files:Before
// jQuery Form Plugin. $libraries['jquery.form'] = array( 'title' => 'jQuery Form Plugin', 'website' => 'http://malsup.com/jquery/form/', 'version' => '3.39', 'js' => array( 'core/assets/vendor/jquery-form/jquery.form.js' => array(), ), 'dependencies' => array( array('system', 'jquery'), array('system', 'jquery.cookie'), ), ); // Dropbutton. $libraries['drupal.dropbutton'] = array( 'title' => 'Dropbutton', 'website' => 'http://drupal.org/node/1608878', 'version' => '1.0', 'js' => array( 'core/misc/dropbutton/dropbutton.js' => array(), ), 'css' => array( 'core/misc/dropbutton/dropbutton.css' => array(), 'core/misc/dropbutton/dropbutton.theme.css' => array(), ), 'dependencies' => array( array('system', 'jquery'), array('system', 'drupal'), array('system', 'drupalSettings'), array('system', 'jquery.once'), ), );
After
(Example from
system_library_info()
to/core/core.libraries.yml
)jquery.form: remote: https://github.com/malsup/form version: 3.39 js: assets/vendor/jquery-form/jquery.form.js: {} dependencies: - core/jquery - core/jquery.cookie # drupal.dropbutton: version: VERSION js: misc/dropbutton/dropbutton.js: {} css: component: misc/dropbutton/dropbutton.css: {} theme: misc/dropbutton/dropbutton.theme.css: {} dependencies: - core/jquery - core/drupal - core/drupalSettings - core/jquery.once
The path of each asset file is resolved relative to the directory of the registering extension. However, absolute paths (e.g.,
/core/assets/vendor/...
or/libraries/...
) as well as (stream wrapper) URIs (e.g.,http://example.com/foo.js
orpublic://color/bartik/color.css
) as well as protocol-free URIs (e.g.,//api.google.com/jquery.js
) are supported, too.The CSS asset file parent keys are mapping 1:1 to Drupal 8's CSS architecture documentation. Please refer to that documentation to learn more.
Side-benefit for maintenance:
Larger library declaration files (like
core.libraries.yml
) are able to leverage YAML references within the scope of the file, which works this way:jquery.ui: version: &jquery_ui 1.10.2 ... jquery.ui.accordion: version: *jquery_ui ... jquery.ui.autocomplete: version: *jquery_ui
-
hook_library_info_alter()
now acts on the (raw) library definitions, as declared in$extension.libraries.yml
files, and any performed manipulations are persistently cached.
API additions
-
The new
hook_library_alter()
allows modules and themes to act right before a specific library is attached and processed.This allows extensions to add request-specific and context-specific data to a particular library. For example:
- JavaScript settings: E.g., Locale module dynamically adds drupalSettings containing translations for the jquery.ui.datepicker library, which are only needed when the current language is not English.
- Additional asset files that should only be loaded when a particular library is attached. This use-case previously required developers to write complex
hook_js_alter()
implementations.
/** * Implements hook_library_alter(). */ function mymodule_library_alter(array &$library, $extension, $name) { if ($extension == 'core' && $name == 'jquery.ui') { // Add the backwards compatibility layer for jQuery UI. $library['dependencies'][] = array('mymodule', 'jquery.ui.compat'); // Add our additional stylesheet to improve the experience of jQuery UI. $path = drupal_get_path('module', 'mymodule'); $library['css'][$path . '/css/mymodule.jquery-ui.css'] = array('weight' => CSS_SKIN); } }
Note:
As you can see,
hook_library_alter()
uses the previously existing/internal format ofhook_library_info()
/drupal_add_library()
. The internal data structure of$library
might be revised prior to Drupal 8.0 — in particular, to retain the nested SMACSS category keys for CSS file assets, so as to avoid having to specify custom'group'
and'weight'
options. (Changing that is out of scope for this issue.)
Not addressed here → follow-up issues
-
Convert all
#attached]['library'][] = array(provider, name)
into#attached]['library'][] = 'provider/name'
throughout core.Reasoning: This patch is large enough already + very hard to re-roll/maintain.
-
Convert
drupal_get_library()
into a service and:- Inject all dependencies.
- Consider to use a single cache item for all extensions.
- Resolve the @todo of determining whether $extension is a module or theme.
- Resolve the @todo of determining the version of contributed and custom modules/themes.
- Introduce proper unit tests for
drupal_get_library()
(there are none yet).
Reasoning: Aside from the retrieval + parsing code,
drupal_get_library()
+ consuming code is not affected or changed by this patch. -
Ensure that libraries and library dependencies are only processed and resolved once prior to rendering out an HTML page, so that the order of dependencies of all attached libraries is correct and all remaining
'weight'
options can be dropped from all declarations.Reasoning:
drupal_add_library()
anddrupal_render()
is not touched at all by this patch; i.e., the problem exists in HEAD already.→ This primarily means to help with #1762204: Introduce Assetic compatibility layer for core's internal handling of assets
.
.
Original report by @nod_
The recent trend has lead to the removal of almost all info hooks. There is a strong possibility that hook_library_info() would be the only info hook left in D8.
I propose that we move everything in hook_library_info() in a MYMODULE.library.yml or MYMODULE.assets.yml file.
It'll make it easier on themer to declare their script and CSS dependencies #1969916-5: Remove drupal_add_js/css from seven.theme and it's more likely they'll do it instead of some ugly PHP to implement in some file.
We'd keep library_info_alter() because that's very much needed. yml file would work because the information in the library hook isn't dynamic.
Comment | File | Size | Author |
---|---|---|---|
#201 | lib.css_.png | 174.05 KB | sun |
#193 | library.info.190.patch | 192.94 KB | sun |
#166 | 1996238-library-info-166.patch | 192.81 KB | longwave |
Comments
Comment #1
PolFYI: I'm using dynamic informations in hook_library_info() for OpenLayers 7.x-3.x, I haven't find anything better yet.
Comment #2
nod_On the dynamic nature of library_info things, I got scolded by sun about it #1737148-89: Explicitly declare all JS dependencies, don't use drupal_add_js which is fair.
About the openlayer case It's convenient to do it like that yet it's still a static list of files. Declaring them one by one would be boring but isn't impossible. I guess you'd have a script generating the data and dumping everything in a yml file.
Comment #3
Polor use hook_library_info_alter() ?
Comment #4
nod_that too, yes. Doesn't feel too right though :p
Comment #5
jessebeach CreditAttribution: jessebeach commentedA module like Editor passes data with the hook_library_info declaration as well.
The idea is that one could declare libraries with the hook or the yml file, correct?
Comment #6
Dave ReidIt's a very common use case to be able to specify the path to my module in the path of the assets. I wonder if we need to require #1308152: Add stream wrappers to access extension files so that we can declare the paths to the assets using module://edit/js/editor.formattedTextEditor.js.
Comment #7
nod_Very good point.
@Jesse: I don't see any issue with declaring data. As long as it's not dynamic data. I want only one way of declaring that not both. That'd suck to explain during training.
Comment #8
jcisio CreditAttribution: jcisio commented#6 #7 Would the path not be relative path? Each module/theme declares its own assets and dependencies. There is no need to use those system stream wrappers.
Comment #9
nod_at least for the system module that's not true. But yeah, for contrib I'd agree with that.
Comment #10
pounardI would tend to think that those stream wrappers could be useful, but not required.
Comment #11
nod_Ok so here's why I sort of have.
system.library.yml
The current structure is really ugly when dumped in yml. So I made a few tweaks.
array('system', 'drupal')
and I turned them intosystem/drupal
. It makes sense for several reasons: It's exactly what we'd do if we were using AMD (see #1542344: Use AMD for JS architecture for context) because you can say thatsystem/
maps to this URL and the AMD loader will load the file from the right place by itself. The final data array is not a 2 dimension array with[$module][$key]
but a 1 dim with[$module / $key]
'path/file/name.js' => array(/* options */)
that makes for an ugly export in yml. So I turned it intoarray('data' => 'path/file/name.js', /* options */)
Because that's what is done to it down the line anyway.Comment #12
jessebeach CreditAttribution: jessebeach commentedRegarding my concern in #5, I went through the hook_library_info invocations in core now and there are no instances of dynamic data. If one were to need dynamic data on the front end, the best approach would be to set up an AJAX request for the information.
Comment #13
nod_Ok that's a dirty patch but it works, files load and all. Didn't put the _alter hook (just forgot, it's easy to add back).
The file includes/test.php is what I used to generate the yml files from the library_info hooks. not all modules are updated.
Wasn't as bad as I thought :)
Comment #15
tim.plunkettThis works for core modules, since they are always in /core/modules.
How does this work for contrib, where they could be in /modules, /sites/*/modules, /profiles/*/modules....
Comment #16
nod_pretty east to add a drupal_set_path in the drupal_get_library call. just need to put relative file path even for core modules.
Comment #17
nod_like that.
I think I got all hook_library_info() converted with this. The result definitely needs to be cached somewhere my patch is probably dead slow.
Comment #19
nod_maybe this one?
Comment #21
nod_That's where I'm at now, not too happy about some code in drupal_get_library() but it works for now.
The
type
key is gone from the yml files, it makes more sense to usefile
,external
,settings
,misc
(yeah that's a new one, otherwise it makes solving #15 kind of a pain, see @todo inline) as keys for people to use. It's converted back todata
/type
indrupal_get_library()
.Comment #22
nod_With the patch that could help…
One nice thing is that it removes 1k lines of PHP from system.module :)
Comment #24
nod_Now with less test failures.
Comment #25
nod_haha didn't expect green, I'm not complaining though :)
( edit ) the patch is NW really. The library info needs to be cached somewhere, I'm pretty sure the current patch puts a real bad perf hit. I'd like to have opinions though.
Comment #26
Wim LeersOverall, I don't see problems with this approach. But the "why?" question isn't adequately/clearly answered by the issue summary yet.
I don't see test coverage yet?
We'll have to update this version string every single time Drupal core is updated for every single core module with libraries. That's… a PITA.
What about
version: core
orversion: CORE_VERSION
, i.e. a magical value?Not quite right :)
Comment #27
tim.plunkettHere is why. Added to the issue summary.
Comment #28
nod_@Wim, good catch: #2002622: Wrong declaration for tabbingmanager script in system_library_info()
Also, Angie raised a concern about themers needing to implement hook_library_info() for their scripts (which they'll need to do) and I'm sure mfer would agree with her. YML file feels like it'd be easier to write for them.
And Yeah, the version think kinda suck. Not sure how to deal with that, get the version from the info file if it's not specified maybe?
Comment #29
Wim Leers#27: Thanks! Makes sense :)
#28: In module
modulename.info.yml
files, we apparently still haveVERSION
. Can't we make that work here too?Comment #30
nod_Was in the middle or rerolling it :)
Here it is, with VERSION.
Caching needs to be added, closed #1787758: drupal_get_library() / hook_library_info() is not cached to get it done here. I'm not sure where to put the code for that so help is appreciated.
Not posting interdiff, only differences are some updated libraries dependencies declarations that happened in
system_library_info()
, the 3 lines for supporting VERSION and I removedhook_library_info
fromsystem.api.php
(not sure where the doc should go though).Comment #31
cweagansInstead of calling the Yaml parser directly, you could just call drupal_parse_info_file(), which is already statically cached and will substitute the value for the VERSION constant.
Comment #32
nod_reroll for #2002622: Wrong declaration for tabbingmanager script in system_library_info().
Caching needed still, help welcome.
Comment #33
nod_with the new files…
Comment #34
Dave ReidSome blockers on this approach considering contrib would need to use this YML approach as well:
Comment #35
nod_Comment #36
tstoecklerRe 1: Module shouldn't be declaring external libraries in library.yml just like they currently shouldn't be declaring external libraries in hook_library_info(). That problem-space is (at least in theory) solved by Libraries API. hook_library_info() has always been specifically for libraries that are shipped with the module, and thus assuming that the files are in the module directory is in fact the only possible assumption.
Comment #37
pounardI do not agree with #36, all libraries and dependencies, external or internal, should be declarer via the same registry, in order to ensure the core is able to include everything, and possibly in a certain order.
Comment #38
tstoecklerRe #38: In theory I agree 100% with you. hook_libraries_info(), however relies on the fact that a specific version of a library is available. That can only guaranteed if the library is shipped with the rest of the code. For libraries that must be obtained separately there is no way to guarantee a single version. Therefore hook_libraries_info() resp. *.library.yml is not suitable for external libraries. It might that Libraries API ends up implementing something like that in order to facilitate proper ordering and dependencies, but that's a whole different issue. That's just how it is currently, I'm not saying it couldn't be any better.
I was just trying to argue that
in #34 is not correct.
Comment #39
jcisio CreditAttribution: jcisio commentedI agree with both lol. All libraries should be declared via the same registry. However, external libraries that module don't know the path yet, can be still declare in .yml then use hook_library_info_alter() to change the path.
Comment #40
jcisio CreditAttribution: jcisio commentedDidn't see #38. "Shared libraries" can be declared via hook_library_info_alter() too.
Comment #41
tstoecklerLet's please leave external libraries out of this. There's a reason Libraries API is a non-trivial project. That's why hook_libraries_info() evaded that entire problem-space from the start. We would all like to have (a complete/modern/awesome version of) Libraries API in core, but that's simply not the case. So, yes we *can and should* make the assumption that libraries provided by modules in *.library.yml are within their filepath.
Comment #42
pounardI don't think it makes the problem being more complex than it already is. It makes nonsense IMHO to separate external from internal libraries, external should just be a state within the declaration that only change the final aggregation behavior, and nothing else (dependency managing and such would be the exact same). It does not change the way we'd deal with internal libraries and their filepaths.
EDIT: Moreover:
1. Nothing prevents a module (for example jQuery Update) altering the info, and changing an internal library by an external one.
2. I don't think that Libraries API is a good example, I personally avoid using it whenever I can.
Comment #43
tstoecklerSorry for being imprecise, "external" in this case can mean multiple things. I was not talking about externally referenced files, as in the 'external' option of drupal_add_js(). I was talking about libraries that are not shipped with modules.
Either way, the last sentence of #41 stands. Can we get back on topic regarding the actual patch now?
Comment #44
pwolanin CreditAttribution: pwolanin commentedThis seems inconsistent compared to the pattern of converting info hooks to plugins, but sure that's a good fit here - why not just leave it as an info hook?
Comment #45
tim.plunkettFixing tags
Comment #46
nod_@pwolanin: it's much nicer to write yml files than php arrays. Themers will need to write them too.
And to address #34 1. as msonnabaum said IRL, we could reference callbacks in the yml file that will generate the libraries definitions similar to what we do for routing.
Comment #46.0
nod_added reasoning
Comment #47
sunIt appears that hook_library_info() is one of the biggest offenders in the remaining list of info hooks in D8:
https://api.drupal.org/api/drupal/8/search/_info
Info hooks should not have a place in D8 anymore.
In addition, the proposed change here enforces an important aspect of the architectural design of hook_library_info():
The library information is declarative.
This aspect is not clear to everyone. In fact, in its very early stages, the (Quick)Edit or Editor module in D8 core registered libraries in a dynamic way, depending on request-specific information, which was incorrect.
By moving hook_library_info() from PHP code into a YAML file,
As a result, this issue should be moved back to D8 and resolved as proposed. Thanks for working on this, @nod_.
Comment #48
andypostSO let's get back to 8.x because a lot of issues could be seriously improved btw
Mostly this will help with #2073823: [META] Remove drupal_add_js() calls
Comment #49
nod_Reroll, be careful it's "yeah whatever I just want it to work" quality.
I included the script to generate the libraries in /lib.php, feel free to use it, should be removed for final patch obviously.
Fair warning, that's not the kind of core work I have fun doing. I have no idea at which level to implement caching and i'm not interested in figuring it out. I really need someone to take care of the rest of those things.
Example:
Comment #50
Dave ReidBasically every module in contrib that uses sites/[all|default]/libraries?
Comment #51
nod_No they don't declare them, they use them as dependencies. It's the libraries module that should declare them. See #41 and #43, who am I to argue against that :p
Comment #53
sunWhat I did not touch (yet):
Due to the massive amount of file changes + created files, now maintained as a proper branch in the Platform sandbox:
http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/librar...
@nod_: Granted you write access to Platform; just in case you want to pick this up again. :)
Comment #54
sunSorry, belated interdiff
Comment #55
sunI missed this one; should be moved into the core library declaration.
Comment #57
sunComment #59
jibran80 chars.
Comment #60
sunAwesomeness warning:
Have a look at the new
core.library.yml
. :)Comment #61
sunCritical spin-off: #2161217: jQuery Cookie library is no longer included in jQuery UI
Comment #62
nod_So much win.
This lib is not in core anymore (gone with the overlay). Other than that, the other changes looks great to me.
Comment #63
Wim LeersSo much win indeed! Thank you, sun!
I agree with this fix, but IIRC core committers felt it was more appropriate that the library as in fact owned by CKEditor module. So let's not change that in this patch.
Important comments, such as this one, should be retained though:
Comment #64
sunA range of clean-ups, also incorporating above comments, before moving forward on the actual library.yml syntax (UX) + processing:
Happy to spin off the CKEditor + Editor module fixes into a separate issue, if necessary. They are required for this patch, and it's long overdue that the originally intended architectural design of static declarations is finally enforced. (cf. #47)
Yeah, focus on "was". :)
Back then,
core.library.yml
did not exist, so System module registering CKEditor module's library made little to no sense.By introducing the capability for the Drupal core base system itself to register libraries, the new world order is this:
/core/assets/vendor
.core.library.yml
.The first and foremost reason for that is simple:
A contributed or custom module that would like to use the existing bundled library but does not like the usage/implementation of the core module, would (1) either have to needlessly depend on the core module to get the library registered or (2) re-register the library from scratch on its own (which breaks dependencies of other modules, since the library owner/provider would no longer be "core").
Furthermore, re-registering a core library in a contrib/custom module would be close to impossible to manage with regard to the declared library version. Drupal core might ship with an updated version, but that will not be reflected in the contrib module's declaration unless it is updated accordingly.
In short, this is required for e.g. Wysiwyg module to use the CKEditor library bundled with core, without having to depend on the ckeditor.module in core. Same for other libraries/modules/use-cases.
Due to that, I did not revert the registration of the ckeditor library. Instead, I added two @todos for picturefill.js (Picture) and jquery.joyride (Tour) to move those external libraries into core.
Next steps, moving forward:
The common declaration for the vast majority of files has no options, like this:
Ugly DX. → Researching and studying the YAML specification once more in-depth:
It is possible to declare hashmap keys without values, as long as the key is terminated, like this:
The parsed value is NULL, which can be easily caught and translated into an empty array in the processing function.
Although the trailing colon is slightly weird, I believe that this would be preferable in terms of DX.
Thoughts/Opinions?
To advance on (1), definitions for external files would become this:
The "external" option could be automated even via
url_is_external()
, but that's a detail.That said, I'm not sure whether externally hosted files really mesh so well with the intended architecture of libraries that are shipped/bundled with your code to begin with, because the library is de-facto not shipped with your code. To me, that rather sounds like a job for the contributed Libraries API, CDN, and other modules.
But anyway, unless we remove support for externally hosted files, their definition would change to the above.
The
group: 0
options do not make any sense.The values were using PHP constants previously; i.e., JS_LIBRARY, JS_DEFAULT, JS_THEME, etc.
I can see two potential remedies:
That way:
That is, because it scares me that we apparently allow a single library to be multiple things at the same time in the first place.
Normally, a single asset either is a standalone plugin of its own, OR it supplies integration code, OR it's a theme-related overload → but please not something in between.
This approach would eliminate all of the remaining
group: 0
options of module integration assets. Instead of adjusting the group of individual files, the entire library would be placed into a dedicated group.In re-establishing the comments as requested in #63, I noticed that some library interdependencies are very fragile:
weight: -1
adjustments, without a single word on why those weight adjustments are necessary.Even after spending so much time on all existing library definitions throughout core, I still don't know (1) why those adjustments exist and (2) what the final loading/execution order is. :(
In particular, I'm not able to see how this system can reasonably scale in a modular/extensible application framework like Drupal. I cannot imagine how many contrib/custom modules need to futz with weights of -0.04...
If you'd ask me, then we should drop all weights (and support for weights) from library definitions.
Instead, every library should only have two facilities available: Dependencies ("run after these") and Dependants ("run before these").
That's most likely out of scope for this issue here, but I do want to raise the concern of how fragile our current library definitions are.
Comment #66
sunLet's ignore the test failures and discuss and focus on the syntax/architecture instead. :)
Comment #67
nod_Few things:
Weights should go away with #1762204: Introduce Assetic compatibility layer for core's internal handling of assets. So it is a problem and it is being addressed, feedback welcome :)
I don't like the trailing ':' in the proposed change. People working with that stuff are familiar with JSON, and that's just what it looks like in the current patch (and an extra - that makes it look like a list). So to me it's not really a DX improvement.
On the group topic, it's pretty much aggregation groups, and we'd like to let people define their own aggregation groups (to optimize to their use case, have the CKEditor group that you can aggregate with the Edit group or whatever). I agree with giving them a proper name but not declaring the libs under one specific group like that. I guess aggregation groups should really be aggregation 'tags' with some aggregation rules. Essentially that'd make groups kinda useless.
Comment #68
Wim Leers#64.1: "Witness: All JS settings are dynamic. → Settings do not have a place in declarative definitions?"
I think you're both right and wrong :)
*Dynamic* settings don't have a place in declarative definitions. So I agree that what
editor.module
andckeditor.module
do is intolerable. However, static settings are not problematic. One could argue that if the settings are static, then what is the point of having them at all; why not hardcode them in the JS? Because it might be easier to override them that way. But even then, contrib modules could just optionally provide such a setting, that doesn't mean the library must use them; core could just "hardcode" the defaults and just check if a certain setting is defined, and if so, use that instead.So: dynamic settings are intolerable in library definitions, static settings are tolerable, but their usefulness is debatable.
In any case, I've rolled patches to right the wrongs you've found: #2161759: Remove unused drupalSettings.ckeditor.modulePath setting and #2161763: Remove unnecessary drupalSettings.editor.getUntransformedTextURL setting :) Please review.
So, I completely agree with
Thanks for explaining your "external libs belong in
core.libraries.yml
" rationale. It's very reasonable :) +1js/foo.js: {}
. That clearly says "no custom options for this one, please".Funny you say that. That's precisely what sdboyer has been working towards with #1762204: Introduce Assetic compatibility layer for core's internal handling of assets :) You should talk to him! I've been a sounding board for him for that issue, and I think nod_ has been as well, as has been catch. So, I think we're all on the same page: our asset dependency system is very convoluted, fragile (extremely!) and frustrating.
It's late in the cycle, but sdboyer has buy-in from catch, and he should be able to land that patch without API changes and just getting rid of weights.
We already have the "this must run after that other library" relationship implicitly (i.e. define dependencies), but we don't yet have the "this must run before that other library" relationship. That's what the "weight: -1" stuff is for, usually. That might require an API addition.
(Whenever I introduced such a case (once or twice I think), I've made sure to document it. Sadly, that hasn't always happened, and in those cases it's not understandable.)
IIRC, this patch would help #1762204. If we (sun, sdboyer, nod_ and I) could put our combined shoulders under this and #1762204, I think we stand a good chance of landing both and massively improving the front-end DX :)
Comment #69
damiankloip CreditAttribution: damiankloip commentedGreat work sun, this is looking good. Here is a reroll to keep things moving. Attached an interdiff as a few updates were needed.
Comment #71
damiankloip CreditAttribution: damiankloip commentedWhoops, sorry.
Comment #73
sunFixed merge conflicts.
Thanks for the re-roll attempt :) Note that due to the massive amount of changes, this patch is maintained in the library-info-1996238-sun branch of the Platform sandbox.
Comment #74
damiankloip CreditAttribution: damiankloip commentedMeh, sorry. Using the sandbox would have been easier for sure.
Comment #76
klonos#2130059: Issue status change not consistently saved in node revisions
Comment #77
sunFixed hook_editor_js_settings_alter().
With that, we should be back to "square two"... Given how painful this re-roll was after just a short period of time, I wonder whether we shouldn't move the Service-ification & Co into a separate follow-up issue, and instead, just try to add some minimal database caching here as a quick & dirty temporary measure?
Comment #78
klonos;)
Comment #79
sunAdded naive persistent database cache for library definitions.
Comment #80
jibranWarning: #2162409: Split up contextual.js and #2162837: Split up contextual.toolbar.js will soon cause more conflicts.
Comment #81
sunRemaining issues I'd like to tackle & try here:
See attached patch. I think these final improvements make a lot of sense.
Clean-ups I'd like to move into one or more follow-up issues:
Unless there are any major review issues or objections, I'd consider this patch as complete and would recommend to move forward with a commit here.
Comment #83
damiankloip CreditAttribution: damiankloip commentedIs there a reason (it was doing this before too) why we invoke library info on a per extension basis, and not just cache all library info as one cache item?
we can probably change this whenever, but I think we could improve this tag, also, the format probably needs to be changed to be inline with other tags.
You should use the Drupal\Core\Cache\Cache::invalidateTags() method here instead. As that will currently only invalidate tags in the default bin. If people switch cache backends, we will need that change.
Hopefully we can fix the whole cache tags/bins separation before release though :/
Comment #84
sunAs this is a pain to merge and re-roll, I hope I'm allowed to tag this issue.
That's taken over from the existing code. I'm not opposed to changing that, but to do so, we'd have to change drupal_get_library() to scan + discover + process all available (enabled) extensions in the system, which I'd like to defer to a follow-up issue (+ service-ification).
The follow-up issue will allow for nice cleanups. I just didn't want to creep the scope further here, as the patch is large enough already + kinda hard to re-roll in case of conflicts...
Comment #86
sunUpdated locale_library_info_alter() for new library definition structure.
...but wait. On a second thought... the manipulations of that hook implementation depend on the current page/request context; i.e., they cannot be persistently cached.
Hm. I think this gets us back to a major shortcoming of our current library system: Modules (or themes) that want to dynamically "attach" something to a library, but only when the library is used, do not have a hook, event, or proper integration point right now — except of introspecting all contents of hook_js_alter() on all pages + conditionally acting there. :-/
The common abstracted use-case is:
In essence, a new
hook_library_alter()
(sans _info), which is invoked bydrupal_add_library()
like this:I really hoped that we could limit the scope of this issue to the changes in the current patch, but I fear we have to resolve this additional problem space in order to not break such existing use-cases.
The only alternative to the above would be to execute hook_library_info_alter() on the cached + preprocessed library definition on every page request. However, I do not like that idea, because it distorts a clean separation of statically cached declarations vs. context-specific operations. I also dislike that manipulations of static data would repetitively happen on every single request instead of being cached in the static/declarative info already (e.g., jquery_update module would swap out the library on every single request, instead of doing it once and done).
Thoughts?
I'm not able to reproduce the Views test failures locally, so I assume that was testbot fluke.
Comment #87
moshe weitzman CreditAttribution: moshe weitzman commentedI personally don't mind this pattern at all. I think it can perform well, and serve the purpose nicely. We added a variant of this pattern recently to the render system with #post_render_cache which is where callbacks can replace tokens in cached data or tale other dynamic actions.
Comment #88
sunAdded hook_library_alter() to perform request-specific adjustments for an attached library.
I'd like to clarify that this does not only resolve the concrete problem space here — I encountered this limitation in the past already; IIRC, for Libraries API and similar modules, having the shared use-case of needing to act when library X is attached, and which currently have to use very complex hook_js_alter() implementations to achieve the desired operation. In other words, this resolves multiple issues at once.
Comment #92
sun88: library.info.88.patch queued for re-testing.
Comment #93
Wim LeersThis is also something sdboyer, nod_ and I have discussed several times now. The piggybacking of "JS settings" into the "JS asset type" causes many problems, both technical and DX. So: yes, absolutely, we need to stop mixing those! That's of course out of scope for this issue, but I just wanted to confirm that you're not the only one thinking that :)
Patch feedback:
$extension
to refer to both themes and modules?Amazing progress here, thanks sun!
Comment #94
sunHm. — The
'external' => TRUE
definition is simply converted into the existing'type' => 'external'
option, like the old code did. The code in drupal_get_library() is barely testable in its current form, and in fact, this patch does not have to significant test changes, which means that there is no proper test coverage for hook_library_info() yet.If possible, I'd like to defer the addition of proper test coverage to the follow-up issue that will hopefully convert drupal_get_library() into a classed service (as well as possibly using a single cache item for all extensions as mentioned in #84), which in turn should allow us to properly test its class methods.
Thoughts?
Comment #95
Wim LeersEditorPluginInterface::getGlobalJSSettings()
, then it'd need test coverage. Plus, because it's *global* settings, it should not receive anEditor
entity.However, I honestly don't see the value of having that method. There's not a single use case I can think of that was impossible before or that this simplifies significantly (in fact, the only use case in core for it no longer applies). All it does in my opinion, is add complexity, because the result is that each text editor can return JS settings from two methods, and it's unclear how they are related:
getGlobalJSSettings()
returns a "top level" JS settings array, getJSSettings" returns adrupalSettings.editor.formats.<text format ID>.editorSettings
-level JS settings array.I strongly disagree that this is an improvement.
JavaScriptTest
, but it's "incomplete" to put it nicely :P. Therefor I think it's okay to defer adding test coverage for this, especially considering your further plans. Also: chances of regressions are low and impact on the rest of core development would be low too.Comment #96
Wim LeersCurrently I'm working on other core patches while this patch is applied, so that I can test it thoroughly. It looks like the resulting order of assets in the HTML is different, though AFAICT not in a breaking way. Ideally, it'd be identical though.
So far, it's working beautifully!
… except in case: the
edit_stylesheets
functionality — the changes toedit_library_info_alter()
and_edit_theme_css()
broke the code. Here's a patch that fixes that, which you can apply to your sandbox.Comment #97
webchickDoes not apply.
Comment #98
sunSorry for the silence. (was extremely busy elsewhere…)
Come to think of it, (3) is the one and only valid and solid answer to the topic of "groups":
There are no groups. Define your dependencies instead, kthxbye. :)
Regarding (4) global editor JS settings, I'm actually very sure that this facility is required and should exist. It's particularly useful for editors/plugins that require a massive amount of JS settings/metadata to operate — which would have to be duplicated in drupalSettings for every single text format otherwise. To resolve that, we invented a global → editor → format settings merge/override concept in Wysiwyg — primary purpose being to reduce page size bloat caused by d(r)uplicated drupalSettings. We might not have the use-case in core, but I'm confident that editors/plugins supplied by contrib will run into the problem space.
Given this update - in case it passes, attached patch might actually be in a commit-ready state.
Of course, we'll have to review + confirm that all libraries are properly defining all of their dependencies. However, I think the only realistic way to do that is to commit this patch and afterwards see whether any libraries are loaded too late or too early.
Upfront, I can already tell that the dependency resolution logic of attached libraries does not appear to be correct — for example, the JS part of the seven/install-page library being used in the installer does (or did) not declare any dependencies (which is correct, because it's raw JavaScript), but yet, it is loaded in the middle of nowhere. Thus, the processing and dependency resolution of all attached libraries needs some love. However, that code is not touched at all here, so that is a separate issue. That said, @sdboyer's Assetic patch might eliminate and resolve that entirely (didn't have time to look at that yet).
Comment #100
klonos#2169435: Fix pift_cron_autofollowup_logic
Comment #101
sunFixed core/drupal.vertical-tabs[.js} must load before core/drupal.collapse[.js].
Comment #102
nod_I'm not a fan of the new syntax. I'm pretty sure people will forget the
: {}
— every single dev will have to write a lib file at some point — and wonder why things are breaking. When it breaks the error message is not helpful:Anyway, I can live with it and it's just a detail but I wanted to voice my disagreement.
Something missing but that could be a follow-up is the dependency declaration and library attach difference:
It'd sucks to do in an already painful patch but I want to make sure we agree that's the end goal.
Views UI is pretty busted though :( in
core/drupal.ajax
the weight needs to be removed, that helps.And the rest of the styling issues comes from the fact that the CSS is pretty much in the reverse order of what it should be. This is going to be a tricky one.
Other than that, edit, wysiwyg, toolbar, contextual links, autocomplete & co works fine.
Comment #103
sun(2) globally fixes the order in which CSS files are being loaded.
It does not fix the fact that
drupal_add_library()
directly processes + attaches a requested library and its dependencies whenever it is invoked - which means that it is only aware of the immediate libraries that it happens to deal with for a particular invocation, instead of properly accounting for all interdependencies between all libraries (once) when an HTML page rendered. That topic is left for #1762204: Introduce Assetic compatibility layer for core's internal handling of assetsHowever, by introducing SMACSS categories into our library definitions, we're able to mitigate a large part of that problem space, because (1) we can apply the correct weights and (2) we still have a limited amount of dependency information to work with.
For the SMACSS category names, I followed the naming of our existing CSS_* constants (in common.inc), which seem to be in line with our formalized D8 CSS architecture documentation. — Apparently, you will notice that this documentation page contains some code blocks that show the exact same structure → Profit! :)
For the SMACSS category classification of individual CSS files though, absolutely none of our existing documentation helped me to make an informed decision on how to classify them. :( — For most CSS files, I used "component". And for all files with ".theme.css", ".admin.css", and ".maintenance.css", I used "theme". I also used "state" one or two times, even though that category totally wasn't clear to me.
Based on manual testing (and some quick introspection of CSS style overrides via Firebug), this appears to fix all of the ordering bugs (including Views). That said, Views UI and dropbuttons are both looking a bit odd in Bartik, but then again, I'm not sure whether that isn't expected; our entire admin UI seems to be styled in Seven only... :-/
Comment #105
effulgentsia CreditAttribution: effulgentsia commentedbot fail, so needs work.
Comment #106
sunMerge branch '8.x' into platform/library-info-1996238-sun. Simple diff context conflict, so no interdiff.
Comment #107
nod_Views UI works fine. The rest still works properly as well. That's a RTBC to me :D
Comment #108
sunComment #109
sunI believe this is finally ready to be committed now.
The 'group' issue has been resolved in a much cleaner way.
The only remaining issue is to convert all manual uses of
#attached['library'] = array('provider', 'name')
into#attached['library'] = 'provider/name'
, but @nod_ and I discussed that it should be fine to do that in a separate follow-up issue.Comment #110
webchickPlease update the issue summary so when this gets RTBC a core maintainer can have an easier time getting up to speed on 100+ comments. :)
I also note this is a huge API change, though I don't see where it was explicitly run by a core maintainer for approval, so not giving it the "Approved API change" tag just yet.
Comment #111
sunAdded a proper issue summary, which is hopefully complete. Reviews/edits welcome. :)
Super-minor: In case I have to re-roll this monster once more, it would be good to add a comment to the top of core.library.yml to clearly state that all libraries are declared in alphabetical order (which is something that seriously drives me nuts in our current service.yml + routing.yml files...)
Comment #112
nod_Since it was mentioned a while back diffstat might be an argument for a change :)
106 files changed, 1792 insertions(+), 2643 deletions(-)
Comment #113
nod_Everything still working fine for me and Issue summary up to date.
Gotta love the new DX :D
Comment #114
jessebeach CreditAttribution: jessebeach commentedI think this is a real win for DX. No need to edit PHP to introduce assets and dependency chains. Super work!
Comment #115
damiankloip CreditAttribution: damiankloip commentedI agree this patch is looking great now.
The only thing is that I do not think the cache tags are sufficient. I think the extension tag is a really good idea, and I would like to work on introducing that in more places. I think we still need the 'library_info' tag though. As extension (and extension:name) is pretty generic.
We have already talked about making a single cache entry for this data etc.. in previous comments here, so we could tackle this in that follow up?
Comment #116
longwaveShould we have consistency/standards for singular vs plural nouns? Just wondering why this is the singular *.library.yml and not *.libraries.yml, when we have plural *.services.yml, *.local_actions.yml and *.local_tasks.yml.
Comment #117
aspilicious CreditAttribution: aspilicious commented#116 has a good point
Comment #118
tstoecklerhmm... we also have routing.yml instead of route.yml or routes.yml which totally falls out of line. With module name we fairly consistently stick to the singular (i.e. filter.module instead of filters.module) so if this were the first yml file I'd say it would be more consistent to go with the singular, but the examples in #116 are a pretty strong precedent for the plural.
One additional note: In D8 contrib Libraries API is likely to have *.libraries.yml files, so if core were to go with libraries.yml as well we might have to think about a rename of the module.
Comment #119
sunThanks for the additional feedback! Happy to quickly adjust that here prior to commit:
Comment #120
klonos...also title update then.
Comment #121
Wim LeersHehe — glad to see I'm not alone :)
Explicit failure… THANK YOU!
Ridiculous nitpick: AFAIK we always use
\Drupal
, neverDrupal
. But that's such a ridiculous nitpick that it shouldn't need to be rerolled. This is not the only place where this exists.The only somewhat important thing that I'm missing, is test coverage for a sample YML file that contains all possible kinds of SMACSS categories.
Like Jesse said: this is a huge DX improvement.
It doesn't matter if this breaks some things in Drupal core — as sun already said: that's the only way to find out, and it's probably the case because the previous library definition already was incomplete/incorrect. It cannot break core in a major way (tests are green and quite extensive manual testing was done). It might break some things in core in a minor way. But that's definitely worth any follow-ups, in my opinion. Between sun, nod_, I and others, we should be able to fix those quickly.
I think this is RTBC, but I want to make sure that sun saw tstoeckler's rationale for having
*.library.yml
rather than*.libraries.yml
, which is what's in the latest patch. I'm fine with either.Comment #122
nod_Comment #123
star-szrReplacing library.yml with libraries.yml everywhere but the original report in the issue summary.
Comment #124
longwaveI think routing.yml is inherited from Symfony, whereas services, local_tasks and local_actions are Drupalisms.
Comment #125
sunre: @Wim Leers: SMACSS category test coverage:
Yes, neither the old code nor the new code validates the structure, syntax, and format of library definitions. As mentioned in the issue summary, the plan is to introduce test coverage for this code in the follow-up issue that turns it into a service.
That is, because as soon as we have a separate
LibraryHandler::processDefinition()
method, we're able to cleanly unit test that function without any files involved.Also, I personally expect that the objectified parsing code will validate all data structure and data type requirements of each processed definition and that it will throw many more exceptions.
re: libraries.yml vs. Libraries API:
I thought through this before making the change. (1) libraries.yml definitely makes more sense, because the file declares multiple libraries and not just one. (2) The plural form is consistent with other declaration files. (3) I'm skeptical whether it will conflict with Libraries API in the first place, because the libraries.yml files introduced here are located in each Drupal extension, whereas the potential declaration files of Libraries API would live in the /libraries directory, since the whole point of that Libraries API idea/concept is to detach the declaration of external libraries from any particular extension.
In any case, I'm confident that Libraries API will be able to cope with it.
Comment #126
sun119: library.info.119.patch queued for re-testing.
Comment #128
sunComment #129
Wim LeersReroll for the changes made in #1636992-19: form.js' formUpdated event is unreliable/incomplete. Necessary, because otherwise almost every form will break with a JS error.
Hence: RTBC +1.
Thanks sun! :)
Comment #130
sunI'm fairly sure that this no longer applies, since #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit was reverted. That commit was the cause for the second to last re-roll in #128.
I don't understand why #2047633 was reverted, given that there was a follow-up issue to fix the regressions already. It doesn't make sense to re-roll back/forward two monsters of this size and have them compete with each other.
Therefore, I'm not going to roll this back to #119 (but yet including the merge conflict fixes from #128 + #129).
We're going to wait for #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
Comment #131
sun129: library.info.129.patch queued for re-testing.
Comment #133
sunFixed merge conflict caused by #2166399: Changing CKEditor configurations by changing text formats causes markup to be lost: warn the user when performing this advanced action
Comment #135
sunFixed merge conflict caused by #2169447: DX: Supply CacheBackendInterface::CACHE_PERMANENT as Cache::PERMANENT
Comment #137
sunTestbot fluke.
Comment #138
sun135: library.info.135.patch queued for re-testing.
Comment #139
sun135: library.info.135.patch queued for re-testing.
Comment #141
sunClean merge.
Comment #142
XanoRe-roll due to
path.module
.Comment #143
sunDouble-checked that the re-rolled patch matches my local merge. (It matches, except for a strange commit message.)
Comment #144
XanoThe commit message is because I did
git show
.Comment #145
alexpottThis needed a reroll because of stuff I've committed... so here is the reroll. Also I'm raising the priority and tagging appropriately.
Comment #146
webchickI do not understand why we literally cannot release Drupal 8 without this patch, so not sure I understand the critical distinction. Nevertheless...
Just as a general patch scoping comment, there's a *ton* going on in this patch that really would've been better to split out separately, IMO. For example, just simply changing PHP arrays => YAML pretty is non-controversial. Probably so is moving the scope from system module to core. But then this is coupled as well with also renaming arbitrary elements of that info (website -> remote), forcing SMACSS groupings for CSS, etc. and suddenly this turns it into something that's incredibly hard to review, and is therefore going to need 3-4 people to do so, each on their own parts. :(
To be committable, this really needs more review from front-end devs in my opinion, especially themers. This change will disproportionately affect them vs. module developers. I'm sure YAML vs. PHP arrays is a no-brainer, but the specific syntax and groupings here could use a weigh-in by someone with more experience on that level. (So e.g. Lewis Nyman, John Albin, Cottser, that sort of folk.) I also see sdboyer talked about in this issue, but I don't see him actually commenting here, and it seems like his opinion would be good as well, to ensure it's inline with his work on the asset system. Statements like "will likely be consumed by" make me nervous, because we certainly don't want to do this work twice.
Eyeballing it DX-wise, one comment I can say is that I don't understand the use of curly braces here.
Elsewhere when we have deeply nested information, we just put each element on its own line, ala config schema files or what have you.
And it especially looks nasty when it's all by its lonesome, like so:
...what's up with that? The issue summary doesn't explain.
That "dependencies" stuff also looks really problematic, at least to me. All the Bartik theme is doing here is adding a CSS file called
maintenance-page.css
that happens to override some of the defined maintenance styling in core (though it could also override other styling in core). So in order to do so, now instead of just providing a (granted, convoluted and scary-looking) syntax to say "here's my CSS, please add it to the page," you also now need to also understand the entire family tree of how that CSS you're overriding got to be on that page to begin with.But how... exactly do you do that, as a theme author?
find . -name maintenance-page.css
only turns up Bartik's. System module's styling is calledsystem.maintenance.css
, how do you know that from here? Even grepping for e.g. ".maintenance-page" didn't turn up any clues.Normally, what you do as a theme author is whip out Firebug or whatever the tool du jour is, hover over the thing that doesn't look right, and add whatever IDs/classes/rules you need to make it look right. Find the next thing, repeat, etc. Now, your workflow turns into, "inspect the element and pay super careful attention to the filename it comes from" (and hope to $diety that CSS aggregation isn't turned on, or you are completely screwed), then grep for the filename to find the libraries.yml file where it's defined, THEN find its machine name in its libraries definition and add that to your "dependencies" array, and finally move onto the next broken piece." That totally breaks theme dev's workflows, or at least the theme dev workflows I've been part of.
So yeah, colour me concerned. :\ I'd be happy to be told that I'm worrying over nothing, but let's get some themers in here and see what they say.
Comment #147
Danny Englander@webchick, thanks for chiming in, as a themer I've been following this for several months now after I ran into #2147021: jQuery.once does not get loaded for anonymous users while porting one of my contrib themes to Drupal 8. I was subsequently directed here. I tested this patch while hanging out at SandCamp but did not have any luck with said contrib theme. By the time I came back to report my findings, the issue had been changed to "needs work" at that point so I figured I would give it a wait and let it play out some more. I hope to find some time this week to retest as I do get a little contrib time from my company. I think it's a good idea for themers to get more involved with this. I think it's a pretty important issue.
As it happens, I got my theme working fine with all the nested arrays but it did take a bit of work. I think having this as YML would be nice to lessen some headaches.
Comment #148
jibranWell @webchick is right that it is not a beta blocker. But it is at least major if not critical.
If it is a bit of work for themers then it is fine. Because for module developers we have a lot of work :D (routes, services, events and etc ;)).
I think other then themer's feedback we are done here in terms of development so can we please have a drafted change notice so that themers/module developer can understand the changes more easily and it is also a brand new core gate. I don't think it is that big of an issue as @sun has written brilliant issue summary.
And finally thank you everyone who worked on this and made this huge improvement.
Comment #149
alexpottSetting the issue status appropriately.
Also if the is to be the big DX win hoped for then the error message for a broken YAML needs to be improved. Currently it is:
Let's catch the ParseException and emit something a bit more helpful that includes the name of the file being parsed.
Comment #150
alexpottAs per #148 we need a draft change record before commit https://groups.drupal.org/node/402688
Also as a potential followup:
Blind acceptance of what is in the file - perhaps we can add validation to check that required keys are present.
Comment #151
sunI've the impression that the majority of concerns raised in #146 are lacking context with respect to how front-end assets and libraries are being handled in HEAD already. If anything, then the proposed change here only brings the developer-facing API much closer to the intended architecture that already exists in D8 HEAD already.
Yes, since the library definitions are converted into declarations, some unnecessary properties were removed (human-readable library label/title) and some properties were renamed/re-purposed ('website' → [git] 'remote' repository URL). However, a conversion from interpreted/executed PHP code into a static declaration has to address additional details:
The
CSS_*
PHP constants are no longer available in YAML. Several options were discussed in #64. But their usage in a dependency-managed library system does not make sense in the first place. Only a few libraries are defining a custom'group'
for CSS files in HEAD and all of those instances actually present hacks to work around an apparently broken CSS file asset grouping + ordering problem that already exists in HEAD, which has been introduced by the global conversion from directdrupal_add_css()
calls to properly defined libraries (again, all of this is in HEAD).The lack of CSS grouping information for all CSS asset files is the root cause for those one-off
'group'
adjustments. I'm 100% confident that every single of those instances required a dedicated core issue and plenty of manual testing to ensure that the resulting CSS files are actually loaded in the right groups and order. So contrary to the concerns being raised, specifically for the purpose of DX, and specifically for the purpose of not having to change library declarations once more, explicit SMACSS category groups are now an inherent part of the library definition itself.SMACSS category grouping has been officially and formally adopted for Drupal 8. The existing CSS architecture documentation page explains their purpose and usage, which is 100% in line with the declarations of this patch.
All libraries are declared in YAML, so any valid YAML is fine. It does not matter whether you are using the YAML inline syntax for hashmaps or the more verbose multi-line syntax to specify asset file options. An empty options hashmap is denoted with
{}
in YAML.Every library defines a list of its own dependencies. As the author of a library, you always know what your (direct) dependencies are. If you declare a CSS library that you load yourself in your theme and which intends to override System module's maintenance CSS, then that system library is your dependency. However, this entire concept is not introduced here, it exists in HEAD already and is the agreed-on plan for D8, the decision was made already (for good reasons).
Comment #152
alexpottAs it stands this patch will prevent us from using the Pecl Yaml parser - see #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available since it uses dots in the node anchors.
Will not work on Pecl.
Will work on both Symfony and Pecl
Comment #153
sunThat's perfectly fine, since the node references are limited to the local scope of a particular file anyway (i.e., they are just maintenance helpers and not a public API; they only exist within the scope of core.libraries.yml).
→ Use 'jquery_ui' instead of 'jquery.ui' as node reference anchor.
Comment #155
sunUpdating example in API changes section of issue summary accordingly.
Merged 8.x.
Comment #156
jhodgdon@sun: I went on a committing spree (with your permission) and committed a bunch of changes that touched some of the same files as this patch (but nothing touching hook_library_info()).
The latest patch still applies for me using the patch command but not using git apply, so you might want to reroll.
Thanks for the encouragement -- a lot of those issues had been waiting at RTBC for several weeks at least.
Comment #157
sunFixed merge conflicts.
Comment #159
klonos...we should really get used to hiding previous, obsolete patches/files when uploading new ones ;)
Comment #160
alexpottNeeds a reroll for #2193425: Adding block does not use dialog too
Comment #161
sunComment #162
klonos...the checkboxes to hide the old files are right there when you upload the new ones. It only takes a couple of extra clicks before hitting the save button.
Comment #163
Wim LeersI think you're essentially fixing a bug here, right? That's what the second highlighted hunk suggests.
If so: *great* catch! :)
Can you explain why we suddenly need to clear *all* statics?
Comment #164
sunre: #163.2:
This test asserts the fully themed page output of the maintenance page, which involves much more than just stylesheets — it also involves scripts and HTML HEAD elements (like meta tags). Each of the corresponding functions are using a
drupal_static()
cache. If those caches are not cleared/reset, then the consecutively rendered maintenance pages are missing the corresponding elements. → This test only passes in HEAD due to sheer luck and the coincidence of fragile function call chains and test assertions.(I actually had to invent a new "verbose diff of $expected vs. $actual output" functionality in
TestBase
to debug and understand what's going on, for which I will create a separate issue, 'cos that was pretty fancy to have :-)).This patch has been sitting in "needs review" for ages now and was RTBC before. We've reached out to various people already. By now I can only assume that the silence is to be interpreted as "Nice!" or at least "No objections."
Can we at least put it back into the RTBC queue, in order to increase the pressure for raising objections (if any) and clarify that - at minimum - the front-end related subsystem maintainers/stakeholders are happy with this change?
Comment #165
Wim LeersHaha, I'm looking forward to that added verbose diff debug output! :)
I totally agree with the sentiment in the bottom of #164. I've confirmed RTBC before, in #129. I'd do it again (after doing manual testing), but unfortunately this patch needs another reroll because it no longer applies… :(
Comment #166
longwaveRerolled.
Comment #167
Wim LeersManually tested again: create/edit node, in-place editing, toolbar, contextual links, Views. Did not find anything broken. Back to RTBC! :)
EDIT: Oh, this *should* have a change record now though, per the recent policy change of requiring a change record *before* commit.
Comment #168
nod_Created a change notice, it needs work to be more wordy and have better examples but it's a start https://drupal.org/node/2201089
Comment #169
webchickI guess I'll not mark this down from RTBC, but I still don't see any reviews from themers like I explicitly asked for in #146. I get that the JS people are all on board, because for JS people it makes sense to know what your dependencies are in advance. For CSS, though, this is a totally foreign concept; the entire point of CSS is you re-declare a rule with different styling and it magically works without you needing to understand the entire family tree of who declared that CSS and where. I'd also like to see validation that the SMACSS stuff makes sense to them, esp. the folks who worked on our CSS coding standards.
Comment #170
webchickAnd it looks like sdboyer is still missing from this issue, too.
Comment #171
markhalliwellReview from a themer:
Totally dig this. No PHP arrays!!!!
This is not entirely true. Given more modern CSS workflows (using a preprocess like SASS/Compass) we're pretty used to declaring dependencies in both our config.rb and Gemfile. It would be very nice to do a simple search against "*.libraries.yml" which is an easier to read format than PHP arrays.
Regardless, this really isn't a "CSS" issues. This is about libraries as a whole (both JS, CSS and whatever else). This makes a lot of sense.
PS: I could totally see a sass/compass module/extension being created to easily parse these files and allow them to be included as an actual "library".
Comment #172
nod_Something webchick wanted a validation on is the fact that we're adding the base/layout/component/state/theme "groups" when declaring CSS files.
Is that ok for everyone?
Comment #173
markhalliwellThe groups are perfect. These are libraries. Libraries should add their CSS to an appropriate group. It helps keep the order of specificity in check. If the library provides foundational CSS, then use base/layout. If it's all colorful and alters the specific look and feel of a site, theme. I can already see that for the Icon API module, I'll be using:
This is appropriate because this is a specific component of a site. Of course, if anyone really truly had an issue with a libraries CSS group/weight, it's probably a bug (that an issue should be created to fix) or site specific; in which case it should just be altered.
Comment #174
mortendk CreditAttribution: mortendk commentedyes it is - to me it makes it very clear what's what & should make it easier for everybody to understand why it's importent to group the CSS
Comment #175
joelpittetThe component and theme organization from SMACSS is great for core! Though I'd rather not force that organizational structure through to other developers if they don't want that. Not everybody follows or cares about SMACSS.
Is it possible to have this as well?
Sorry for my yaml ignorance if I didn't do that right.
Comment #176
RainbowArrayAs a themer, I'd much rather work with a .yml file than a PHP array.
I think one important note on the SMACSS notation to consider is that, based on the CSS architecture discussion at [https://drupal.org/node/1887922], state and theme styles can be intermingled with component (module) styles in the same file. This is particularly important when dealing with media queries, which are state declaration. It's better to declare variations on a component (or a layout) due to media queries right next to the thing that's being modified at various viewport sizes, rather than splitting those variations out into separate files. Separating the media query (state) variations from the component/layout declarations is very bad for maintenance. The same is true for other state variations such as pseudo-elements like :hover and such.
If somebody wants to separate out state and theme declarations into separate files, that's fine, but I wouldn't want that to be a requirement.
I am a little concerned about Webchick's comments in #146. She's right, if I find something messed up in a site's CSS that I want to fix, I typically find the rule I need to override it, create a reasonable override for that rule, and then I'm done.
That said, I'm not sure that's what's going on here. If I just want to make some changes to the site's appearance, in a subtheme of a base theme, for example, I can just do that in a stylesheet attached in info.yml.
My understanding is that libraries.yml is only for pulling in some sort of external library or framework. So as far as I understand it, the only dependencies that would need to be declared for something in core would be if a theme's libraries.yml was overriding a library that was already in core, right? That's going to be much more rare than the much more common task of overriding a particular CSS rule.
I can understand that it would be a challenge to find the core module necessary to declare jQuery as a dependency if aggregation was turned on, but if I'm working on a theme, I have the ability to turn aggregation on and off. I can't imagine why I couldn't do that in order to find what the dependency is.
So I'm mostly okay with this as long as we're not requiring state declarations, and possibly theme declarations, to be in separate files.
Comment #177
Wim Leers#176: theme CSS isn't declared as libraries. i.e. this remains unchanged before vs. after the patch, in
bartik.info.yml
:The changes here are solely for "libraries", which are logical groups of CSS + JS assets for (reusable) UI components.
Comment #178
LewisNymanI've never really seen any theme declare their CSS/JS as a library outside of core and contrib, so the affect this will have on themers is minor? Declaring JS dependancies in a custom theme is going to become required in Drupal 8, because we are no longer loading jQuery on every page, right?
Either way, this issue is a big improvement from a front-end perspective. It's a lot closer to the structure of bower.json and package.json which are becoming more and more common in front-end development. Maybe we could just parse the bower.json files that are included with most of our third party assets in core but that's probably not realistic.
Comment #179
nod_Correct. It's already required actually.
To be clear, if you declare your css file as a library you'll have to use the "groups". So #175 is not possible. But I mean if you just want to add a CSS file you can still use
[#attached][css]
and add the file directly, bypassing the whole library thing.Comment #180
Danny EnglanderI just cloned the latest from git and applied the patch and that seemed fine. I then removed the old
hook_library_info
call in my contrib theme and added this to a new libraries.yml file in my theme:... and then under a .theme file page_preprocess function:
I still cannot get jquery.once to show up for anonymous users so I suspect I am doing something wrong here but I just don't know what. Not sure if I should open a separate issue for this.This is now working with the code above.
Comment #181
nod_the "groups" things is only for CSS:
That should work. And don't forget you have to #attach it somewhere.
Comment #182
Danny Englander@nod_ - Do you mean I actually have to use
#attach
to add those scripts in my.theme
file or is this more of naming a corresponding function in the.theme
file? Are there any examples you can point too? Thanks.Comment #183
Danny EnglanderBTW, this should probably not be held up on account of my issue above, I think I'll get this sorted, help has been offered outside this issue, sorry for derailing this.
Comment #184
NaheemSays CreditAttribution: NaheemSays commented@highrockmedia: The comment above yours suggested a change to your libraries.yml file to make it work:
Change
to
Comment #185
sdboyer CreditAttribution: sdboyer commentedthis is all fine and +1 from me. the only thing i really need to note is #1762204-134: Introduce Assetic compatibility layer for core's internal handling of assets, which demonstrates that we have not actually done the rigorous work of correctly declaring dependencies in the context of CSS.
it may be that things have improved since then (i haven't been keeping a close watch). my issue there also *may* be fixed by the introduction of these SMACSS layers, which i have yet to accommodate in that patch.
Comment #186
sdboyer CreditAttribution: sdboyer commentedsorry, x-post
Comment #187
webchickSorry, I broke this with #2177469: Move node base widgets to the top level of the form, which was a critical that blocks other criticals so it jumped the "avoid commit conflicts" line.
I'm planning to do a huge commit spree right now, so maybe hold off on re-rolls until ~2-3 hours from now so work isn't wasted.
Comment #188
joelpittet@_nod so my stance is still the same, I'm not confident that forcing the SMACSS groupings is the best way to go. I still am fine with doing it that way in core but I wouldn't want to force that convention on contrib or anybody else defining libraries. Everything else in this patch is great btw, and it seems not too many people share this opinion about the groupings. If you and others are confident that's the way to go then I'll trust your good judgement.
Comment #189
markhalliwellOne could easily just use the group "theme" for everything. I think that is why I don't really share the opinion that it's being "forced". It simply allows a level of granularity that is desperately needed to separate for core/contrib that do need it.
That all being said, I will admit there is a little inconsistency (edit: and confusion) between how the JS is implemented than the CSS as evident by #180-#184. I haven't read this entire issue, so this may have already be proposed and may have a reason as to why we're not doing it this way, but an alternative would be:
Comment #190
Jeff Burnz CreditAttribution: Jeff Burnz commentedThe SMACSS stuff seems to have been of most interest for themers to review and this seems reasonably strait forward - however I have one question.
Why the "theme" group (not a group really but a weight)?
For me this caused confusion because the constant is CSS_SKIN which we use in #attached etc, so while this says it is a 1 to 1 mapping to the SMACSS layers (which really it is) but why use theme here at all, I'm not sure I follow the reason why to do this:
Instead of just
$options['weight'] += constant('CSS_' . strtoupper($category));
Am I wrong here, I have hardly read this issue and only spent a few minutes on it when porting some stuff I am working on and thought I better see how this works.
EDIT: just wanted to add that this is an awesome patch and I found it very easy to port my stuff, and I really like the "remote" key. The actual syntax is a little odd at first compared to other yml files but is just different and nothing major IMO.
Comment #191
benjy CreditAttribution: benjy commentedWhat @MarkCarver said in #189 about the inconsistency for developers which was proven by #180 was the first thing that came to mind for me too.
Otherwise, I'm really liking the direction of this, great work!
Comment #192
Jeff Burnz CreditAttribution: Jeff Burnz commented#191, yes perhaps, for me I work with these yml files a lot so having a different array/data structure for different things within a yml file is totally normal for me, however I can see how someone might be confused by this - core.libraries.yml is a good place to start and after I read that the differing structures were self evident.
Comment #193
sunThanks everyone for all the feedback, insights, and reviews! :-)
Most questions have been answered very accurately by @nod_ and @Wim Leers already, so I will only address the last remaining questions of:
A. (In)Consistency between JS and CSS declarations
_drupal_add_js()
do not participate in the dependency resolution/management for all other JavaScript assets in D8; i.e., a correct output/loading order cannot be guaranteed. That is why that function has been as private. All code uses #attached instead.foo.css
is completely unrelated to the addition ofbar.css
. As a consequence, the "group" ofbar.css
cannot be derived fromfoo.css
(or any other CSS file/library, for that matter).foo.css
is a CSS component, whilebar.css
is a pure skin/theme CSS.foo.css
andbar.css
. If there is no dependency chain between two (or more) CSS files, then even the best dependency resolution (directed acyclic graph) implementation is not able to help you further, because there is simply no relation between A and B. — But yet, we still have to load CSS asset files in their correct groups.→ Because there is an expected difference with regard to whether any kind of dependency chains exist, the CSS asset file declaration is different from the JS asset file declaration.
→ A JS file without ancestors/sisters/children can be unconditionally loaded first. A CSS file cannot and must be loaded in a specific group.
This elaborate explanation hopefully addresses all concerns with a reasonable argumentation.
B. The "theme" group diverges from the actual SMACSS category "skin"
This patch is really not guilty for that. The group name "theme" is literally derived from the Official Drupal 8 CSS architecture documentation.
AFAIK, the name "theme" was chosen instead of "skin", because Drupal already has a concept of "themes", and themes want to be able to override any kind of
.theme.css
. In addition, we introduced a formal split intofoo.base.css
andfoo.theme.css
for D7 already. That naming decision is also the reason for why the corresponding CSS files are namedfoo.theme.css
instead offoo.skin.css
in D8.I'd be more than happy to change the group name from "theme" to "skin", but given that "theme" is a formal part and collectively agreed-on naming of our CSS architecture standards policy for D8, that appears to be a much larger aspect and discussion of its own.
→ Renaming the group "theme" to "skin" is the only follow-up issue to the new library declaration format being introduced here that I can think of.
→ See you in #2202671: Rename CSS_SKIN constant to CSS_THEME :-)
I hope we can keep that topic in that issue.
Comment #195
sunUgh, sorryyy — @Mark Carver just pinged me on the inderdiff...
The last interdiff was stupid/nonsensical to attach; it contains the complete differences between 8.x from ~1-2 weeks ago and today. :P
I only attached it out of habit. There were plenty of conflicts when merging latest 8.x, but I was able to resolve them in the merge commit already, so there is no interdiff, and thus, please ignore that interdiff. ;-)
That said, testbot claims a test failure just in time. But that looks like a random testbot fluke to me, so re-testing in a moment.
Comment #196
Jeff Burnz CreditAttribution: Jeff Burnz commented@193 - top work sun, thank-you for the detailed explanations and your heroic efforts with this patch.
Comment #197
sdboyer CreditAttribution: sdboyer commentedi didn't realize there was this much discord over SMACSS.
it needs to be understood that we require *clear* rules on which to do the sorting for the whole new graph-based system to be able to work. it's SMACSS, or not SMACSS, period.
fwiw, "not SMACSS" will produce fewer aggregates.
Comment #198
sunYeah, to clarify further:
This patch just (1) enforces a SMACSS group for every CSS file and (2) processes that group into the existing
'weight'
option that is passed to_drupal_add_css()
later on:Whether we want to keep it that way (just stick with weights), or whether we want to pass on the group information separately to the graph implementation, is a question that we do not need to answer here.
What matters is that every CSS file (1) is guaranteed to have a group and (2) will be loaded [at least] with the correct weight of the corresponding group. Given a group, pretty much all CSS files should not need any further or more granular adjustments to their weight within the group; i.e., hacks like
weight: -1
should be obsolete.In other words:
→ All we're doing here is to ensure that we have the most minimal group/weight information for each CSS file.
→ How exactly that information is used later on when resolving/loading dependencies and producing aggregates is a different question, which doesn't really matter for the library declarations. — The code for processing the library declarations can be happily changed to match and produce whichever information we want to have for resolving/grouping/aggregating assets.
Comment #199
RainbowArrayI hadn't read through the discussions on CSS architecture changes and the use of SMACSS. Now that I have, it makes a ton of sense.
So +1 to finishing up any necessary rerolls and getting this in.
There's no need to bikeshed SMACSS vs non-SMACSS here. There's already been a ton of thought and work put into heading in the direction of SMACSS organization. This patch just implements those already-made decisions.
Getting this into YML is a huge win. Thanks for all the work @sun.
Comment #200
sdboyer CreditAttribution: sdboyer commented@mdrummond - this may be a discussion that needs to go elsewhere, but it is not a bikeshed.
@sun re: 198 - there *is* no weight information. weight is gone. as are the aggregate groups that we used to use.
do what y'all are gonna do. if i can work around it, i will. if i can't, we'll cross that bridge.
Comment #201
sunJust to clarify on #200, because it sounds a bit negative: ;)
I just had a longer discussion with @sdboyer in IRC in order to get on the same page and double-check whether this patch introduces a conflict for the upcoming Assetic changes. We both concluded that it does not.
We even questioned whether the SMACSS categorization/groupings on their own aren't (theoretically) sufficient for sorting CSS asset files (sans any further dependency resolution), because the order of individual CSS files within a SMACSS group should (theoretically) not matter... Anyway, in the end, we concluded that the big challenge remains to be and is going to be aggregation, or more specifically, to ensure that multiple aggregate files on the same page will not override more-specific CSS of a former aggregate with less-specific CSS in a later aggregate. But that's rather off-topic here :)
Now, just for extra kicks, I'm attaching a graph of all libraries (in core) that are declaring CSS asset files (excluding JS dependencies).
(Green libraries do not have own dependencies, so if those are loaded on their own, then only the SMACSS category kicks in. Red libraries are flagged as "every_page".)
Comment #202
webchickOk, awesome. Thanks very much for seeking out themer opinions on this issue. Sounds like everyone's basically on board with this being a good direction, though there are some follow-up issues to discuss further refinement. Also sounds like this has a neutral effect on the work Sam's doing, so +1 there, too.
Committed and pushed to 8.x. YEAH!
Comment #203
webchickBtw, it'd be great for one of the folks who tried out porting their libraries to this new syntax to check over the change record at https://drupal.org/node/2201089 and review it prior to publishing to make sure it has everything you would've needed to do so. :)
Comment #204
markhalliwellAdding "Approved API change" for prosperity/record keeping and since it was committed.
Changing title/status and adding "Needs change record" since it's in draft and not published.
Comment #205
sunFixed some faulty HTML markup and published the change notice: https://drupal.org/node/2201089
Created the follow-up issues as stated in the issue summary:
#2203407: Replace #attached library array values with provider-namespaced strings
#2203411: Convert drupal_get_library() into a service
Comment #206
sunAlso created #2203415: Verify/fix missing dependencies in libraries to evaluate and investigate the currently declared dependencies.
Comment #207
sunCreated two additional follow-ups to resolve @todos for some of the current core libraries, as discussed earlier in this issue:
#2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0
#2203435: Move all external libraries from core modules into core.libraries.yml
Comment #208
Wim LeersAwesome work, sun!
And thanks to those who provided "themer feedback" for an excellent, mature, non-bikeshedding discussion! :)
Hurray for another DX improvement!
Comment #210
tim.plunkettThis broke the Views UI, could use a review on #2217669: [REGRESSION] Views UI broken
Comment #211
webchickHere is some developer feedback from a themer actively trying to use this to add in an external JS library to a theme: #2298551: Documentation updates adding/adjusting inline/external JS in themes Would love suggestions there on how we could've made this not an hours-long task, but still retain the benefits of what this patch is trying to do.
Comment #212
cilefen CreditAttribution: cilefen commented