Fix CSS Aggregator

subscribing - I'll take a look.

This looks like great work, and very valuable. I haven't reviewed the code yet.

See also #494498: Add CSS parsing capability to Drupal.

IMO, really valid aggregation AND actually valid tests will be based on parsing and determining that the resultant parse tree is the same.

~

There seem to be a lot of changes rolled in here. Some might be bug fixes and some are definitely new features. D7 is past the new features stage and now only bug fixes can go in. Plus, this definitely makes changes to the API and that is frozen as well.

I'd suggest a few things.

First, break the patch up into bug fixes and new features. The new features should go in an issue for Drupal 8 and the bug fixes can stay in an issue for Drupal 7.

Second, per Webchick advice, mega patches that change a bunch of things are bad. She would rather deal with individual patches per thing rather than one patch that does a lot of things. This makes it easier to review and get committed.

Third, share and promote your CSS Preprocessor module in contrib during D7. I'm curious to see how it goes.

For Drupal 8 I'd like to make both the CSS and JS preprocessors pluggable. So, we can swap them out with others from contrib. Some people will use something like YUI Compressor to shrink down CSS and not take the runtime performance hit. Others, could take advantage of what you do here. And, still others just want something simple like what core offers now. What do you think of that?

In past issues we've not been able to get core to do anything complex when it comes to shrinking this stuff down. In the JS preprocessing patch it went in initially and was later removed.

The CSS and JS preprocessors are not pluggable in core. hook_js_alter and hook_css_alter allow you access to the original files but that does not provide the ability to swap out the preprocessor itself. For example, see http://drupal.org/project/sf_cache. This module allows for multiple preprocessed files in groupings. You would need a pluggable system to support the groupings. Something like #352951: Make JS & CSS Preprocessing Pluggable. This is a good place for a plugin (swappable subsystem) not a module.

When it comes to having a more complex parser 2 things concern me. First, is there a project already doing this we can leverage so we don't duplicate code? Second, slow performance of a more complex parser is why tools like YUI say not to use them in setups like this. So, what is performance like? What is the performance change to drupal core?

You said, "* all inline css is merged into one

@casey: It looks to me like you're still adding new features ('preprocess', 'aggregate'), which definitely won't fly.

Please confine this to bug fixes, and make sure (if you haven't already) to label the tests as to what bug they test.

For this version of the patch, it would be nice to have comments indicating which bug is fixed by what change as well.

Thanks for detailing out the issues with @import. That's the kind of information we need here.

The patch does a lot more than change how @import is handled. This is a mega patch addressing multiple issues. Keeping it that was isn't going to help get the patch committed. I've learned this the hard way. Each desecrate issue needs to be broken out separately. Even if it is easier for you to write it that way.

This patch changes APIs and we are past the API freeze. A whole new way to handle this that changes internal APIs will need to wait for D8. That's the point in the process we are at. If you want this in D7 i'd suggest changing this to multiple issues that address the individual bugs and do them one after another. Otherwise, this will become a D8 issue and the D8 maintainers can sort it out.

What version of PHP are you using? 5.2.x?

I'm afraid that a large, disruptive patch like this won't fly. I skimmed it and came away with the impression that "here's a bunch of code that will fix what ails you". But no clear justification for the things you're doing. I'm sure you have a clear vision, but the community process means that you have to sell the rest of us on your vision.

Also, it's clear that you're changing APIs if you had to change color.module. That's clearly past the code freeze.

Here's what you need to do, IMO, to continue successfully with this:

1. Explain in this issue what problems you're trying to solve.
2. Explain in this issue exactly how you propose to solve them.
3. Explain in this issue how the tests that you've provided can give us adequate confidence that nothing else will break as a result.
4. Explain how we can demonstrate current system failures using the tests that you provide, and map them to open issues.

The most explanation I see is in #8... I think you need a treatise on what you want. You have a big job ahead.

You have to remember that we're at the very conservative *end* of the release cycle, and that CSS aggregation mostly works for most people and has for some time. So people are going to be very reluctant (rightly or wrongly) to take a risk on a huge new redesign. I wish you'd started this initiative 6 months ago - I'd be much more optimistic.

Basically, I agree with mfer. You won't get this whole thing into D7. And you should probably work on one piece at a time. I know it needs a redesign, and that that is probably the correct approach. But it's too late in the release cycle.

I also agree with mfer that your best chance of getting this into D8 is by building a very high quality CSS aggregator module, as you've started to do. If that is high quality and popular, it will both solve the failings of D7 (people can switch to your module when they hit new bugs) and it will also be a very good candidate for integration into D8.

Thanks for the good work on this, and I look forward to good long-term results. Sorry for the pessimism about the short term.

@Casey, please realize we want to help you with this. We want the best system possible and are glad you've come along to try an help improve on that. Sometimes the way to get stuff in past the core committers isn't the way we like to work the best.

Missed this first time around, just found it via the css parsing issue.

I think we need a new, more targeted patch for this issue.

It's been pointed out by several people that this patch was unacceptable for several reasons. This issue has been silent for >3.5 years. None of the code applies anymore. Closing. If you still want to work on this, please open new, focused issues.

Thanks!