Comments

markhalliwell’s picture

Ok, code feature freeze is fast approaching. I guess my question is this: isn't core moving more into a framework code base? If so, couldn't Color really just be included in a distro? After following several issues that have to constantly re-patch against the core head, I think it would benefit the development of Color to decouple it entirely.

I know people have made it clear from the previous issue that this module gives you an "out-of-box" experience. I feel, however, that could also easily be accomplished with a distro. I don't think core should have modules that are very specific to a type of functionality that, at best, are really just an optional enhancement.

tim.plunkett’s picture

Considering the Link and Views module are in Core and Date module is soon to follow, I'd say this is wont fix.

markhalliwell’s picture

Understandably those modules make more sense to be included in core. They are widely used/required by many contributed modules and provide a very broad use case. On the flip side, Color is used very specifically and I would suspect that it is only used initially by a developer/themer to setup the theme. Once the configuration is done, I think Color is usually put on the back burner (or at least it is by me), in terms of functionality. How is this the same with Views, Link and Date?

boombatower’s picture

Yea views and link being in core is irrelevant.

dodorama’s picture

At this point in time, 5 days from feature freeze, it seems unlikely that color module will see any enhancement/refactor to make it core worthy. In my opinion this functionality is better suited in contrib, where it can easily be accomplished in conjunction with a modern preprocessor (SASS, etc.).

markhalliwell’s picture

I completely agree. I would love to have new features included with core, but given the deadline now, I don't think that's going to happen. It would be far easier to actually make changes as a contributed module, than in core. Especially with all the constant HEAD pushes and having to re-roll. I just don't have the time in 5 days to get everything that I want in it and then get everyone's feedback/approval.

markhalliwell’s picture

Any chance we can get some higher-up discussion on this? I still think if Color becomes a contrib module, it could easily be added in a distro. No real reason to have it in core.

tim.plunkett’s picture

Once you remove it from core you have to remove Bartik's support for it from core. How would you add that back in from contrib?

markhalliwell’s picture

Why would you have to "remove" the support of color in Bartik??? The theme would still work, regardless if Color was installed or not. We could add some notes to a README file in Bartik that explains how to enable color support. Again, if Color were packaged in a distro, then it would work "out of the box". I'm not saying that there is an "elegant" solution here, but I honestly don't feel Color will receive much work if it continues to remain in core. I feel there is just too much overhead to consider (especially with all the restructuring of core) for an add-on that is used rather uniquely and specifically.

If there's qualms about having support for something that wouldn't technically exist in core, here's another solution. Provide Bartik's color support IN the contrib Color module. It's a given that Bartik is included with Core. If the Color module is added or packaged with a distro, support for Bartik could be automatic. It wouldn't be that hard to provide some support and checking if the theme in question matches "bartik", then load the "bartik" specific resources.

In fact, this might be the better approach given that there might be quite a bit of API changes in Color and how to configure a theme's "color support". This way, core can remain clean and any additional features or changes won't impact core directly.

tim.plunkett’s picture

Core doesn't ship with support for contrib modules, that's just how it goes.

Provide Bartik's color support IN the contrib Color module

I'd like to see code that does that, I'm very unsure how that would work.

markhalliwell’s picture

I figured as much (core would be nasty if that were the case lol).

