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.

CommentFileSizeAuthor
#23 2612876-20-22.patch902 bytesvictoria-marina
#20 2612876-20.patch901 bytesleolandotan
#20 interdiff-2612876-11-20.txt761 bytesleolandotan
#11 2612876-11.patch661 bytesgaurav.kapoor
#2 fix-2612876-2.patch655 bytesNickDickinsonWilde
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NickWilde created an issue. See original summary.

NickDickinsonWilde’s picture

NickDickinsonWilde
He/Him/His
Primary language English
Location Victoria, BC (T'So-uke lands)
CreditAttribution: NickDickinsonWilde commented
Assigned: NickDickinsonWilde » Unassigned
Status: Needs work » Needs review
Issue tags: +rc target triage
FileSize
fix-2612876-2.patch655 bytes
RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Yup.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: Fix \Drupal\Core\Asset\CssOptimizer::processFile docblock » Fix \Drupal\Core\Asset\CssOptimizer::processFile() docblock
Status: Reviewed & tested by the community » Needs work
Issue tags: -rc target triage

Thanks @NickWilde!

+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -53,7 +53,13 @@ public function clean($contents) {
+   *   A CSS asset.

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

NickDickinsonWilde’s picture

NickDickinsonWilde
He/Him/His
Primary language English
Location Victoria, BC (T'So-uke lands)
CreditAttribution: NickDickinsonWilde commented

@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)?

NickDickinsonWilde’s picture

NickDickinsonWilde
He/Him/His
Primary language English
Location Victoria, BC (T'So-uke lands)
CreditAttribution: NickDickinsonWilde commented

further investigation, has revealed drupal_js_defaults() but no equivalent function for css assets.

magi.yv’s picture

magi.yv CreditAttribution: magi.yv at Zyxware Technologies commented

It 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?

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
2612876-11.patch661 bytes

Rerolled the last path for D8.3.x . I thing current description is sufficient for this method.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

#4 was not fixed, but the reason why this is not fixed was explained in #5.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Needs work

@borisson_ I think @xjm is correct we 

+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -48,7 +48,13 @@ public function clean($contents) {
+   * @param array $css_asset
+   *   A CSS asset.

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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Version: 8.9.x-dev » 9.4.x-dev
Issue summary: View changes
Issue tags: +Bug Smash Initiative, +Novice

Still valid for 9.4.x.

Suitable for a novice.

leolandotan’s picture

leolandotan CreditAttribution: leolandotan at Promet Source commented
Status: Needs work » Needs review
FileSize
interdiff-2612876-11-20.txt761 bytes
2612876-20.patch901 bytes

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

anagomes’s picture

anagomes CreditAttribution: anagomes at CI&T commented
Status: Needs review » Needs work

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

victoria-marina’s picture

victoria-marina CreditAttribution: victoria-marina at CI&T commented
Assigned: Unassigned » victoria-marina

@anagomes I agree with you. Working on that.

victoria-marina’s picture

victoria-marina CreditAttribution: victoria-marina at CI&T commented
FileSize
2612876-20-22.patch902 bytes

I upgraded the last patch with the correction typo.

victoria-marina’s picture

victoria-marina CreditAttribution: victoria-marina at CI&T commented
Assigned: victoria-marina » Unassigned
Status: Needs work » Needs review
anagomes’s picture

anagomes CreditAttribution: anagomes at CI&T commented
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 31e0876662c to 10.0.x and 5bff9c8e847 to 9.4.x and fa4a632fbf5 to 9.3.x. Thanks!

  • alexpott committed 31e0876 on 10.0.x 
    Issue #2612876 by leolandotan, NickDickinsonWilde, victoria-marina,...

  • alexpott committed 5bff9c8 on 9.4.x 
    Issue #2612876 by leolandotan, NickDickinsonWilde, victoria-marina,...

  • alexpott committed fa4a632 on 9.3.x 
    Issue #2612876 by leolandotan, NickDickinsonWilde, victoria-marina,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.