Provide a way to add a new default component to existing projects

This would just be a change to a Drupal.org setting. Moving to a more appropriate place for consideration.

Also note that project maintainers can customize these choices per project, and changing the site-wide defaults won't retroactively add this choice to all existing projects. It would only affect new projects added after the setting was changed. In theory we could write a batchimg update to go through all existing projects and add it, but that's a bit of a pain and folks could just remove this option again, anyway.

+1, there's really no negative here: if it's not used, it's invisible, and it's an option, it can be used to help devs sort by type of feedback.

If I change the setting, any *new* projects created from this point forward will *start* with a "Markup" option as a component.

- 5000 existing projects won't have that option
- New projects are free to delete this option

Why not just start using a "Markup" issue tag? That way a) it works on all existing projects and b) it can't be removed as an option.

pros for making it a component level option:

1) It's right there, not hidden as a potential tag someone would need to know about or look for.
2) It might not be right for all 5000 groups, and might be deletable, but most won't delete it, and it does apply to most projects.
3) It helps to separate form from function: Right now, in most cases, the only (likely) option to pick is 'code'.
If you are outputing valid code, but extra markup (or missing markup, etc), it's not really 'bad code', and it's very easy for someone to feel like that's a false error "Hey, the _code_ works fine..."

Cons:
1) A bit of work to add to existing projects. Not a huge amount, but non-trivial.
2) Tags could be used... (but by that logic, why do we have components anyway? Why not just use tags? - Answer: because some fixed categorization makes sense for bug reports)
3) You'll encourage these damn designers that they have something to contribute to Drupal. (grin)

I think this is a great idea! It helps move design considerations to the same "level" as code, user interface, and documentation, which is very important for Con 3). :D

But yes, we should make an upgrade path for this. Maybe something like:

if project has the default set of components in it somewhere, add "Markup" to the list.

This way, projects with a highly customized components list, such as Documentation, wouldn't have to be bothered removing a bunk listing.

I'll donate two hours of dedicated Drupal 7 code review on your patches of choice to anyone who writes the upgrade path for this issue! :)

@winston and sethcohn: You seem to have mistaken me for someone opposed to making our issue queues more effective for designers. ;) I'm not at all opposed to that. I just know that an issue tag takes 0 work to implement and could serve positive benefits immediately (even if it has some drawbacks), whereas the 15 second version of adding a "Markup" component is full of limitations and problems, and the elaborate task of an upgrade path is going to be a few hours of work. Long, long ago I stopped being able to spend 2-4 hours on any random issue that people had an itch about, just because I happened to be the most qualified person to do the work. I need to make a living, and that means prioritizing paid work. So, yes, if the lure of 2 hours of webchick's D7 patch reviews is enough incentive for someone else to work on the upgrade path, hurray. But, I'm trying to make sure that people understand the implications of the choices they're advocating for.

Many proposals for some d.o feature that would require a few hours of custom work are never implemented. So, you can either immediately have a partial solution (issue tags), or risk never seeing this happen because it withers on the vine and dies like so many bright ideas that no one is willing/able to implement...

Just being honest (and probably jaded by prior experiences -- if so, apologies for that).

Cheers,
-Derek

+1

The simple fact is, those designers that we are REALLY trying to attract to Drupal that aren't used to issue queues, or commiting code, or sorting through issue queues with hundreds of issues, this gives those designers that really DO want to try to help out the Drupal community an EASY way to just say... "Ohhh, this markup category applies to me, I might be able to actually contribute there..."

It really seems like a no brainer, and not that complicated to implement.

himerus: Um. Huh? :) Did you not read dww's comments? Yes, it's trivial to implement for new projects that are created after today, but it leaves out all of the 5,000 existing projects, unless someone writes an upgrade path. We're not going to attract designers to the project when only 1/5,000 projects (and none of the common, most widely used ones) has this option; it needs to be a Drupal.org-wide thing to be effective.

Or, if you were referring to the upgrade path when you said that this is not complicated, then awesome! Please provide a patch! A good place to start would be downloading the Drupal.org testing profile which will help get all of the various project* modules in place.

