Problem/Motivation
On #3060153: Use PostCSS in core, initially only for Claro PostCSS capability was added to Core.
The generated CSS files get this header in them:
/*
* DO NOT EDIT THIS FILE.
* See the following change record for more information,
* https://www.drupal.org/node/2815083
* @preserve
*/
The link in that header goes to this change record about JavaScript compiling:
https://www.drupal.org/node/2815083
It should instead go to the change record about PostCSS:
https://www.drupal.org/node/3084859
Proposed resolution
Novice task!
Generated CSS files should have the correct change record URL in their headers. Looking at the patch for the parent issue, it appears that these headers are generated from scripts, and the header source is included in two places:
core/scripts/css/compile.js
core/postcss.config.js
So, both of them need a 1-line change to fix the URL.
The other thing that needs to be done is to fix the headers in the existing CSS file output under the Claro theme (the only one in Core currently using PostCSS I think) . There are several ways this could be done:
a) The correct way is to follow the compiling procedure in the change record: https://www.drupal.org/node/3084859
b) The tedious and technically not correct way is to edit all of the .css files that do not have ".pcss" in their file names under core/themes/claro/css, and fix the URLs in their headers.
c) The less tedious and also technically incorrect way to do it would be to write a script that would replace the URLs.
I'm pretty sure the patch will end up the same any way it is done, but definitely (a) is the right way to do it if possible.
Remaining tasks
1. Make a patch as noted in Proposed resolution.
2. Review and commit the patch.
User interface changes
None.
API changes
None, but compiled CSS output from PostCSS input will be slightly different.
Data model changes
None.
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#11 | core_claro_PostCSS-notice_fix-3155159-11.patch | 38.36 KB | laura.gates |
Comments
Comment #2
jhodgdonAdding some notes to the summary.
Comment #3
Kristen PolTagging this issue for new contributors to work on during the Friday DrupalCon Global sprint.
Comment #4
qjac CreditAttribution: qjac commentedI'm working on it! DrupalCon 202 Contrib Day
Comment #5
laura.gatesI'll give this one a go for Global2020 :)
Comment #6
laura.gatesOkay, "fingers-crossed" that this patch works.
Comment #7
jhodgdonThanks for the patch! A few thoughts:
a) When you upload a patch, set the issue status (under Issue metadata) to "Needs review". The needed tests will automatically launch. You don't have to add the tests manually, and we don't usually launch that many tests on one issue unless it is specifically needed. Setting to "Needs review" also alerts human reviewers that the patch is ready to be reviewed.
b) When I tried to apply the patch, I had to switch to the "core" directory to apply it. Always make patches using "git diff", which should make the patch relative to the Drupal root git directory instead of a subdirectory.
c) Your patch file didn't apply. If you click through to the test results, it says:
fatal: corrupt patch at line 494
I get the same result at the command line... Here is line 494:
I am not sure what is going on... the file looks OK to me but there must be some weird character in the file. Maybe try making the patch again or re-uploading it?
Comment #8
laura.gatesI rerolled the patch trying a strait
git clone
for the dev branch rather than doingcomposer create-project drupal/recommended-project my_site_name_dir
Comment #9
laura.gatesComment #10
jhodgdonThanks for the new patch! It seems to be correct for the CSS files, even though if you click through to the coding standards messages, you find that it generates 145 new coding standards messages. I have no idea why that would be.
That aside, the patch does not have the fixes for
core/scripts/css/compile.js
core/postcss.config.js
as mentioned in the issue summary. It only contains the fixes for the Claro output files.
Comment #11
laura.gatesAh shoot, I must have forgot to hit save.
Hopefully, third time is the charm!
Comment #12
laura.gatesComment #13
Kristen Pol@laura.gates Thanks for the patches. It's helpful to have an interdiff included when patches are changed so we can see the changes easier: https://www.drupal.org/documentation/git/interdiff
Comment #14
laura.gates@Kristen,
I think the issues I was having with the other two patches was from not using command line to save the patch.
Comment #15
jhodgdonWe're not worried about why the patches weren't correct... you are learning! :) Still, an interdiff file is still very helpful for reviewers, especially when the new patch is presumably fairly similar to the previous one with just a few small changes.
Comment #16
Kristen PolMarking RTBC since:
1) Patch applies cleanly.
2) Tests pass.
3) Patch changes
2815083
to3084859
as described in the issue summary.4) After applying the patch and searching for
2815083
, the files found are all JavaScript files which is correct.5) Searched for
3084859
and the files found are all CSS files in Claro pluscore/scripts/css/compile.js
andcore/postcss.config.js
which looks correct.6) Searched for Claro CSS files that do NOT have
3084859
and they are either thepcss.css
files or the following CSS files that don't have PCSS equivalents:Comment #17
alexpottCommitted and pushed 1ba1cd8b78 to 9.1.x and d052178664 to 9.0.x. Thanks!
Backported because this is a docs fix
Comment #21
Chi CreditAttribution: Chi commentedGive that's a bug why wasn't it backported to 8.9.x?