WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#30666 closed enhancement (fixed)

Add a comment to RTL and minified files for new contributors

Reported by: SergeyBiryukov Owned by: jorbin
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-patch
Focuses: docs Cc:
PR Number:

Description

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.

Attachments (4)

30666.diff (3.0 KB) - added by valendesigns 5 years ago.
30666.1.diff (3.3 KB) - added by valendesigns 5 years ago.
30666.2.diff (3.2 KB) - added by valendesigns 5 years ago.
30666.3.diff (3.3 KB) - added by DrewAPicture 5 years ago.
refresh

Download all attachments as: .zip

Change History (39)

#1 @SergeyBiryukov
5 years ago

  • Type changed from defect (bug) to enhancement

#2 @netweb
5 years ago

  • Keywords needs-patch added

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/ */

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

#3 @SergeyBiryukov
5 years ago

  • Focuses docs added

#4 follow-up: @kpdesign
5 years ago

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 */

#5 in reply to: ↑ 4 ; follow-up: @azaozz
5 years ago

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.

#6 in reply to: ↑ 5 @netweb
5 years ago

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. */

#7 follow-up: @azaozz
5 years ago

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.

Last edited 5 years ago by azaozz (previous) (diff)

#8 in reply to: ↑ 7 @netweb
5 years ago

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

#9 @kpdesign
5 years ago

What about:

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

@valendesigns
5 years ago

#10 @valendesigns
5 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

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

#11 @netweb
5 years ago

  • Keywords needs-refresh added; has-patch removed

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)

#12 @valendesigns
5 years ago

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

#13 follow-up: @valendesigns
5 years ago

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

#14 in reply to: ↑ 13 @netweb
5 years ago

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
5 years ago

#15 @valendesigns
5 years ago

Should be sorted now. Cheers!

@valendesigns
5 years ago

#16 @valendesigns
5 years ago

  • Keywords has-patch added; needs-refresh removed

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

#17 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 4.2

@DrewAPicture
5 years ago

refresh

#18 @DrewAPicture
5 years ago

  • Keywords commit added

30666.3.diff looks good.

#19 @nacin
5 years ago

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.

#20 @valendesigns
5 years ago

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

#21 @DrewAPicture
5 years ago

  • Keywords commit removed

This ticket was mentioned in Slack in #bbpress by netweb. View the logs.


5 years ago

This ticket was mentioned in Slack in #core by drew. View the logs.


5 years ago

#24 @DrewAPicture
5 years ago

  • Keywords needs-patch added; dev-feedback has-patch removed

#25 @netweb
5 years ago

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

This ticket was mentioned in Slack in #core by drew. View the logs.


5 years ago

#27 @jorbin
5 years ago

  • Milestone changed from 4.2 to Future Release

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.

#28 @DrewAPicture
4 years ago

  • Owner set to jorbin
  • Status changed from new to assigned

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

#29 @netweb
4 years ago

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"
   }
 }

#30 @jorbin
4 years ago

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

#31 @DrewAPicture
3 years ago

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

#32 @DrewAPicture
2 years ago

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

#33 @jorbin
2 years ago

  • Milestone changed from Future Release to 4.9

#34 @jorbin
2 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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

#35 @swissspidy
2 years ago

In 41277:

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

See #30666.

Note: See TracTickets for help on using tickets.