winston: I installed project_issue (well, the entire project testing profile) before I saw this, and you're pretty much on the money in your analysis.

Serialized array of component values as a single field 'components' in project_issues_projects table.
Loop thru all records (you could limit to only current ones, skipping old ones, but for simplicity, let's assume anything that matches below conditions should be updated)
If the current value of that field is equal to the old default components array (serialized), (or at least has all of those options intact?, unserial and check) (or maybe just has 'Code'?),
then add 'Markup' as new option at end of unserialized array, reserialize and save.
If not, log it and move on.

I think this could written up as a pretty basic patch to avoid one off coding and allow future similar changes if desired, so that on the admin page
(functions project_issue_settings_form and project_issue_validate_default_components in admin.settings.inc)
(maybe a new textfield on the form under the current list in the text area? so when it's populated, could trigger this sort of 'Go update all old projects components similar to the defaults with this new entry' function.)

Heck, if you were willing to accept only updating projects that were _exactly_ the old defaults, and not customized at all by the maintainer, a quick and dirty SQL solution will do that:

UPDATE {project_issue_projects} SET {components} = 'a:5:{i:0;s:4:"Code";i:1;s:13:"Documentation";i:2;s:13:"Miscellaneous";i:3;s:14:"User interface";i:4;s:6:"Markup";}' 
WHERE {project_issue_projects}.{components} ='a:4:{i:0;s:4:"Code";i:1;s:13:"Documentation";i:2;s:13:"Miscellaneous";i:3;s:14:"User interface";}' ;

If that command was run right now on d.o, most projects would be all set with the new value. Then generate a report for all other 'components' values, and ask those projects admins to make the change, or just do the unserialize/add/reserialize on them.

Sure, a patch to project_issue to make it possible to safely do this in the future would be lovely.

For some related code that could be very useful, and which is already using the BatchAPI, check out: #140989: Renaming components is unsafe

Thanks for working on this!

Cheers,
-Derek

Yeah, I started writing it last night, should have it done later today, if I can finish other (paid) tasks first. I'll then be away for the weekend, and most of the next week, so if someone (winston, dww, anyone else) wants to take it and refine it a bit, have at it. I welcome the eyeballs.

#140989: Renaming components is unsafe points out the complications of changing _existing_ components. My patch will avoid that mess (I do hope that patch will get refactored and committed, but I don't have the time to do that, and it didn't directly scratch my itch) As a result, it's clear that the most simple approach (if all we did was replace the component field on projects that still had the old 'default' list) isn't the best, since someone who removed an existing component from the default list would cause existing issues to have incorrect values. Replace/Rename/Delete MUST be done in a manner as in the other patch. Enforcing that no components be removed seems too draconian and limiting for a patch, so I went a different way...

What my patch does do (or will do, once I finish and post it): Adds 2 optional textfields, for 'add a new component' and 'but only if this other component exists'. Then it calls a batch function as part of the form submit, which SQL selects the matching projects (all, or only ones that have the preexisting component already, thanks to the serialized field SQL can easily match this without having batch check every single project), And then adds the new component to the list for just those projects.

This easily allows for our desired use (Find all projects with "Code" component and add a new "Markup" component), and is still flexible enough for general use. (If someone would like to improve this to make it optionally do a NOT, to allow adding only to projects lacking a given component, that should possible for example)

Looking at the top 10+ projects with the most issues, many of which _are_ custom component lists, either they have removed all of the defaults and put in a custom list, or they have minorly added to the default list, or they are stock default.
So the projects with custom components will most likely be left alone, which is the desired behavior. In the top 10, I didn't find any that would have been given 'Markup', due to 'Code' existing, where it wouldn't have made sense as another viable component choice.

Generically, other uses might include adding a new component to only projects with a minor (or very focused) existing component, for (a random and not necessarily perfect) example, "find all projects with 'cck.module' as a component, and add 'core fields' as a new component.

As an advantage, should someone fix up #140989: Renaming components is unsafe, this would remain a useful addition in functionality not covered in that patch, and compatible with it.

I should have something later today. I spent part of the morning wrestling with Batch API, and will continue later.

Enclosed find the patch, as promised.
(Batch API was a pain... took a while to figure out that due to this taking place in an include file, I needed to pass it the file location of the include, which was in the docs on API, but unclear that was the reason it was running and displaying initial batch data screen, but never doing anything else, never executing the batch callback itself. When in doubt, re-read the docs till you turn blue and then feel stupid and turn red.)

Derek and Winston, feel free to massage as needed... if I had more time, I'd run coder on it, and double check a few things. I did test a few different cases, and it worked correctly in all I tried.

@winston: HEAD is from CVS. In project and project_issue right now, the 6.x-1.x-dev releases are pointing to HEAD. You can always figure it out by visiting the "View all releases" page for any project, and searching for the one that says:

"Nightly development snapshot from CVS branch: HEAD"

Anyway, I've recently committed some changes to project and project_issue. If you check out from CVS, be sure to use "cvs update -dP" to make sure you're getting the new directories I've added.

Also, there was a syntax error in project_release.module until I just committed a fix a few minutes ago.

winston, no, you didn't do anything wrong. It does have additional requirements beyond the listed ones. I've got (thanks to Drush for the list (trimmed a bit)):

Name Enabled/Disabled Description
Code Filter (codefilter) Enabled Provides tags for automatically escaping and formatting ...
Comment alter taxonomy (comment_alter_taxonomy) Enabled Allow users to alter the taxonomy terms of a node from a ...
Comment upload (comment_upload) Enabled Enables file attachments on comments
CVS integration (cvs) Enabled Provides access to commit logs, account management, and ...
Database logging (dblog) Enabled Logs and records system events to the database.
Devel (devel) Enabled Various blocks, pages, and functions for developers.
Install Profile API (install_profile_api) Enabled Utility functions that help with install profile creatio ...
Profile (profile) Enabled Supports configurable user profiles.
Project (project) Enabled Provides a project node type and browsing of projects.
Project issue tracking (project_issue) Enabled Provides issue tracking for the project.module.
Project releases (project_release) Enabled Provides a release node type to represent releases of pr ...
Update status (update) Enabled Checks the status of available updates for Drupal and yo ...
Views (views) Enabled Create customized lists and queries from your database.
Views UI (views_ui) Enabled Administrative interface to views. Without this module, ...

I think Views and Views UI weren't listed in the requirements for the project install profile.

Plus I know had to use the dev for install_profile_api-6.x-2.x-dev.tar.gz, not the 2.0 release

bump, anyone get a chance to review this patch yet?

I didn't install it from cvs, it was all tarballs, all -dev versions, with the exception of views-2.6
and I believe one patch #429792: Add API for manipulating issue status options needed to fix a profile install bug.

First of all, thanks so much to both of you for working on this patch, both writing it and testing it! It's greatly appreciated. I haven't tried running the patch yet, but I just did a thorough review of the patch and found some problems, most of them minor/cosmetic. I could easily just fix these myself, but in the spirit of mentoring and training, I'm going to take the time to explain the problems so y'all can both learn from this. If you're going to be contributing more patches in the future (and I sure hope you do!), especially for core, you're going to need to deal with these sorts of things and should just be in the habit of writing code "properly" in the first place. ;)

A)