Considering that Color ALREADY hacks the theme's config form (see: color_form_system_theme_settings_alter()) to insert it's own form, you can see that it knows what theme it is by the following:

  if (isset($form_state['build_info']['args'][0]) && ($theme = $form_state['build_info']['args'][0]) && color_get_info($theme) && function_exists('gd_info')) {

Ok, so the real power behind Color is the color_get_info() function. How hard would it be to do the following in that function, I mean seriously?

  $path = $theme == 'bartik' ? drupal_get_path('module', 'color') . '/bartik' : drupal_get_path('theme', $theme);
  $file = DRUPAL_ROOT . '/' . $path . '/color/color.inc';

I think this is how support for Bartik should be provided. I also think this would help in clarifying the structure of color support for documentation. We could then say that if a theme want's to "provide support for color" to look at the example "bartik" folder included with the color module.

In regards to how color currently functions, when the theme settings form is saved (see: color_scheme_form_submit()), it generates a color.css file in the files directory and sets variables of what "color" files to use for that theme.

Where the files get injected currently is determined on the theme level... and in my opinion this is EXTREMELY backwards to begin with.

Currently a theme must provide hooks, (using Bartik as the example): bartik_process_html() and bartik_process_page(). I should also note that the maintenance page doesn't even provide color support: bartik_process_maintenance_page().

These hooks then call the respected "internal" color module functions: _color_html_alter() and _color_page_alter().

Now, let's actually take at the purpose of _color_html_alter(). I understand that this is to inject the color css AFTER the theme's, but couldn't that be accomplished by throwing a hook_init() in color, loading the current theme and then adding the necessary CSS files using drupal_add_css($file, array('group' => CSS_THEME, 'weight' => 1000));?

Now, let's actually take a look at the purpose of _color_page_alter(). Ok, so it's just replacing the logo. Can't this also be handled by inserting a hook_page_alter() for color??? Or at the very least, we can accomplish this when the theme's settings form gets saved: color_scheme_form_submit().

Ultimately, how we provide "support" for color still relies too heavily on the theme. Why should the theme itself "ensure" that the color support is injected properly? If they already provided the color support by providing the necessary "color" files, isn't that were the "support" should stop? Shouldn't the Color module actually handle HOW and WHEN the files are injected? Isn't that the point of a module?

markhalliwell’s picture

Status: Closed (won't fix) » Active

I think the very fact you brought up about how coupled Color is with Bartik should be reason enough to move it to a contrib module. You bring up a valid point, Color and Bartik (even Garland) are co-dependent. This just adds to the reason why I feel Color should be decoupled entirely from core. Given the the advancements in D7 alone: parsing a theme's info file, better page and html alters, better CSS and JS heirarchy a LOT of the issues and limitations (mostly because the module hasn't changed since it was introduced in core) that currently exist in Color can be refactored and fixed to truly become a standalone module.

If I were to create a new XYZ theme utility module (even in D7), of any kind, the XYZ module can't add support to Bartik because it's part of core. So it adds native XYZ support for Bartik actually IN the XYZ module.

Granted it will automatically look for ANY theme with this support, but it knows Bartik is a core theme and is always there. It changes where it should look for the support files when using the Bartik theme. This allows the XYZ module to demonstrate it's functionality as a standalone module. If a theme wishes to add support for this great and awesome new XYZ module, then the project and documentation pages would point the themer to look at the existing Bartik subfolder in the XYZ module which "mimics" the file structure needed in a theme. The XYZ module would also include a detailed README with instructions on how to provide theme support.

This is pretty typical of any module that enhances or extends the functionality of core. So no, Bartik shouldn't have any Color support in Core. It should be moved to the module that actually provides this only type of functionality in the first place.

Edited: First line to better reflect what I was trying to say. It's not the entire basis of why I think it should be moved out of core.

Bojhan’s picture

Status: Active » Closed (won't fix)

Sorry, but this is a won't fix. As clearly stated by others this is part of Drupals initial offering, you might not fall in the audience that might use this - but frankly, this functionality is very often used by our small site long-tail.

Although you might disagree with its implementation and expect to see it in a distro, removing it just because of that to me sounds silly. I am sure this will be reopened again and again, but I am pretty sure it will end up being a won't fix for Drupal 8 :)

markhalliwell’s picture

Status: Active » Closed (won't fix)

You know what, I'm not even going to bother with this anymore. I get asked questions, I respond with solutions. Then they're just shot down, over and over. This "community" has some serious issues. The whole point of this issue was to get the community's (as a whole, not just a few people) feedback and ultimately a decision from Dries or Catch or someone that has the final say so.

I am beyond frustrated and extremely discouraged right now. I feel like the people (including ME) who actually USE this module, wanted a better TOOL for their themes. It's why I got involved in March (#445990-24: [META] Refactor color module) and proposed radical ideas. There was absolutely zero feedback from higher-ups (except Rob, who was at least nice to say he loved the designs, thanks Rob! :D). I am a themer who dabbles heavily in development as well, but I am no where near ready to dive into the complexity of patching and discussing core (especially with all that's going on lately), I just don't have time for it.

I knew it would be FAR easier for this module to be managed in a contrib module. You have less core noise and more focused attention (I should know, I respond rather quickly to my modules as I have them emailed to me). I'm not really looking to be a core maintainer, but if that's what you want then good luck. Color might just sit for yet another two releases. I was willing. At least you got some good ideas out of me if you even choose to use 'em.

And for the record, the Color module isn't "functional" it's a cosmetic feature; an enhancement. It does not break a site nor does it cause one to be irritated that something doesn't "work" right. The whole basis and arguments people have with the UX aspect of Color is also largely bloated and bias.

Is it cool to have that "out-of-box" UX? Yes. Is it completely necessary. NO. Could we have offset Color's removal from core by placing it in a Distro that the public downloads (which would have also included other commonly used modules)? Yes.

But whatever, have it your way... so much for open source.

JurriaanRoelofs’s picture

Hi Mark, I understand your frustration but I think we should also be more understanding of core maintainers who already have a lot on their plate and have to choose their battles. This battle is a risky one because taking color away from core will be a very visible change to many users. Then when everyone finds out there is a new, actively developed color module people will have an opinion about the new stuff and both the maintainer of that module and the core maintainers will be responsible for how well that works.

Not saying that this shouldn't happen, it definitely should but like you and me; everyone has limited time and this takes a lot of time to do it right. If you want any help with coding a contrib module with a better UI you can always message me. I think I have good authority on color module because while in the original post #445990: [META] Refactor color module I reported to have made 5 color module themes that's now more like 20 themes.

Dave Reid’s picture

Issue summary: View changes

Definitely think that Color module should not be removed at all. With the news that core 8.1 can contain backwards-breaking changes, if someone can champion refactoring Color module, there's no reason it couldn't be included in a point release for Drupal core.