Term Delete Replace

1. Remove the LICENSE.txt file, it is automatically added to packaged files.
2. Remove the version line in the term_delete_replace.info file: http://drupal.org/node/171205#version
3. Tabs instead of spaces in term_delete_replace.module
4. Run through the coder.module

I'd call this module 'Term merge' instead, as it more clearly describes what it does.

Searching for 'term merge' in the modules throws up some existing modules, too.

> Also, please post a link to any module doing exacly this, without anything more added to it's functionallity and usecases.

That's a good point. Small, concise modules that do one thing well is very much the Drupal way.

> But do you really think it's better with Term Merge? Esentially, it's replacing a term reference of a node to another term upon deletion of the former one.

Yup. But saying 'term delete replace' is describing the UI rather than the real action you're doing.
Imagine someone looking for this functionality. Are they thinking 'My terms are a mess, I really need to be able to *merge* the duplicates' or 'I want to delete the crufty terms and switch in the good ones'? My bet is the first, as it's simpler conceptually.

On to a code review....

> name = Term Delete Replace

Follow standard capitalization for modules.

> particilar

Typo in two places.

> * @file

Good, but you should actually say something in the docblock! ;)

> $items['taxonomyreplace/autocomplete/%/%'] = array(

Better to use your module name as a base path here.

> * Implements hook_form_alter().

This is in fact hook_form_FORM_ID_alter().

> $form['description']['#markup'] = t($form['description']['#markup'] . '

' . t('You can replace any occurrences of the deleted term with another term by specifying the replacement term below. This is useful when correcting typos, or to consolidate multiple synonymous terms.'));

Don't re-include the original text in your t()! It surely will have been run through t() when it was put in the form in the first place.

(Also, BTW, gah, what is it with the taxonomy UI on D7? It's a total mess! That should be in hook_help surely! But this isn't your problem unless you feel like writing core patches...)

> '#type' => 'textfield',

I'm actually a bit concerned about this being a textfield... surely it should be taking a term ID, so in the submit handler you don't have to mess around with drupal_strlen and check_plain?
Also, what if there are several terms with the same name -- that could happen if you have a lot of duplicates to clean up.

> * Adds te chosen term to all nodes that was using the deleted term.

Typo. Also, 'were'. ;)

> // Check if the replacement term is the same as the deleted term. In that case, tell that to the user and return.

You should be preventing that earlier on, in the form itself. If you go on tids rather than strings, that's easily done. (Also, for future reference, this sort of thing should be happening in a validation handler not a submit handler, and with form_set_error not drupal_set_message.)

> // Create and save the replacement term if it doesn't exist.

That's just weird... why create a new term here? Surely if you want to delete a term and then replace it with a new term... you could just *rename* the original term!

> 'Added term reference "@replace_name" to all fields referencing the deleted term.',

Well... you're only doing nodes. Though I think you indeed should be covering all term fields, on all entity types.

> if (strpos(drupal_strtolower($tn), drupal_strtolower($typed)) !== FALSE && $tn != $deletedTermName) {

Avoid abbreviated var names and camelCase var names.

And finally, you need a newline at the end of the file.

Overall, this is clean code and there aren't any huge problems.

The two big things I think are needed are working with fields on all entities, and at a later stage, tests.

I would really recommend you ask to take over the abandoned project at http://drupal.org/project/term_merge, and put your code there as a brand new D7 branch.

> Do I have to be a "approved Git user" to maintain an already existing project?

Yes, I think you do.

Also, as that project is marked abandoned, but still has a named maintainer I'm not sure what the procedure is.

My recommendation is to keep working on your sandbox for now, and yes, contact that maintainer.

What I would suggest is a textfield autocomplete, but which within the $form itself stores a tid value, so your submit has a tid to work with.

You could still get problems with the autocomplete offering you two or more identical strings... in which case, yes, having '(tid)' at the end as well would be a good plan.

> - Now replaces term references in fields of all entities, not only nodes.

Nice job!!!

> - All actions use core API functions to make sure that the right hooks gets fired.
- API function term_delete_replace_merge can be called from any other module to merge terms.

Not only using Drupal API functions, but adding your own! Nice, I like that.

> (function ($) {
$(document).ready(function() {

I don't know that much about JS... but IIRC in D7 there's a special Drupal.behaviours thing that you put everything inside. Look at docs, and in core code for example.

> * Implements hook_form_validate().

Heh, that's not actually a hook. I think it's a handler... maybe? I tend to put "Validation handler for the foo_form form." where foo_form is the name of the form builder.

But again, see what core does for a good example to follow.

> $_SESSION['term_delete_replace_merged_terms'] = $source;

You should put that into the batch, as the argument to pass to the operation, rather than mess around with the session.

> function _term_delete_replace_merge_insert_field_values($update_data, &$context) {

If this is your batch operation callback, say so in the docblock header, eg 'Batch operation for wibbling the whatsits.'

> $options = array('create_replace_new_8563' => '<--- ' . t('New term, specified below') . ' --->');

What's the funny key for?

I've not yet installed this, that was just an eyeball review. Will try to find time to try it out later.

There's a few spacing and formatting issues and some typos to go over too :)

> I've contacted the maintainer for Term Merge but haven't heard from him yet. What should I do if he doesn't answer? Maybe I could rename my module to something similar to Term Merge at start? Any suggestions in that case?

If you sent a direct message using the Contact form, standard procedure is to wait 2 weeks, and then contact a site admin. I would give a link to this review issue too. The other thing to consider is that the maintainer has marked their project as abandoned already, so in theory it's there for the taking :)

Given the amount of work I've seen already, and the standard of the work, I would be happy to mark this RTBC. As noted above though, I think this should live at the abandoned term_merge project, so we should wait for that.

UI review coming up...

> // Only modify the form if it is in the "Confirm Delete" states.

I reckon you need more than this.
I'd actually forgotten that this module acted in the delete confirmation form. So I was in the same position as a user who has downloaded your module without properly reading the project page or the README. And I spent 5 minutes looking for a 'merge' button on the term edit form, or a 'merge' link in the term list, found nothing, checked the module was enabled, and then started putting debug statements in the code to check it was actually doing something..

> 'New term, specified below'

I still really don't get the point of this at all! Why would you ever want to do this? If you wanted to change the name of the term... just *change the name of the term*! ;)

> Merged one occurancy.

'occurrence' ;)

More thoughts on JS: are dependent form widgets doable with Drupal 7 core? It looks like you're doing quite a lot of JS there...

I also found a bug, which I'll file in your issue queue...

Regarding the abandoned project, see http://drupal.org/node/251466

See #180528: Batch API overview.

Not terribly well documented, but this will do it:

    // Store some result for post-processing in the finished callback.
    $context['results'][] = check_plain($node->title);

I think you're going to have to pass it in to the operation handler, which can then take care of stashing it for the finish function to find. Messy, but less messy than using $_SESSION.

Technically, to be clean, you should only add a deleted term tid to that list once your operator has successfully completed its work.

Though you might also consider filling a feature request on Drupal core for the finish callback to have parameters in D8...

Brilliant.

Marking this RTBC :)

The maintainer can give you Git access themselves (and they did) - so this is no longer necessary. But in the future when you want to create a new module you'll still have to go through the project application process.

> But in the future when you want to create a new module you'll still have to go through the project application process.

That seems a bit unfair.
What is being granted by this review process is a role -- successful applicants may freely create full projects as they wish. Nylin submitted a complete module here, and on the basis of that code I think the full project access should be granted.

If for the purposes of your project application someone accepts you as a co-maintainer, as far as I know it renders the project application void. This is why we want to encourage project applications to make sure to reach out to existing modules early to coordinate rather than investing a lot of time into your own code and application.

This falls under "Module 'Foo' no longer appears maintained so I have written a new version I would like to contribute." on http://drupal.org/node/539608:

Realistically this forms duplication and Drupal.org tries to avoid this where possible (see the previous link for why this is so). The best way forward in this situation is to provide feedback to the module with patches rather than strike out afresh. If the module appears abandoned and patches sit by without review of commit, use the Webmasters to ask to take over an orphaned module.

I'm sorry that you've invested time in this project application, but you have gained knowledge of how the process works, using proper APIs, etc that should help your next application go much faster. When you do apply again for a new project idea, I'm sure that joachim and myself would be happy to give you a quick and fair review.

Dave, you perhaps haven't read all the details of this application.

> The best way forward in this situation is to provide feedback to the module with patches rather than strike out afresh.

Not quite the case here.

The existing term_merge is marked as experimental.
The applicant here has supplied a complete, working module.

Also, while we are reviewing *modules*, we are granting *access*.
We give the *module* full project status, but we at the same time grant the applicant the ability to go on and create further full projects, without the need for approval.

I feel that the applicant here has more than amply demonstrated their ability and talent, and should be granted this level of access.

We can't bypass the process or bend the rules for someone if we don't consistently do it for everyone. I agree completely but identifying duplicate modules and possible coordination should be the first step before any more time is invested in a project application as it prevents hard work from 'being deined'. Further discussion should probably happen on http://groups.drupal.org/code-review.

Well I'm rather disappointed about this, and I believe that the in the essence of any fair system or set of rules is that it can be bent according to circumstance -- no rule is an absolute, everything depends on circumstance. That's what using one's judgement is about.

> When you do apply again for a new project idea, I'm sure that joachim and myself would be happy to give you a quick and fair review.

Yup.

Nylin, I'd like to say that I've been very impressed with the standard of your work -- the Drupal community needs people like you so I hope you won't be too put off :)

I'll try to find time to check in on your ongoing work with term_merge (as it'll now be called). If -- or rather when -- you have an idea for a new project, please contact me directly and I'll sail your application through to RTBC :D