Last modified: 2014-09-20 21:11:13 UTC
I think it was a design flaw that we went ahead with implementing php-land functions at all. It was a questionable experiment, but we know better now and I think we should admit that and phase it out ASAP. This brings three main problems: 1) Decreased compatibility; (can't easily compile it with lessjs without porting the functions over and keeping in sync). 2) Extra maintenance; (when we switch lessphp implementations or upgrade, these will need updating). 3) Separation of concerns and flow control; (embedding is supposed to be part of CSSMin and run *after* CSSJanus; doing it in LESS before it even turns to CSS disrupts this). Problem #3 causes bugs like bug 66091, where in Less files certain things don't get flipped properly by CSSJanus because it jumps the gun on mapping things to files on disk and reading them. I think we should deprecate the less embed() function and instead ensure the /* @embed */. This provides many benefits: * Reduced duplication. * Less output is easier to debug with (no embedded data uris[1]). * No maintenance overhead (and bugs like bug 66091) by having to keep its behaviour in sync with non-LESS files with regards to CSSJanus flipping. * Keeps minification restricted to CSSMin (which runs anyway, the only way it makes sense to start minifying or reading files from disk earlier is if we somehow phase out CSSMin entirely, which would be an interesting adventure of its own)
Note that OOjs UI, for example, as being a standalone project, never used the 'embed()' function because we use its compiled distribution file which is already CSS (compiled by lessjs via Grunt). And yet it has no impact on its performance because /* @embed */ degrade gracefully for third parties and is still picked by CSSMin when MediaWIki loads oojs-ui.css. This is not a coincidence, CSSMin was designed this way on purpose and Less kind of broke that paradigm.
This loosely depends on bug 54673. I don't think we have to block on it (it mostly works already) but there's weird edge cases where Less messes up the comments and those are hard to detect so ideally it would "just work" before we give that up.
(In reply to Krinkle from comment #2) > This loosely depends on bug 54673. I don't think we have to block on it (it > mostly works already) but there's weird edge cases where Less messes up the > comments and those are hard to detect so ideally it would "just work" before > we give that up. My understanding is those weird edge cases are exactly why the custom functions were introduced, so it does sound like a blocker to me.
Change 161245 had a related patch set uploaded by Bartosz Dziewoński: Fix CSSJanus flipping in LESS mixins and remove broken custom LESS functions https://gerrit.wikimedia.org/r/161245
Change 161751 had a related patch set uploaded by Krinkle: Fix CSSJanus flipping in LESS mixins and remove broken custom LESS functions https://gerrit.wikimedia.org/r/161751
Change 161245 merged by jenkins-bot: Fix CSSJanus flipping in LESS mixins and remove broken custom LESS functions https://gerrit.wikimedia.org/r/161245
Change 161751 merged by jenkins-bot: Fix CSSJanus flipping in LESS mixins and remove broken custom LESS functions https://gerrit.wikimedia.org/r/161751
