Add a comment to RTL and minified files for new contributors

[SergeyBiryukov]

Can we add a small header to RTL and minified files explaining that they are autogenerated and should not be patched directly? This might reduce some confusion for new contributors.

[netweb]

In bbPress we add a 'banner' to the each minified CSS and JavaScript file by adding a 'banner' option to the associated Grunt task which results in

e.g. /*! bbpress - v2.6.0 - 2014-12-07 8:43:33 PM UTC - https://wordpress.org/plugins/bbpress/ */

• Minified LTR CSS
  • Source: ​https://bbpress.trac.wordpress.org/browser/trunk/Gruntfile.js#L134
  • Result: ​https://plugins.trac.wordpress.org/browser/bbpress/trunk/includes/admin/css/admin.min.css
• Minified RTL CSS
  • Source: ​https://bbpress.trac.wordpress.org/browser/trunk/Gruntfile.js#L145
  • Result: ​https://plugins.trac.wordpress.org/browser/bbpress/trunk/includes/admin/css/admin-rtl.min.css
• Minified JS
  • Source: ​https://bbpress.trac.wordpress.org/browser/trunk/Gruntfile.js#L267
  • Result: ​https://plugins.trac.wordpress.org/browser/bbpress/trunk/includes/admin/js/common.min.js

Any specific verbiage request for the patch? Should we include WordPress version? Timestamp? Links?

[kpdesign]

What about:

/*! WordPress 4.1.0 - auto-generated on 2014-12-07 8:43:33 PM UTC from filename.ext - please do not patch this file */

[azaozz]

Replying to netweb:
Replying to kpdesign:

That banner should be "static"/hard-coded. Including the date/time in there means the minified file will always change, even when there are no code changes. This triggers unneeded merges/updates downstream.

We had to change that for the concatenated TinyMCE: https://core.trac.wordpress.org/browser/trunk/Gruntfile.js#L376.

[netweb]

Replying to azaozz:

That banner should be "static"/hard-coded. Including the date/time in there means the minified file will always change, even when there are no code changes. This triggers unneeded merges/updates downstream.

The three Grunt tasks we'd look to do this with, cssmin, cssjanus, and uglify only compile files into the /build folder, they never touch files in /src.

The WordPress packages (nightly, latest etc) are built using the Grunt tools but I don't believe the /build folder for any of these packages are version controlled, correct me here if I'm wrong. ;)

We could also omit the date/time and just include the version and verbiage requests:
/*! This file was automatically generated as part of the WordPress 4.1.0 release, please see https://make.wordpress.org/core/handbook/... for details on creating patches relating to this file. */

[azaozz]

I don't believe the /build folder for any of these packages are version controlled

What about ​https://core.svn.wordpress.org and big sites that use svn/git for deployment. Also that will (probably) trigger new nightly builds even if nothing was changed/committed.

Thinking a short message would be best, not even sure it needs ...as part of the WordPress 4.1.0 release or - v2.6.0 - for bbPress.

[netweb]

Replying to azaozz:

I don't believe the /build folder for any of these packages are version controlled

What about ​https://core.svn.wordpress.org and big sites that use svn/git for deployment. Also that will (probably) trigger new nightly builds even if nothing was changed/committed.

Ha, there it is ;)

Thinking a short message would be best, not even sure it needs ...as part of the WordPress 4.1.0 release or - v2.6.0 - for bbPress.

I'll think about the bbPress side for a bit (it works as simple plugin version checker for people in the support forums ;))

/*! This is an automatically generated file. */
We could add a link to a handbook page or not, happy for others decisions on this ;)

[kpdesign]

What about:

/*! This file is auto-generated - please do not patch. */

[valendesigns]

Patch 30666.diff​ adds a banner to auto generated CSS & JS files with grunt-banner
​https://github.com/mattstyles/grunt-banner.

One of the reasons I went with this option over the way bbPress adds banners is because grunt-contrib-cssmin has removed banner support in v0.11.0 and would leave us without an upgrade path.
​https://github.com/gruntjs/grunt-contrib-cssmin/commit/985b3d8cb60dd45ff9eaa472662ebea69d3b5eb0

Cheers,
Derek

[netweb]

Thanks Derek, as grunt-contrib-cssmin is removing banner support I think this is the best way forward, I'd not be surprised if grunt-contrib-uglify followed suit in the not to distant future and also removed banner support.

A couple of issues on includes and excludes:

