Follow-up to #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name
Problem/Motivation
stylesheets-override and stylesheets-remove as is don't necessarily give you the level of granularity needed to meet real world use cases.
Proposed resolution
From @Wim Leers in #9
We discussed this before at some point. I think the solution lies in fully embracing the asset-library-based Drupal 8, and thus removing any remnants from the individual-asset-based past.
That means:
libraries-override
- The ability to override entire libraries and parts of libraries:
libraries-override: # Replace an entire library core/drupal.collapse: mytheme/collapsed # Replace one particular library asset with another. subtheme/library/css/theme/css/layout.css: css/layout.css # Remove one particular asset. drupal/dialog/css/theme/dialog.theme.css: false
- Note that we could even omit
libraries-remove
if we'd allowlibraries-override: # Remove an entire library. core/modernizr: false
That would cover the use cases of stylesheets-override
and stylesheets-remove
.
(Note that this is basically a declarative form of hook_library_info_alter()
.)
Remaining tasks
- Add libraries-override
User interface changes
N/A
API changes
API additions.
Beta phase evaluation
Issue category | Task because it is an API addition. Nothing is broken. |
---|---|
Issue priority | Major this would be a huge themer experience win and needed to help provide functionality that was lost when stylesheets-override was removed. Not critical because nothing is broken. |
Unfrozen changes | Unfrozen because it is an API addition. |
Disruption | This should not be a disruptive change unless we decide to remove stylesheets-remove. |
Comment | File | Size | Author |
---|---|---|---|
#166 | libraries-override-2451411-166.patch | 37.93 KB | almaudoh |
#166 | interdiff.txt | 4.34 KB | almaudoh |
#165 | libraries-override-2451411-165.patch | 39.03 KB | almaudoh |
#165 | interdiff.txt | 3.58 KB | almaudoh |
#163 | libraries-override-2451411-163.patch | 37.97 KB | almaudoh |
Comments
Comment #1
star-szrComment #2
dawehnerI'm curious whether we could do something like the following:
We store libraries per theme, so that the cached libraries are processed with the defined entries in stylesheets-overrides and stylesheets-remove.
This would allow you to get rid of using that logicon runtime, but rather keep using libraries everywhere.
Comment #3
LewisNymanI mentioned on IRC that I think sticking with paths and wildcards would be more conventional. Similar to how the magic module syntax works right now.
Comment #4
davidhernandezI definitely would this to work using namespace or full path, and not by library.
So I assume by namespace it would be?:
and with path:
By name is preferred.
Comment #5
lauriiiI think it makes sense to be able to remove single css file from a library because if it would be base on libraries you would have to replace whole library. Anyway the solution we have now doesn't fullfil the expectations of people using it which at least for me in most of the cases is that I want to override/remove single CSS. However after all you are going to replace all instances of that CSS file existing in the Drupal installation.
After changing this to be based on the path of the file we might be able to remove the stylesheet-override because you would be able to remove single CSS file and then include it using libraries.
Comment #6
star-szrI think override still makes sense and is needed because libraries are #attached only in certain cases, so if you just remove CSS from a library and add your own, it won't be loaded the same times as the original library.
It seems pretty possible to me to allow for overriding/removing a single CSS file from a library. And I think if all CSS is added by libraries it probably makes sense to override/remove that way rather than by extension. As long as library names are globally unique – which I think they are?
Comment #7
davidhernandezI'm pretty sure they're not. I can create a subtheme of Bartik with a library called "global-styling".
Comment #8
davidhernandezI hadn't thought of that. In that case, the override makes sense.
Comment #9
Wim LeersWe discussed this before at some point. I think the solution lies in fully embracing the asset-library-based Drupal 8, and thus removing any remnants from the individual-asset-based past.
That means:
libraries-remove
libraries-override
libraries-remove
if we'd allowThat would cover the use cases of
stylesheets-override
andstylesheets-remove
.(Note that this is basically a declarative form of
hook_library_info_alter()
.)… but we can go further, and consider improvements through API additions:
(copied verbatim from #2377397-17: Themes should use libraries, not individual stylesheets)
Comment #10
davidhernandezLooking at #9, I'm fine with doing it by library if we retain the ability to target individual files. There is no objection to the library concept, only to being limited to removing/overriding the whole library without flexibility.
Comment #11
star-szrIndeed, I would love to embrace libraries, what @davidhernandez said :)
Comment #12
star-szrArguably a major improvement to be had in the granularity and control we are talking about here :)
Comment #13
Wim LeersIt looks like we agree on that.
Comment #14
sqndr CreditAttribution: sqndr commentedI agree with #10 as well. :)
Comment #15
davidhernandez@alexpott suggests we merge this with #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name because that critical fix exposes underlying brokenness that this issue intends to fix. If we do that, do we still want to tackle using libraries or just fix stylesheets-remove/override? I wasn't sure if Wim's suggestion in #9 would remove stylesheets-remove, or the library functionality would just be an API addition. If it is an addition, we can fix stylesheets remove in the other issue and do the addition work here.
Regardless, we need to decide on how to specify the assets, so we can do it the same in both places. If we are going to break an API, lets only do it once.
Comment #16
joelpittetComment #17
joelpittetComment #18
davidhernandezI talked to xjm and alexpott about this real quick. There shouldn't be a problem getting this into 8.1 if we don't get it done in time. We should still try to get it done before RC, but if we run out of time we don't need to postpone it to 9.
Comment #19
mducharme CreditAttribution: mducharme commentedSince stylesheets-override was removed in https://www.drupal.org/node/2389735 should we also consider dropping the libraries-override and simply rely on libraries-remove for consistency?
Comment #20
davidhernandezWe want libraries-override. The stylesheets-override removal happened because we couldn't fix a critical bug while maintaining its functionality. Libraries-override would allow a theme to replace a particular asset, without removing the whole library. For example, if you want to replace just one CSS file, but keep the functionality of the rest of the library, especially its conditional inclusion.
Comment #21
andypostlibraries-override
is needed forvertical-tabs
andprint.css
Comment #22
jstollerThis appears to be needed for #2489564: Move user.theme.css to Classy.
Comment #23
almaudoh CreditAttribution: almaudoh commentedWorking on this for the next couple of hours.
Comment #24
almaudoh CreditAttribution: almaudoh commentedEventually took longer than a couple of hours. After much thought, I figured the best approach is to implement individual component overrides/removes in the
LibraryDiscoveryParser
while deferring entire library overrides/removes to theLibraryDiscovery
.I'm going with #9. libraries-remove is implemented by specifying
libraries-override: false
First patch just to get things going...Added tests to confirm functionality. Also left in
stylesheets-remove
for now. Can be removed later.Comment #28
star-szrJust a thought (and may be overkill…) but should we provide library granularity here so that an asset included in multiple libraries can be treated differently?
Comment #29
almaudoh CreditAttribution: almaudoh commentedThat's what this does actually. The
classy/base
refers to the library definition, not the file path. The file path starts fromcss/layout.css
relative to the library where it was defined. So if you define that same .css file in another library, it will get included via that library.Comment #30
almaudoh CreditAttribution: almaudoh commentedFixed the test fails and added some code comments.
Comment #31
almaudoh CreditAttribution: almaudoh commentedPrevious interdiff was wrong.
Comment #33
star-szrOops, missed that part. Great! Looks very close to green :)
Comment #34
almaudoh CreditAttribution: almaudoh commentedForgot to create the css and js files referenced in the new
test_theme.libraries.yml
. This should be green now.Was just wondering if we could extend the scope of this issue to include libraries-extend as well - seems that is really needed at this time too.
@Cottser: are we ok with the nomenclature here? I know libraries-override came from stylesheets-override, but I think library-overrides would sound more correct.
Comment #35
almaudoh CreditAttribution: almaudoh commentedOops, sorry wrong patch. Correct one attached. Interdiff is correct.
Comment #37
star-szr(I canceled the patch from #34)
@almaudoh awesome! I'm reluctant to extend the scope of this issue but we could definitely use your help in implementing libraries-extend! I haven't had a chance to create the issue but you are welcome to if you'd like :)
Not sure right now on the nomenclature, if we went with library-overrides would that make it library-extensions or something then?
Comment #38
star-szrA couple small questions, sorry not a full review at this point.
Are the quotes needed? Probably not, right, because YAML?
I think we use lowercase booleans in YAML.
Comment #39
davidhernandezIf we stick with convention, that would mean using plurals. "libraries" since you can manipulate more than one library at a time. The match works for removal of a library so we don't need "libraries-remove". Is there a way to work extending into the same grouping? I'm guessing that would be hard with the "something: something" syntax.
Comment #40
almaudoh CreditAttribution: almaudoh commentedIt would not allow for adding new stuff into an existing library. The syntax in #9 would be more flexible and therefore would need to be in a separate group.
Comment #41
star-szrTo follow up on #38:
This only had quotes because @classy is some kind of token according to the YAML spec. No @, no quotes needed - #2481183: Quote tokens in theme .info.yml files.
Comment #42
almaudoh CreditAttribution: almaudoh commentedOne more iteration. Addressed comments in #38 and #41. I'm always confused by this uppercase true/false - lowercase TRUE/FALSE dichotomy. Left to me we should just use lowercase throughout.
Also ripped out
stylesheets-remove
completely, though I retained the tests to confirm equivalent functionality is still provided bylibraries-override
.Looks much better now.
Comment #43
almaudoh CreditAttribution: almaudoh commentedSelf-review.
This will result in loss of settings attached to the original component. Re-use existing settings for now.
Some libraries define additional settings e.g. normalize library defines
assets/vendor/normalize-css/normalize.css: { every_page: true, weight: -20 }
. This would be lost in the code block below. Will fix in next patch.That also exposes the fact that the current
libraries-override
syntax would not allow for overriding those kinds of settings. Are there possible use-cases for this, or maybe we can do that inlibraries-extend
Comment #44
almaudoh CreditAttribution: almaudoh commentedFixed #43 and added test for it.
Comment #45
davidhernandezWe should leave stylesheets-remove in, at least until we get confirmation from a framework manager that we can take it out. Removing it changes this from an api addition to a disruption change, so we need to verify we can do that at this late stage in the beta phase.
Also, I'm adding needs change record. The addition of libraries-override will need one. If we do remove stylesheets-remove we'll need a separate one for that as well.
Comment #46
almaudoh CreditAttribution: almaudoh commentedComment #47
star-szrI agree with @davidhernandez. Thanks for all the work here @almaudoh!
I think this issue can still switch over all (or some) of the uses of stylesheets-remove, but yes that should probably be removed in a separate issue (if at all because of where we are in the release).
Comment #48
Wim LeersOne of the best, clearest, cleanest, most well-tested patches I've seen in a while. Awesome work, @almaudoh. Thank you! :)
Hrm, let's do this in two steps for clarity:
"Active theme defines an override for an asset within this library."
(For similarity to the if-branch comment at line 333.)
Let's use strict equality.
What are "library components"?
Needs a newline in between.
"overrides" vs. "override"
One newline too many.
Excellent test coverage!
Where did you find this "component" term? If it's used elsewhere, it's fine, but so far, the terminology has been to my knowledge: "asset library", which contains "assets". No mention of "components".
AFAIK these files don't even need to exist!
If you want to keep them, then please capitalize "css" and "js" to "CSS" and "JS", respectively.
More excellent test coverage! Thank you! :)
s/js/JSS/
Unwanted additional newline.
#43: great point. IMO there are two ways to go about this:
\Drupal\Core\Render\Renderer::mergeAttachments()
howdrupalSettings
is supposed to be merged.drupalSettings
in a library, and throw an exception in case somebody tries to do so. Add test coverage that verifies that this happens.I think option 2 is fine, especially because it could simply be a non-API breaking feature addition in a future version of Drupal 8.
Comment #49
almaudoh CreditAttribution: almaudoh commentedThanks for the reviews and comments.
#45 / #47: @davidhernandez, @Cottser: Beta evaluation guidelines allow "follow-ups from a recent critical or major change" as prioritized changes. I know there's not much disruption for core (one or two lines in Bartik, Seven and Stark and the test themes). I don't know exactly how many themes in contrib use stylesheets-remove at this time. IMO, i think we should remove it completely (or at least convert all core usages and maybe leave in a BC-shim). I haven't reverted it in the patch yet, still awaiting the decision of the framework managers.
#48: @WimLeers: Thanks for the great review and nice comments :)
TwigTransTest
fails if the files don't exist because it installs bothtest_theme
theme andlocale
module.Locale
module tries to load JS files inhook locale_js_alter()
when it callslocale_js_translate()
and fails if the file doesn't exist.On #43 I was actually referring to the attributes specified for JS/CSS assets (Used the wrong word again :P). However, your point on drupalSettings makes sense - and I agree to option 2 as well. Implemented.
Added explicit check for malformed libraries-override asset specification and throwing an exception if not well formed.
Comment #50
Wim Leers10. Aha, ok!
This is now very close IMO.
Let's say what is wrong in the exception message.
s/should not/may not/
s/cannot/may not/
In the exception message, I'd point the user to
hook_library_info_alter()
instead. There, they can do this.Can you also update the name of this assertion, so that no "component" stuff is left?
The
$this->pass()
+$this->fail()
can be replaced with a single$this->assertEquals()
.Comment #51
almaudoh CreditAttribution: almaudoh commentedThanks @WimLeers:
1. Done. Hope this is good enough.
2. Done.
3. Done.
4. Nice one. Didn't see that. Thanks.
Still waiting for framework managers review and comments on whether to keep
stylesheets-remove
or not.Comment #52
Wim LeersUnless we already have consensus, we can remove
stylesheets-remove
in a follow-up. Then we can get this in. Progress matters!Please add a change record and a beta evaluation. Once that's present, this is RTBC IMO. Just a bunch of minor things:
This is plural, but elsewhere it's singular. Let's be consistent.
Missing
@returns
.s/overrides/override/
Plural, even though the key in YAML files is actually singular.
Here you got it right.
Comment #53
joelpittetI'm with the other theme system co-maintainers on this. We should probably not remove
stylesheets-remove
at this point. Definitely remove it from this patch to unblock it.Maybe deprecate it to discourage use and as a reminder to remove it in 9.x.
Unless there is a compelling case to remove it (aka breaks things... or other beta evaluation conditions)?
Comment #54
Wim LeersI think the key question is:
. I think the answer is .But, because
stylesheets-remove
is now a subset of whatlibraries-override
does, we could potentially internally mapstylesheets-remove
tolibraries-override
. That would then allow the logic to be simplified, and to just have one bit of (easily removable in D9) code that translates one to the other.Comment #55
davidhernandezComment #56
davidhernandezAdded a draft change record for the libraries-override part.
Comment #57
almaudoh CreditAttribution: almaudoh commentedOk. Fixed review comments in #52 and restored
stylesheets-remove
.The one thing that makes it hard is that
stylesheets-remove
uses relative paths and placeholders like '@classy/css/layout.css' to specify stylesheet paths. I don't think it makes sense to write new code (including tests) to map this back to the correct libraries for something we are deprecating. I have therefore left in the code and tests as it is in HEAD.One change though is moving the
stylesheets-remove
piece inThemeInitialization
to a separate (private) method so we can easily delete later on.Also added todo's for removal in D9.
Comment #58
almaudoh CreditAttribution: almaudoh commentedand the patch...
Comment #59
almaudoh CreditAttribution: almaudoh commentedComment #60
Wim LeersYep, that's what I feared. So +1 to your solution!
I think this will be the last review round :)
There are trailing spaces on these 2 lines.
I don't think we ever make these private. Let's keep it at protected.
Similarly, let's make this protected
Needs newline in between.
Doesn't this mean we have no test coverage left for
stylesheets-remove
?Comment #61
almaudoh CreditAttribution: almaudoh commentedThanks @WimLeers. #60:
1. Fixed
2. Done. I noticed we never use
private
in Drupal, even in cases like this where we don't want the method to be sub-classed. But no harm though.3. Done.
4. Fixed.
5. There's still test coverage in
ThemeInfoTest::testStylesheets()
using thetest_basetheme.info.yml
andtest_subtheme.info.yml
which I reverted in the last patch (see the interdiff).Since we're deprecating
stylesheets-remove
, I don't think we should be using it in core themes. Note that this test is still useful forlibraries-override
so can be left in whenstylesheets-remove
is eventually, well, removed.Comment #62
Wim LeersAlright. Thanks!
Splendid work here! :)
Comment #63
star-szrHere's the issue for libraries-extend: #2497667: Add libraries-extend to themes' *.info.yml
Comment #64
alexpottI think we should deprecate
stylesheets-remove
a schedule it for removal in Drupal 9.0. Which is the approach the current patch takes - great! And we've still got test coverage inDrupal\system\Tests\Theme\ThemeInfoTest
. Still to review.Comment #65
alexpottLet's use sprintf here - exception messages don;t need to go through SafeMarkup.
Do we test base theme inheritance of library overrides anywhere? I couldn't spot it.
Missing docblocks and an asserts these should return a bool. Just do
return $this->pass($message);
andreturn $this->fail($message);
We need to ensure
stylesheets-remove
is still tested - how about leaving this in and removing another stylesheet in the libraries-override.Comment #66
lauriiiThere is not tests for the theme inheritance and libraries-override yet. Fixed other points from Alexpotts comment and might be fixing the tests little later.
Comment #70
almaudoh CreditAttribution: almaudoh commentedThanks @laurii for #66
Working from there, added test coverage for basetheme/subtheme libraries-override inheritance. Nice catch @alexpott!! This revealed some bugs which has led to a gamut of fixes.
In this patch:
1. Fixed problems with parsing of protocol-free and full-protocol URI's in
libraries-override
and added tests.2. Fixed HEAD's mangling of full-protocol URI's if 'external' is not specified (a bug??) and added a test.
3. Fixed some more calls to
SafeMarkup::format()
in exceptions and assertions.Comment #72
almaudoh CreditAttribution: almaudoh commentedOh...
TwigTransTest
:/ Changed all the test files to css to avoidlocale_js_alter()
hook's file checks.In the process I've found that
_locale_parse_js_file()
rejects files specified with streamwrappers e.g.public://dir/file.js
- that's another issue I guess.Comment #73
almaudoh CreditAttribution: almaudoh commentedComment #75
almaudoh CreditAttribution: almaudoh commentedWas too sleepy to post this yesterday...
Comment #76
lauriiiThanks a lot for working on this! Here's what I found for now:
s/else if/elseif
Even though its not a coding standard in Drupal I'd prefer a extra line before return :)
This could be on the first line I think
s/array()/[]/
Maybe string[]
Comment #77
Shamsher_Alam CreditAttribution: Shamsher_Alam commentedComment #78
Shamsher_Alam CreditAttribution: Shamsher_Alam commentedComment #79
Shamsher_Alam CreditAttribution: Shamsher_Alam commentedComment #80
lauriiiShamsher_Alam: Thank you for working on this issue! Maybe if you have time you could still add an interdiff for the patch you posted? Here's tutorial how that happens: https://www.drupal.org/documentation/git/interdiff
Comment #82
almaudoh CreditAttribution: almaudoh commented@ Shamsher_Alam: I think you've lost some parts of the patch. The patch at #75 is 33KB while yours is 30KB.
Comment #83
Shamsher_Alam CreditAttribution: Shamsher_Alam commented@lauriii
Inter diff of patch.
Comment #84
Shamsher_Alam CreditAttribution: Shamsher_Alam commentedComment #85
almaudoh CreditAttribution: almaudoh commentedExtra space
This change is out of scope.
s/string/string[]/
Comment #86
almaudoh CreditAttribution: almaudoh commentedFixed #85
Comment #89
almaudoh CreditAttribution: almaudoh commentedComment #90
cilefen CreditAttribution: cilefen commentedI am rerolling this.
Comment #91
cilefen CreditAttribution: cilefen commentedreroll
Comment #92
cilefen CreditAttribution: cilefen commentedThis is not an exhaustive review.
This looks like an unrelated change.
Is there not already a core function for testing valid URIs that we could use here?
This needs a return comment.
This needs a return comment.
This needs a return comment.
Non-native speakers of English may not understand the invented word "edge-casey".
A comment for this return is needed.
A comment for this return is needed.
Is this use statement needed by the added code?
These do not seem to be used in the class.
Comment #93
almaudoh CreditAttribution: almaudoh commented#92:
1. Not really related, just code cleanup ;)
2. Didn't find any.
3. Well, I thought that would just be redundant considering what's already in the doc block. But no problem. Fixed.
4. Fixed
5. Fixed
6. Fixed
7. Fixed
8. Fixed
1. Fixed
2. Fixed
Thanks for the review @cilefen
Comment #94
almaudoh CreditAttribution: almaudoh commentedPatch
Comment #95
davidhernandezWe should get a little manual testing on this to make sure it is working well and to make sure how it operates makes sense for themers. You can use the change record to see how it works, or we can probably add some instructions if needed.
Comment #96
star-szrWarning: Lots of nitpicks. Overall this is looking fantastic to me and the test coverage seems very thorough from my perspective.
Note that the review doesn't follow source order, I've rearranged things so that my questions/code things are first, and then the rest is all nitpicks, all the time.
I'm probably missing something but what is the purpose of the unset() here?
Minor but I couldn't see why these
use
s were added.What is sub_key here? In other words could this exception message be improved so that it's easier to fix when someone runs into this?
Minor: elseif per https://www.drupal.org/coding-standards#controlstruct.
Minor: s/drupal/Drupal/
Minor: "…in the library" would be a bit less terse here (2 instances).
Similarly here, "where the given asset…" (2 instances).
Minor: The parts underneath @param and @return here need be indented one extra space (3 instances).
Minor: s/pagei/page/
Minor: I think the second lines here need to end in a period per https://www.drupal.org/node/1354#file.
Super minor: These should end in a period (4 instances).
Comment #97
almaudoh CreditAttribution: almaudoh commentedRe-roll after #2349711: Remove all visual from stark + fixed #96 nitpicks.
#96:
1. This is for when you want to remove the entire library. This can't be done in the
LibraryDiscoveryParser
because the parser doesn't know about the $extension.2. Fixed
3.
sub_key
is like'js'
or'css'
. A better name is welcome.4. Fixed
5. Fixed
6. Fixed
7. Fixed
8. Fixed
9. Fixed
10. Fixed
11. Fixed
Thanks for the review. Still needs manual testing per #95. Looks RTBC from my end though.
Waiting on this to go in so we can progress a patch for #2497667: Add libraries-extend to themes' *.info.yml
Comment #98
joelpittetI think sub_key is fine as that was what it was. I was thinking group or type... that may be a slight improvement... but unless someone has better or likes one of those, I think we can leave that as sub_type too. @Cottser follow-up maybe?
Comment #99
star-szrIf sub_type can only be CSS or JS can we add something short to the end of the exception message to say (sub_key is either CSS or JS) or something? It just feels a bit cryptic now.
Comment #100
almaudoh CreditAttribution: almaudoh commentedType could be easily confused with other meanings so I didn't go with that. I think group or sub-group sounds better. I didn't really like sub-key myself.
Comment #101
star-szrWhat about something like asset_type? And to expand a bit more on my phone-typed comment from earlier, I think an exception message along these lines would be more helpful, but this could be improved probably:
I'm doing some manual testing.
Comment #102
star-szrTo add to my last comment, should we also mention it needs to include the category for CSS? That could easily throw people.
After manual testing, some thoughts:
Maybe these should all fail silently so we don't have people creating dependencies on things they are wanting to remove/override but I accidentally ran into the second case while testing by typo'ing. Seems like we'd want to make sure the library exists at minimum.
Comment #103
Wim LeersWe are fixing that in #2389203: Validate the CSS categories in libraries.yml files..
Comment #104
almaudoh CreditAttribution: almaudoh commentedThese make sense. TX++. Will add in the next patch.
Comment #105
almaudoh CreditAttribution: almaudoh commentedThis should throw an exception if a non-existent library is assigned on the right-hand side.
It also throws an exception for a non-existent library or library asset.
It doesn't yet verify that the libraries and assets on the left side exists. The code for that appears quite complex.
Next, work on the tests.
Comment #109
almaudoh CreditAttribution: almaudoh commentedFixed the buggy code. This should:
Added tests for 2) and 3). Will add test for 1) tomorrow.
Comment #111
cilefen CreditAttribution: cilefen commentedRe #18, we have a non-passing patch. I feel like we should postpone to 8.1.
Comment #114
almaudoh CreditAttribution: almaudoh commented@cilefen: let's try one more time... if we can't get the DX improvements requested in #102 to pass, then maybe we just do without those. Patch was passing before then.
Comment #115
almaudoh CreditAttribution: almaudoh commentedThinking through this, I just realized I was doing it the wrong way. The
LibraryDiscoveryParser::applyLibrariesOverride()
only knows about the libraries in one extension at a time, so there is no clean way to verify that all libraries overrides for all extensions are correctly specified.Again, considering some more, I realized that #102 is better implemented using assertions (https://drupal.org/node/2492225) since it's for the themer writing the theme and not the user.
I've therefore opened a follow-up #2562839: Follow-up to [#2451411]: Add assertions in Library(Discovery)Parser to prevent typos. to do that using assertions and removed it from this patch. Added an interdiff against #97 to see what has actually changed.
Comment #116
star-szrThanks @almaudoh! I think it's fine to punt on some of this stuff, did not mean to hold things up.
Comment #117
Wim LeersThis looks awesome. Mostly nitpicks, and a few code organization questions/remarks.
Which tests are still missing?
Nit: s/false/FALSE/ for improved clarity.
Shouldn't this instead throw an exception? That sounds like an invalid override to me.
If possible, it'd be great if the code in these if-tests could be moved into helper method, that'd make it much easier to follow.
s/libraries-override/libraries overrides/
Nit: "This" fits on the previous line, perhaps even "allows".
s/libraries-override/libraries overrides/
s/base theme/base themes/
Can't you move this into a helper method? It'd make the tests much easier to read.
IIRC these dummy files are not necessary.
s/streamwrappers/stream wrappers/
s/protocol-free/protocol-relative/
Comment #118
almaudoh CreditAttribution: almaudoh commentedThanks for the review @Wim Leers:
1. Fixed
2. This was introduced to allow e.g. to override a script with some version located in another domain. Are you saying this is not a valid use case?
3. How's this?
4. Done.
5. Done.
6. Done.
7. Will do in next patch.
8. See #49.10 where I responded to same query in #48.10
9. Done.
10. Done.
Comment #119
almaudoh CreditAttribution: almaudoh commentedThis patch re-organizes the test per #117.7 to improve readability.
Comment #120
almaudoh CreditAttribution: almaudoh commentedSelf-review: could only find some nits, can be fixed on commit - or the next re-roll.
Extra space...
Period at comment end.
Comment #121
borisson_Fixed the nitpicks and reviewed the code, looks good to me.
Comment #122
joelpittetThis now has tests, no need for the tag:)
Let's get some people who want this feature to manually test to ensure it's doing what they expect!
Screenshots proving you manually tested and it worked would be appreciated.
Comment #123
lauriiiI was trying to override and remove a CSS from both module and theme but couldn't get neither working. Am I doing something wrong? This is what I added to my theme info.yml file:
I'm also very worried about DX/TX of the current solution because it might work a little bit too much with magic. It looks first like a file path but it actually includes the theme/module name and the library name and then it has the path for the CSS file.
Comment #124
almaudoh CreditAttribution: almaudoh commentedThat's what had been proposed right from the issue inception. Do you hav something better?
The first should work. The next two don't looked like they correctly specify existing library assets. (You're right - maybe we need to improve the syntax :))
The issue #2562839: Follow-up to [#2451411]: Add assertions in Library(Discovery)Parser to prevent typos. would help improve TX/DX by throwing exceptions when invalid entries are made.
Can you do another test. I'll so manual testing of my own.
CNW for further reviews / manual testing by others.
Comment #125
davidhernandezWould it be possible to use a different separator? I'm not too familiar with YML conventions.
Something like
theme:library:path/to/css/style.css
?Comment #126
Wim LeersThat'd be inconsistent/confusing too IMHO. Because
theme/library
already is a thing in library dependencies.So, IMHO
theme/library@path/to/css/style.css
would be better, where@
is the separator we'd like to use.Comment #127
davidhernandezWe could also stick to something similar to what we already do in library files, which is just using separate lines?
I'm not entirely sure how the last one would work.
Comment #128
Wim Leers#127++
Comment #129
lauriiiBoth #126 and #127 are great suggestions but #127 sounds a bit better for me
Comment #130
almaudoh CreditAttribution: almaudoh commentedThis still looks magical to non-"insiders". There's still potential to confuse
css/theme/css/layout.css
with an actual filesystem path. So if we're using the same kind syntax to existing libraries.info.yml, how about:Comment #131
star-szrThat sounds pretty sensible to me @almaudoh.
Comment #132
davidhernandezOh I wasn't paying attention. I literally took that to be a system path. What you have in #130 makes sense. That looks to be exactly the same as what we do in the library file.
For example, from the system module:
Just to be clear, "theme" is the SMACSS category. So when overriding the CSS file you would just replicate the same category used by the original library (theme, component, base, layout), correct?
Comment #133
davidhernandezAnd to add, it looks the only different is that you have to specify the theme/module name, but that is because we don't have unique library names. I think that is ok.
Comment #134
Wim LeersThat's not really true. Library names are unique within an extension (module/theme). And global uniqueness is ensured because libraries are namespaced by the extension:
extension/library
.That's why we have this syntax :)
That's why libraries are able to declare dependencies on any other library.
Comment #135
davidhernandezThat's what I'm saying. The library name itself isn't global. We have to reference the extension, which is something we don't have to do when creating the library. I'm just remarking that it is slightly different syntax to creating the library, so we have to make sure it is documented.
Comment #136
Wim LeersRight, fair! Something like
Comment #137
almaudoh CreditAttribution: almaudoh commentedCorrect!
+1
Comment #138
almaudoh CreditAttribution: almaudoh commentedRe-roll
Comment #141
almaudoh CreditAttribution: almaudoh commentedWorking on this now
Comment #142
almaudoh CreditAttribution: almaudoh commentedThis patch implements the syntax in #130, also extends the test coverage. Some interesting things in the course of making these changes:
Expect some test fails as the
LibraryDiscovery
cache invalidation doesn't seem to be working fine inLibraryDiscoveryIntegrationTest
. Will look into that later.Comment #145
lauriiiI was talking with @fabianx and he said that this might be useful tool to help people using libraries properly which is important for BigPipe to work correctly.
Comment #146
almaudoh CreditAttribution: almaudoh commentedIn trying to fix the test fails in #142, I found
LibraryDiscovery::clearCachedDefinitions()
, does not properly clear the static and persistent caches. So raised #2575735: LibraryDiscoveryCollector::reset() does not properly reset its $cid, resulting in loading wrong library assets if the active theme changesComment #147
almaudoh CreditAttribution: almaudoh commentedMoved some of the code around to fix a hidden flaw in the logic.
This implementation is flawed in that a call to
LibraryDiscovery::getLibrariesByExtension()
would miss out on the overrides of entire libraries. And that method is a part of the LibraryDiscoveryInterface. This patch refactors the code to ensure that a call to either public interface methods returns similar results.Also fixed the
LibraryDiscovery::clearCachedDefinitions()
bug so tests here can pass. Will transfer that hunk over to #2575735: LibraryDiscoveryCollector::reset() does not properly reset its $cid, resulting in loading wrong library assets if the active theme changes.Comment #150
almaudoh CreditAttribution: almaudoh commentedRe-roll
Comment #153
davidhernandezDon't give up hope, almaudoh. We're all seeing these updates and rooting you on!
Comment #154
almaudoh CreditAttribution: almaudoh commentedThanks @davidhernandez :). Almost there... Just a minor fix on a unit test.
Comment #157
almaudoh CreditAttribution: almaudoh commentedOops :( ... silly me. While the interdiff in #154 is correct I went and uploaded the exact same patch as in #150. Here's the right patch now, same interdiff, sorry for the noise.
Comment #158
Wim LeersThanks for your hard work, @almaudoh!
Oh, wow, that sounds very bad. Unacceptable, even. #2575735: LibraryDiscoveryCollector::reset() does not properly reset its $cid, resulting in loading wrong library assets if the active theme changes cannot possibly fix that. But are you sure this is the case? The library "registry" already is cached per theme. Go to the front page and then to an admin page, then note how both
library_info:bartik
andlibrary_info:seven
exist in thecache.discovery
cache bin (cache_discovery
DB table).The before vs after here is AFAICT the root cause of the caching problems you're seeing.
Note how
\Drupal\Core\Asset\LibraryDiscovery
in HEAD says this:i.e. the sole reason we have
LibraryDiscovery
on top ofLibraryDiscoveryCollector
, is for these uncacheable, super-dynamic alter hooks. They're only cacheable within a request (and therefore use static caching), and not across requests. For the things that are cacheable across requests, we haveLibraryDiscoveryCollector
.And here's the key thing:
libraries-override
is cacheable across requests, so it should actually live inLibraryDiscoveryCollector
, and not here. That already is cached per theme, and libraries overrides are also per theme, so that will then actually work perfectly :)Incomplete review — I focused on the above part of my comment — reviewing the rest mostly needs people like @davidhernandez and @lauriii.
s/where-in/in which/
s/sub_key/sub key/
Nit: s/Drupal relative/Drupal-relative/
s/regular/absolute/
I understand the addition, by why the removal?
Comment #159
almaudoh CreditAttribution: almaudoh commentedThanks for the review @Wim Leers.
I will do another check, but I think the second point below takes care of that.
Agree completely. In fact, I was thinking of doing that in a follow-up, but we might as well just do that here.
Comment #160
almaudoh CreditAttribution: almaudoh as a volunteer commentedSo this patch moves the libraries-override cleanup code for entire libraries from
LibraryDiscovery
intoLibraryDiscoveryCollector
.Fixed #158 1 - 4. #158.5 was removed because stylesheets-override was removed in #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name. This is just a leftover which we might as well remove in this issue.
Comment #161
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - looks great to me now!
Comment #162
Wim LeersLooks great!
Comment #163
almaudoh CreditAttribution: almaudoh as a volunteer commentedDocs fixes
s/will/with/
Comment no longer valid or needed.
Comment #165
almaudoh CreditAttribution: almaudoh as a volunteer commentedSome of the library files used in tests were moved around in #2566771: [Voltron patch] Move all remaining *.theme.css to Classy.
Comment #166
almaudoh CreditAttribution: almaudoh as a volunteer commentedSorry, patch in #165 is wrong (mixed up some work I'm currently doing on libraries-extend in there). Re-attached correct patch and interdiff.
Comment #168
Wim LeersComment #171
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPatch looks great to me. Ticking some credit boxes.
Comment #173
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think the patch already includes good test coverage and some manual testing of this has already been done. I also think that if manual testing uncovers problems during RC, they can be fixed. If I turn out to be wrong, we might need to revert this, but in the meantime, pushed to 8.0.x! Thanks for the great work on this.
Comment #174
almaudoh CreditAttribution: almaudoh as a volunteer commentedWooooot!!! Thanks.
Updated change record to match new syntax - https://www.drupal.org/node/2497313/revisions/view/8951259/8951961
This now unblocks #2497667: Add libraries-extend to themes' *.info.yml which already has a green patch. Hope we can get that in also before RC1.
Comment #175
davidhernandezVery awesome work by you, almaudoh. Thank you.
Comment #177
hass CreditAttribution: hass commentedThis patch has a bug, see #2642046: libraries-override does not update drupalSettings libraries array.
Comment #178
hass CreditAttribution: hass commentedAnd one more issue #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides
Comment #179
star-szr@hass something not acting exactly as you hoped is not always a bug. "suxxx" (in bold too) is not acceptable or respectful. Cut that out.
Comment #180
hass CreditAttribution: hass commentedNot being able to use "@classy/..." to wipe out conflicting css files is clearly a critical issue. You cannot deliver such contrib themes as they break or are broken out of the box just because of a path. This may be seen as feature, but it is more a critical bug.
Well - i may was a bit harsh after several hours of continued failures without any light. Every day with theaming in D8 is really hard as I find bugs and bugs. I'm close to 5 or more core patches and several unsolvable issues just to get one theme working properly. And if I count how often I need to presss clear all caches? 10.000 times? I have cache backend null enabled, twig and all other caches disabled, uninstalled the core cache modules, but it is not working...
Do you think this is theming fun? I do not. My current D8 theming frustration level is very high.