Posted by Gábor Hojtsy on August 26, 2011 at 8:16am
6 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | language system |
| Category: | task |
| Priority: | normal |
| Assigned: | tarmstrong |
| Status: | closed (fixed) |
| Issue tags: | D8MI, language-base |
Issue Summary
Amazingly http://api.drupal.org/api/drupal/modules--locale--locale.admin.inc/funct... is the only place where language deletion happens. We all know doing these in form submit hooks is equivalent to driving us off a cliff. This should be API-ified like we do for loading and saving languages proper.
Let's start with just reworking the code we have in the form submit function to a language deletion with hooks for modules to chime in.
Parent issue
#1260488: META: Introduce real language APIs
Comments
#1
Tagging.
#2
Assigning to me.
#3
Here's a patch for adding a locale_language_delete() function in locale.module.
#4
Thanks! Some comments:
1. Let's modernize this code while we are here like we did with #1215716: Introduce locale_language_save(). The multilingual_settings_changed hook is about to be removed with these changes in favor of more specific hooks like _presave and _insert/_update in that function. We should have similar patterns applied here. Then other modules can react on the delete action of a language, and the node related code can then be moved to node module into a hook implementation that reacts to a language being removed. And other modules can react to this. This is a seriously missing feature in Drupal 6/7.
2. The $language argument is just a language code. We so far use $langcode for that, so let's stick to that here too. It is not a language object, so $language is misleading.
3. You have a static variable for the language list. This means if someone attempts to delete the same language code multiple times in a request, it will fire all the database queries and the hooks and fire the watchdog messages.
4. There might be other places where the language list is cached with drupal_static, those caches would need to be cleared I think. Whether it is this function's job or should be done via hooks is a question of whether they are internal to locale module I think or external.
That is all, keep up the good work :) Thanks!
#5
#6
Oh, and also, let's not include the issue URL in the phpdoc comment (we never really do that) but do include a line of doc on the $langcode. You'll find standard docs for that on other functions where it is used. I know its a pattern, but its best to have it documented, especially on core API functions like CRUDs for stuff.
#7
Thanks for the feedback, Gábor. Here's an updated patch. I cleaned up some of the variable names so it's a little more readable now.
Two questions:
#8
#9
@tarmstrong: great improvements. Let's make it even better :)
1. Taking $langcode is good and consistent with node_delete(), user_delete(), etc. it makes best sense to keep consistency with the rest of Drupal's APIs.
2. I think we can return TRUE if we successfully deleted it, and FALSE if not. That is not really standard to things like node_delete() and user_delete(), but its good info for the outside to make noise if they want to. You can "make noise" in watchdog() but please no drupal_set_message() in an API function :) That should be the role of the UI. See the next point also :)
3. I'd move the drupal_set_message() back to the form API submit callback. API functions should not put out user messages. If you want to delete 10 languages at once with the API, you probably like to output a summary message not to show the user all 10 messages :) Its the responsibility outside of the API.
4. It is important to pass on the language code with the delete API invoke, so implementers of the API would know which language is removed. The old multilingual settings changed hook did not do that but that is a mistake :) Here we know exactly what info to pass. Again, like with node_delete(). Consistency is key. In fact now that I look at it, node_delete passes on the whole node object in its hook invokes. Hm. Hm. Then probably we should do that too. (And do the hook invoke before we actually delete the data, like that).
5. Cross dependencies are to be avoided. We actually have an issue to remove node related stuff from locale module at #540294: Move node language settings from Locale to Node module, but this issue is what makes it possible for language delete, so we should take advantage of it and do it here. Let's do our implementation of hook_locale_language_delete() then in node.module and move the node query there :) Decoupling is good :) This solution will make it possible for other modules to do the same things. Like we don't have this for path module or user module. If a language is deleted but you have paths in that language or comments or users, those will stay around. Kind of broken. Not to be solved here, but just as examples, that we make that possible here for them to hook into language deletions and act on it.
6. Now that we are adding hook_locale_language_delete(), we need to document that in locale.api.php, see again the locale_language_save() issue for that stuff.
#10
Thanks again!
Should there be a pre_delete and a delete hook, or is that excessive?
#11
Once again, I think whatever other delete workflows have, we should if possible, but not any more if possible :) Consistency, consistency, consistency :)
#12
Just adding the @return line to phpdoc for locale_language_delete().
#13
Very good, very good. Two things I noticed still:
1. The api.php function should have some kind of short example there. Like a short update to some imaginary data or something. Other functions like that have similar examples. It could use the node implementation as an example for simplicity :)
2. I'm not sure the page cache clearing belongs back to the form API submit. I think it made sense in the API function. Yeah, removing language does have wide reaching implications. :) This is probably debatable and will/might be debated by others :)
#14
Good point about the cache clear. I can't imagine a situation in which I would want to delete a language without deleting the page cache. But there are more creative people than me out there :)
Updated with the api.php change and the moved page cache clear.
#15
Looks good to me now. Would be good to have more reviewers, but I think its good now as-is :)
#16
In fact, we figured out in the save patch that we can partially remove the very ambiguous and non-standard multilingual_settings_changed hook once this patch lands. The latest save patch at http://drupal.org/node/1215716#comment-4938280 already deprecates the hook and offers more standard alternatives for insert and update language operations. This patch should complete the removal by implementing the delete hook on behalf of the field module (like it does for node module already). Yay for standardization!
#17
Updated to change field_multilingual_settings_changed() to field_locale_language_delete().
#18
#19
subscribe
works for me
moving cache_clear_all outside from db_delete stack, cause DrupalCacheInterface::clear can be overriden in contrib modules and not safe. Stopping at this point can delete all the strings from table locales_target without language deletion from table languages
<?php
// Remove translations first.
db_delete('locales_target')
->condition('language', $language->language)
->execute();
// Remove the language.
db_delete('languages')
->condition('language', $language->language)
->execute();
?>
Deletion is very fast even on large tables so no need to use transaction (paranoic 8)) )
#20
Looks good!
Updated the patch a bit:
* Remove information about hook_multilingual_settings_changed() in locale.api.php. Not sure if this is too early to be removed, since #1215716: Introduce locale_language_save() has to land as well before the hook is completely gone. But it'll have to be removed at some point anyway...
* Removed useless return statement in locale_languages_delete_form_submit()
#21
#20
looks good
except
podarok@pubuntu:/var/www/d8/drupal$ git apply -v locale_language_delete-1260528-20.patchlocale_language_delete-1260528-20.patch:151: trailing whitespace.
locale_language_delete-1260528-20.patch:154: trailing whitespace.
#22
Fixing whitespace
#23
#22
nice!
looks like RTBC
#24
Agreed, looks good.
#25
Reviewed this and it looks like a nice clean-up that had great reviews. Committed to 8.x. Go i18n initiative, go! :-)
#26
Automatically closed -- issue fixed for 2 weeks with no activity.
#27
Tagging for base language system.