Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The docblock on \Drupal\Core\Asset\CssOptimizer::processFile both is totally inaccurate (ie that function does NOT do what the docblock says) and also missing param and return information.
Steps to reproduce
Proposed resolution
Update the documentation
Add a followup to describe a css asset
Remaining tasks
#15. A small change to the parameter description and a followup. It has been a while since the request for a followup it would be best to search to see if this has been fixed elsewhere.
Comment | File | Size | Author |
---|---|---|---|
#23 | 2612876-20-22.patch | 902 bytes | victoria-marina |
Comments
Comment #2
NickDickinsonWildeComment #3
RobLoachYup.
Comment #4
xjmThanks @NickWilde!
It would be good to provide a reference here that describes the structure of the asset array (e.g. a reference to another class/method/etc.).
Comment #5
NickDickinsonWilde@xjm: All of the other related classes/methods (that I've looked at anyways) just say something like:
array $asset: An asset.
Much to my surprise I haven't been able to find a basic minimal example array anywhere - best reference I found was the parser class's build method, and that doesn't have it detailed in the docstring but rather throughout the build process. So if I do something here should I also do something to all the other classes with a similar lack of helpful information (and if so keep that in this or as seperate per file patches)?
Comment #6
NickDickinsonWildefurther investigation, has revealed drupal_js_defaults() but no equivalent function for css assets.
Comment #7
magi.yv CreditAttribution: magi.yv at Zyxware Technologies commentedIt seems the array contain 3 members type, data and pre-process. Type is about type of data it should be file. Data is a file path. Preprocess is a Boolean value which define pre-processing enabled or not.
It is not documented anywhere else. So how could I proceed?
Comment #11
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedRerolled the last path for D8.3.x . I thing current description is sufficient for this method.
Comment #14
borisson_#4 was not fixed, but the reason why this is not fixed was explained in #5.
Comment #15
alexpott@borisson_ I think @xjm is correct we
I think we should file a followup to properly document what an asset array contains (it is slightly different for JS and CSS) but in this issue we should at least mention the property we use in the processFile method - which is the 'data' key. Which has to be the path to the CSS file for the code to work.
Comment #19
quietone CreditAttribution: quietone as a volunteer commentedStill valid for 9.4.x.
Suitable for a novice.
Comment #20
leolandotan CreditAttribution: leolandotan at Promet Source commentedI have tried to add the detail @alexpott mentioned which is to "at least mention the property we use in the processFile method - which is the 'data' key. Which has to be the path to the CSS file for the code to work.". Hope this is alright.
Comment #21
anagomes CreditAttribution: anagomes at CI&T commentedThe patch in #20 applies perfectly and addresses what was mentioned in #15, it seems good enough for me. I'm only moving this to needs work due to a minor typo, in
+ * Processes CSS file and add base URLs to any relative resource paths.
, - I'm not a native english speaker so I may be wrong - I believe a letter 's' is missing at the end of 'add', it should be 'adds'.Comment #22
victoria-marina CreditAttribution: victoria-marina at CI&T commented@anagomes I agree with you. Working on that.
Comment #23
victoria-marina CreditAttribution: victoria-marina at CI&T commentedI upgraded the last patch with the correction typo.
Comment #24
victoria-marina CreditAttribution: victoria-marina at CI&T commentedComment #25
anagomes CreditAttribution: anagomes at CI&T commentedPatch passed all the tests and applies correctly, with the change I had previously mentioned. Now it is RTBC to me!
@victoria-marina in this case, this is not a big problem, because this is a small patch. But for a next time, it's good for you to know that when you make a patch that's based on another patch, it's a good practice to provide an interdiff.
Comment #26
alexpottCommitted and pushed 31e0876662c to 10.0.x and 5bff9c8e847 to 9.4.x and fa4a632fbf5 to 9.3.x. Thanks!