+   $newcomponent = check_plain(trim($form_state['values']['project_issue_add_new_component']));

We don't sanitize data on input, only on output. By using t() with a placeholder like '@component' the component's name will be sanitized already -- so, this patch ends up double-escaping, and if you had a component like "wording & grammar" it would end up looking like "wording & grammar"...

B)

+ $title = t('Adding new component @component to existing projects @conditional', array('@component' => $newcomponent, 
+ 	'@conditional' => $conditional ? "with ".$conditional : '' ));

using t() like this makes "with" untranslatable, and generally makes life difficult for translators, especially in right-to-left languages (RTL) like Arabic. Instead, you want something like this:

if ($conditional) {
  $title = t('Adding new component %component to existing projects with %conditional.', array('%component' => $newcomponent, '%conditional' => $conditional));
}
else {
  $title = t('Adding new component %component to existing projects.', array('%component' => $newcomponent));
}

Also, by using %component instead of @component, the string is passed through theme('placeholder') which not only sanitizes the string, but also allows sites to consistently format the placeholder inside the wider message. By default, it's wrapped in <em> for italics...

Finally, we don't normally start a newline for the array of t() placeholder values like this -- generally, drupal's code style is to put everything like this on a single line.

C) A function named "project_issue_add_new_component()" sounds like a nice API function someone might want to call programatically. But in this patch, it's really a submit handler for a specific form. So, if someone wants to do this operation, they have to fake a form submission. I'd rather see this function have a signature like this:

