Move negotiation settings out to their own include file

Big improvement!

I think like you. "Selection" is less ambiguous than "negotiation". The first time I read "negotiation" my mind was confusing about its purpose.

- +1 for "selection"
- I would name the new file locale.selection.admin.inc (or whatever term is chosen) to stress the fact that this file handles the admin part of language selection (though, that would mean to do the same on many other files, including the import, export files in [31219236]
- You say that this patch only moves code, so I guess a follow-up issue will be registered as soon as this one is committed that targets updating the comments and renaming the functions where appropriate? If so, I would propose to create that issue right now and set it to postponed (as being waiting on completion of this one).

There's now lots of "configure" in both function names and url's. I would change that all to "selection" (or whatever will be chosen). That's what I meant to say. You could open an issue about the naming anyway, to restrict this issue purely to moving code around.

If restricting to moving code, I consider this issue reviewed by myself and tested by the bot. Not sure if that is enough for RTBC :)

A big +1 to rename everything "detection" or "selection" (personally I prefer the former). I agree at all with #5.

About file naming conventions I think we might want to ask a feedback from our friendly core-gardener (@sun): let's follow them if any, otherwise let's find an agreement on something that can be used consistently all around core.

+++ b/modules/locale/locale.negotiation.inc
@@ -0,0 +1,262 @@
+ * Langauge negotiation administration functions for locale.module.

Typo: "Langauge"

11 days to next Drupal core point release.

I'm sorry, but the proposed split doesn't make too much sense for me.

locale.admin.inc contains UI functionality for:

• adding/editing/removing languages.
• configuring language negotiation.
• listing/filtering localized/translated strings.
• importing/exporting localized/translated strings.
• editing/deleting localized/translated strings.
• listing/adding/editing/resetting date formats (types).

Now, categorizing this functionality, I see:

1. Language setup and language negotiation configuration, including the only "true" locale configuration possibility in core thus far (date formats)
2. String localization/translation

So, if splitting this file at all, then I'd split it that way.

However, the question of splitting in which direction is a tough one, as it largely depends on the outcome of a couple of other discussions we currently have.

I'd see the following two options:

1. Split string localization/translation functionality into locale.pages.inc or locale.translation.inc.

   That would be on the assumption that all of this string translation functionality is actually a whole different beast than the pure core functionality of setting up languages and locales. As long as you setup language and locale configuration, you can happily use a totally different module and way to actually translate, import, and export strings. And as the http://drupal.org/project/l10n_client module proves, it's also not really "administrative" functionality (in the sense of living on an admin/* path). So that current code only lives in locale.admin.inc, because we have a poor UI.

2. Split language, negotiation, and locale configuration functionality into locale.language.inc.

   That would be on the assumption that all of this functionality should actually be part of a new and separate Language module. And if/when that module exists, we'd move and rename that entire locale.language.inc more or less as-is into language.admin.inc.

In short,

- I'm against splitting for the pure sake of splitting. When splitting, there should be a larger sense for doing so.

- I'm also opposed to renaming "language negotiation" to something else. The term is kind of an industry standard, which naturally involves detection and selection.

I guess that larger sense is the D8MI. Gábor clearly states that this would be the first of a series of refactorings to more clearly separate and clean up these features. And as long as a large refactoring can be split up into more smaller ones, that is the way to go. Even if such an individual small refactoring makes no real sense on its own. Overall it will be easier to test, and trace errors when they are introduced during the time it takes to complete a large refactoring.

@fietserwin:

1. The "larger sense" cannot be D8MI, because that's just a container for various topics.
2. We're fully aware of the proposed series of refactorings, but that doesn't remove the necessity of ensuring sanity in every step involved. It doesn't help if individual changes are moving in different directions.
3. No one claimed that this refactoring here wouldn't make sense on its own. I've stated that the currently proposed split doesn't make sense. Because it's only touching a very small subset of code that actually belongs to a larger subset, and may not be in line with the other change proposals currently being discussed.
4. Moving these functions has absolutely zero impact on testing, tracing of errors, or whatsoever. The functionality itself properly implements core APIs already. The proposed change is "cosmetical"; pure moving of code only.

1. + 2. Given that we're still hashing out conclusions in the thread you linked to, it doesn't sound wise to me to perform an "arbitrary" code re-organization in the mere hopes that it's going to be in line with the final decisions and direction. We should properly figure out how to cleanly separate monolingual/multilingual code logic first, before attempting to move individual parts here or there.

3. Field API's separation was never agreed-on in a community-wide discussion. The separation was introduced as part of the initial Field API patch, and that one was so large that this detail didn't really gain attention. As a developer who tries to find certain stuff relatively often in those files, in particular Field module's organization is often confusing and sub-optimal. Lastly, there's also an I/O performance factor involved in every file include. So by all means, this shouldn't serve as a good example to follow (yet). As of now, the community-wide agreed-on standard is $module.admin.inc and $module.pages.inc (where applicable).

4. Right now, I'm seeing much more architectural design agreement on the suggested splitting of gettext/import/export related stuff (as in the other issue), as well as on the topic of string translations/l10n_update/l10n_client. It might also very well be that these topics are just simply easier to figure out, since they don't have system-wide implications. But anyway, in light of that, it would make much more sense for me to do option #8 1) then, "Split string localization/translation functionality into locale.pages.inc or locale.translation.inc." The net result is going to be more or less the same: locale.admin.inc vastly decreases in size. The difference IMHO is though that there's a much less vague plan for what to do with that localization UI admin functionality in the long run -- whereas I currently fail to see a sign of a solid concept for the monolingual/multilingual topic yet; too much to hash out API level wise and UI level wise in order to even think about which code should live where, what interdependencies might exist, etc.pp.

Does this mean that you disagree with splitting away the string localization admin UI functionality instead?

• locale.module » API functionality
• locale.admin.inc » Language, negotiation, locale configuration UI
• locale.pages.inc » String localization UI (+ l10n_client stuff, if that's on the table at some point), mayhaps moved into translation.translation.inc (or not) at some point.
• locale.bulk.inc » gettext mass-import or export UI