• usebanner:cssjanus looks good
• usebanner:cssmin should only add a banner to the files we minify the /wp-admin files listed here, /wp-includes looks good
• usebanner:uglify is adding a banner to wp-includes/js/zxcvbn.min.js, your excluding the exclusion ;)
• usebanner:uglify should exclude adding banners to TinyMCE JavaScript files, then after concat:tinymce has run, add a single banner to the concatanated build/wp-includes/js/tinymce/wp-tinymce.js` file (Here's what it currently looks like ​https://cloudup.com/cm8rM4N1lae)

[valendesigns]

I will get those changes made and send it back shortly.

[valendesigns]

@netweb - Are the color files in usebanner:cssmin ok?

[netweb]

Replying to valendesigns:

@netweb - Are the color files in usebanner:cssmin ok?

The three you'd expect colors.min.css, colors-rtl.css and colors-rtl.min.css are fine.

That said, each of the four CSS files for each color scheme file are automatically generated by the build tools sourced from each of the colors.scss files, starting with

• sass:colors generates each colors.css file
• cssjanus:colors generates each colors-rtl.css file from the colors.css file
• cssmin:colors generates both colors.min.css and colors-rtl.min.css from their respective non-minified files

So we should add a banner for each of the colors.css files also :)

[valendesigns]

Should be sorted now. Cheers!

[valendesigns]

I've refreshed the patch, since the 7 week old one didn't apply cleanly.

[DrewAPicture]

refresh

[DrewAPicture]

30666.3.diff​ looks good.

[nacin]

This duplicates a lot of file paths across tasks. I think there are some things we can do to make this a bit simpler. I normally wouldn't slow down a ticket, but this is a pretty big increase to the maintenance costs of this file.

I would remove "please do not patch" entirely. Someone looking at this file is not likely to be a potential contributor. We shouldn't be implying that you can hack away at the non-auto-gen files as a user. Instead, I'd just go for something simple like /*! This file is automatically generated. */. If a contributor can't figure out to not patch said file, then I'm not really sure how many more cues we could give them.

When these files are sucked into load-scripts.php and load-styles.php, I surmise there will be repeated "This file is auto generated" throughout. Probably not ideal, though matters little.

[valendesigns]

@nacin I can work on another patch to update the text and and simplify the maintenance aspect by not duplicate the paths. Do you have any suggestions on how you want that to look before I start?

[netweb]

@valendesigns I just read the logs from last night....

Two things:

• cssjanus has been replaced with rtlcss via #31332
• We need to ensure the Grunt watch tasks still operate as expected which hadn't previously been included included in past patches.

[jorbin]

I agree with Nacin's comments about both the string and the repetative paths. We should try and reuse filepaths (perhaps going so far as to create a generic file path grouping object so that all file paths are kept in a single place). Simplifying this is beyond trivial, so let's punt it until the next release and we can work on it then.

[DrewAPicture]

@jorbin: Mind taking a second look at whether we want to try to come up with a solution here?

[netweb]

I've got a work-in-progress patch for this, it's piggy backing on top of PostCSS using ​PostCSS Banner, it needs a bit more work and tweaks for paths but the primary issue as it stands is the watch task integration, once I finish up #29792 I'm more than happy to finish this off if no one beats me to it.

#!diff
diff --git Gruntfile.js Gruntfile.js
index 25069f9..7157716 100644
--- Gruntfile.js
+++ Gruntfile.js
@@ -45,6 +45,20 @@ module.exports = function(grunt) {
 				src: [
 					'wp-admin/css/colors/*/colors.css'
 				]
+			},
+			banner: {
+				options: {
+					processors: [
+						require('postcss-banner')({banner: '! This file is automatically generated. '})
+					]
+				},
+				cwd: BUILD_DIR,
+				dest: BUILD_DIR,
+				ext: '.min.css',
+				src: [
+					'wp-admin/css/{<%= cssmin.options["wp-admin"] %>}.css',
+					'wp-includes/css/*.css'
+				]
 			}
 		},
 		clean: {
diff --git package.json package.json
index a2a8fd6..a17d9d8 100644
--- package.json
+++ package.json
@@ -29,6 +29,7 @@
     "grunt-postcss": "~0.5.4",
     "grunt-rtlcss": "~1.6.0",
     "grunt-sass": "~1.0.0",
-    "matchdep": "~0.3.0"
+    "matchdep": "~0.3.0",
+    "postcss-banner": "~1.0.0"
   }
 }

[jorbin]

@netweb's solution is on the right track. We need to make sure it covers the following files:

1) All -rtl files
2) All colors files
3) all .min.css files.

As far as wording, I think "This file is build generated" makes a bit more sense than "Automatically generated". Super minor, but it helps people realize that WordPress uses a build system.

[DrewAPicture]

@netweb Care to followup on @jorbin's feedback in comment:30 here?

[DrewAPicture]

@netweb Sending a second ping for a followup on @jorbin's feedback :-)

[jorbin]

In 41271:

Build/Test Tools: Add banner to minified CSS files

Patches occasionally come in on generated files. We should be kind to new contributors and give them a hint that these files are auto generated.

Props drewapicture, samuelsidler, netweb, valendesigns, kpdesign, nacin, jorbin
Fixes #30666

[swissspidy]

In 41277:

Build/Test Tools: Remove unneeded commas to fix JSHint error introduced
in [41271].

See #30666.

[SergeyBiryukov]

In 46589:

Build/Test Tools: Add banner to RTL CSS and minified JS files.

Patches occasionally come in on generated files. We should be kind to new contributors and give them a hint that these files are auto-generated.

This is a follow-up to [41271], which added the banner to minified CSS files.

Fixes #48424. See #30666.