function project_issue_add_new_component($new_component, $conditional = NULL) {
  ...
}

Then, we'd want something like project_issue_admin_settings_submit($form, &$form_state) that was the actual form submit callback, which got the values out of $form_state and called project_issue_add_new_component().

D) Speaking of project_issue_admin_settings_submit() -- since we're using system_settings_form() we're getting a form submit handler added for this form automatically, which does some specific things with the $form_state['values']. In particular, every value submitted with the form is stored as a drupal setting variable available via variable_get(), etc. It's wasteful to persistently store project_issue_add_new_component and project_issue_add_new_component_conditional as variables. Instead, our custom submit handler should unset those values from $form_state['values'] and that should prevent them from being saved via the submit handler added by system_settings_form().

E) We should be consistent with using t('something') instead of t("something"). The only time to use t("something ain't right") is when you've got any need for ' inside the string. Some translators seem to care about this, and I'll defer to them. ;)

F) I think this might be a problem:

+     	$result = db_query("SELECT nid FROM {project_issue_projects} WHERE components LIKE '%s'", '%'. $conditional .'%');

I believe this is better, given how %s gets escaped and replaced:

  $result = db_query("SELECT nid FROM {project_issue_projects} WHERE components LIKE '%%%s%%'", $conditional);

I know it looks a little worse, but that ensures we get unescaped % before and after the conditional...

G) Minor code-style bug:

+     } else {

we do:

  }
  else {

H) Always use t() placeholders for stuff instead of concatenating strings, since the order might be different in RTL languages. So, instead of this:

+     $message = t('An error occurred while processing with arguments:'). print_r($error_operation[0], TRUE);

we want something like this:

    $message = t('An error occurred while processing with arguments: @error', array('@error' => var_export($error_operation[0], TRUE)));

Also, since we're using '@error' like this, the error message itself is escaped before printing, which is important in case the contents of $error_operation[0] ever include any unsanitized user input.

I) Minor code-style:

+     while($nid = db_fetch_array($result)) {

use a space after while and before the '('. We do the same for if (), foreach (), etc -- and your patch gets this right in the other spots I can see.

J) We don't use tabs for anything. So, this:

+     $context['finished']                  = 0;
+     $context['sandbox']['max']            = count($context['sandbox']['projects']);
+     $context['sandbox']['progress']       = 0;
+     $context['message']                   = t('Adding new component...');
+     $context['results']['updated']		= 0;
+     $context['results']['processed']      = 0;

isn't what we want. I also don't find this style of indentation any more readable, and would rather just see it like so:

    $context['finished'] = 0;
    $context['sandbox']['max'] = count($context['sandbox']['projects']);
    $context['sandbox']['progress'] = 0;
    $context['message'] = t('Adding new component...');
    $context['results']['updated'] = 0;
    $context['results']['processed'] = 0;

K) Minor code style: we always put the final ',' on any array definition. So this:

+       'file' => drupal_get_path('module', 'project_issue').'/includes/admin.settings.inc'

needs a trailing ','

L) Minor code style: indentation on the PHPdoc comment:

+ /**
+ * Batch 'finished' callback
+ */

should be:

/**
 * Batch 'finished' callback.
 */

Argh, the code tag formatter is screwing this up... :( it should be something like this:
/**
 * Batch 'finished' callback.
 */

M) The other new functions should have comments like this that explain what they are.

That's about all I saw. ;) Thanks again for your work on this!

Cheers,
-Derek

+1 from me. Looks good.

Came unstuck by this issue also

Is this still relevant for D7? We came pretty close from the looks of it to getting it into D6.

If there isn't momentum around it, let's just close the issue.