Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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).
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff-2203411-43.txt | 1.35 KB | damiankloip |
#43 | 2203411-43.patch | 49.33 KB | damiankloip |
#24 | library_discovery-2203411-24.patch | 49.48 KB | dawehner |
#22 | interdiff.txt | 8.96 KB | dawehner |
#22 | library-2203411-22.patch | 50.72 KB | dawehner |
Comments
Comment #1
sunComment #2
dawehnerLet's try it.
Comment #3
dawehnerLet's post the work of tonight, there are not unit tests yet.
Comment #5
dawehnerThis should cover at least somehow the basic cases for this service.
Comment #7
dawehnerups.
Comment #9
sun7: library_discovery-2203411-7.patch queued for re-testing.
Comment #11
dawehnerThe last failure seemed to be random.
Comment #13
dawehnerembarrassing, there is nothing else to describe it.
Comment #14
dawehner.
Comment #15
dawehner13: library_discovery-2203411-13.patch queued for re-testing.
Comment #16
Wim Leers:)
Missing docs.
HURRAY! Explicit failure!
Missing description.
Shouldn't we have a more specific exception for this?
Another specific exception would be nice?
This appears to be dead code?
Great test!
This looks bizarre… If this is intentional, it could definitely use a comment.
Comment #17
dawehnerThank you for the review and all the grammar/case sensitivity fixes.
Okay, here are some:
People started to not put any pointless description onto phpunit tests.
This is used by the test to mock its external dependencies.
Do we have a better syntax for such an subarray? The idea is (at least for now), to support both the "external/example_external" and the other syntax. Maybe we could already drop the functionality in this issue.
Came up with two custom exceptions. I am not sure about the remaining UnexpectedValueException one, as it provides somehow enough context already.
Comment #18
damiankloip CreditAttribution: damiankloip commentedI like these.
Seems like a naming mismatch. Can we just go with setCache/getCache. This is more inline with naming elsewhere IMO.
Nice. This will be a good tidy up. Is there an issue for it yet?
Not for this issue at all. Would be nice to have some non procedural file stuff. Seems a long way off though :/
Comment #19
dawehnerNot sure, as I just copy and pasted the stuff.
Wait, really? I guess at least for drupal_get_path() we will have a solution with the extension subsystem.
Comment #20
dawehnerThank you for the review damian! Sadly the interdiff failed :(
Comment #21
tim.plunkettAll of this except the final else (with $path . '/' . $source) are not covered by the unit tests.
Do we care about adding some extra tests for this?
Oh that's way smarter than separate !defined checks, +1
I love the usage of @covers here, but am sad about missing it on the rest.
Comment #22
dawehnerAdded some to the rest of them.
Let's just do it.
Comment #23
sun#2203407: Replace #attached library array values with provider-namespaced strings has landed, so this needs a re-roll now.
Note that the former support/handling of
array('provider', 'lib-name')
has been removed entirely by that patch; it's alwaysprovider/lib-name
now.Comment #24
dawehnerRerolled. The interdiff are the changes after the rerole.
Comment #26
dawehnerLet's see
Comment #28
dawehnerUnit tests are not everything.
Comment #29
dawehnerChange-notice: https://drupal.org/node/2216591
Comment #30
tim.plunkettAwesome work!
Comment #32
Wim Leers#2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses broke this.
Reroll + fix super minor nitpicks (for which there is an interdiff).
Comment #33
dawehnerThank you for the rerole and the nitpicks!
Are you sure this still passes? I remember that I needed the tab to get a failure in the unit test.
Comment #35
dawehnerLet's go with a slightly simpler example.
Comment #36
dawehnerback to RTBC, this will pass.
Comment #37
Wim Leers#33: sorry, git complained, I figured it was the actual value that was going to trigger a fail, apparently it wasn't :( Thanks for the simpler example in #35!
RTBC +1
Comment #38
moshe weitzman CreditAttribution: moshe weitzman commentedThis helps get rid of several cache gets. Up priority.
Comment #39
damiankloip CreditAttribution: damiankloip commentedComment #40
catchStarted reviewing this but I'm almost out of battery so posting what I have so far (which wasn't far in)..
ThemeHandler isn't documented as a property, isn't set in __construct(), and never gets used.
Comment #41
catchStill partial review, but:
Themes can implement hook_library_info_alter(), but the current theme isn't taken into account in the cache key. Looks like a pre-existing problem though.
Comment #42
damiankloip CreditAttribution: damiankloip commented#41, yeah, if anything I think we could roll that into the work we will do after this patch in #2223143: Consolidate library extension caches into a single cache entry?
Comment #43
damiankloip CreditAttribution: damiankloip commentedRemoved the theme handler from the LibraryDiscovery constructor.
Comment #44
jibranHaven't found anything to object. Tests are there to make sure every single scenario is tested so I think it is good to go RTBC
Comment #45
dawehnerReally good catch!
Comment #47
damiankloip CreditAttribution: damiankloip commented43: 2203411-43.patch queued for re-testing.
Comment #48
jibran:/
Comment #49
dawehnerMeh!
Comment #50
catchCommitted/pushed to 8.x, thanks!
Comment #52
catchThanks!
Comment #53
damiankloip CreditAttribution: damiankloip commentedYes, nice work sun, dawehner!