Merge List field types into Options module

Ouch.

I can't find the issue, but somewhere there's an issue to rename locale module, which is going to run into the same upgrade path issue as this one.

lists makes sense to me.

+++ b/core/modules/system/system.installundefined
@@ -1888,6 +1888,13 @@ function system_update_8010() {
 /**
+ * Rename list fields to lists fields
+ */
+function system_update_8011() {
+  // Don't know how to do this but something has to happen here

That's basically just a db_update that renames list to lists I think, also the filename.

The problem is that updates from list(s).module will only be detected after this, which means you'll need to re-run update.php. Will also need a dependency on this one. Maybe we can do it in the update_to_d8 function that's run extremely early?

Also, I guess most tests just failed because the module name needs to updated in setUp().

Since we usually don't do plurals (I can see why this would be an exception), what about listing.module?

Could more information be provided as to why Drupal\list is running into problems? What other words are we likely to hit this with?

http://www.php.net/manual/en/reserved.keywords.php

list is reserved because of the list($a,$b,$c) = $array stuff.

Edit: So at least print.module and echo.module are other candidates that will run into this.

Another way to solve this would be to introduce the discussed namespace=something to .info files because then list.module could define it's namespace as list_module or something like that.

I'd prefer not to rely on the namespace thing in .info for this, that was discussed for registering extra/custom namespaces, not to replace the main one, and I'm still not comfortable with the idea in general.

We should document that starting D8 you shouldn't name a module like a reserved word. If you don't have any classes that would still be possible, but since that will be the edge case in D8 I assume, I think we should generally warn about that.

This is a rather unfortunate architecture limitation of PSR-0. I have two major concerns here: a rename like this, with the current update system is going to be painful. I recommend putting the update in field.install... also, the print module maintainers will be very happy too.

In hope of getting this issue a little bit more back on track, I still like listing.module. What do you guys think?

Almost all of the other PSR-0 patches are now committed, so this is quickly becoming a blocker.

I will see if I can lead a bikeshed session here at the #d8mi #drupaldevdays sprint to put this sucker away.

If #1591852: Convert field tests to PSR-0 was in first, this patch would be much easier.

Done. :)

Btw, just to explain a bit more about what this module does:

name = List
description = Defines list field types. Use with Options to create selection lists.

/**
 * Implements hook_help().
 */
function list_help($path, $arg) {
  switch ($path) {
    case 'admin/help#list':
      $output = '';
      $output .= '<h3>' . t('About') . '</h3>';
      $output .= '<p>' . t('The List module defines various fields for storing a list of items, for use with the Field module. Usually these items are entered through a select list, checkboxes, or radio buttons. See the <a href="@field-help">Field module help page</a> for more information about fields.', array('@field-help' => url('admin/help/field'))) . '</p>';
      return $output;
  }
}

function list_field_info() {
  return array(    'list_integer' => array(
      'label' => t('List (integer)'),
      'description' => t("This field stores integer values from a list of allowed 'value => label' pairs, i.e. 'Lifetime in days': 1 => 1 day, 7 => 1 week, 31 => 1 month."),
...
    ),
    'list_float' => array(
      'label' => t('List (float)'),
      'description' => t("This field stores float values from a list of allowed 'value => label' pairs, i.e. 'Fraction': 0 => 0, .25 => 1/4, .75 => 3/4, 1 => 1."),
...
    ),
    'list_text' => array(
      'label' => t('List (text)'),
      'description' => t("This field stores text values from a list of allowed 'value => label' pairs, i.e. 'US States': IL => Illinois, IA => Iowa, IN => Indiana."),
...
    ),
    'list_boolean' => array(
      'label' => t('Boolean'),
      'description' => t('This field stores simple on/off or yes/no options.'),
...
    ),
  );
}

So in this context, I don't think "listing.module" makes much sense. I would expect "listing.module" to basically be Views. :)

It's actually kinda more like keyvalue.module, but I would expect that to be a key/value storage.

Naming is hard. :P

"field_list"? Since it's adding list capabilities to field module?

Eh. I thought about that, but we don't call it "field_image", "field_file", etc. so this would be inconsistent.

Same, btw, with 'lists' which otherwise would be a no-brainer replacement. We might want to do 'lists' anyway, however. :P

When has Drupal been consistent? :P "lists" sounds like fun to me. The namespace seems open.

... We could rename them all to "field_*" to help make Drupal consistent. Supporting the upgrade path via field module, like chx suggested, shouldn't be difficult.

Ok, I'm going to recommend we go with 'lists.'

Marking RTBC to get some more eyeballs on this.

DamZ suggested listfield (note the field suffix). I'm happy either either one. It really doesn't matter what the name is, as long as it's not "list"... In the mean time, we'll still need a patch here, and have Views to consider.

Oh. Listfield. If anything then that. Much better than lists.

Really what this module does is fields that give you options (selects, radios, checkboxes, listboxes). I have always thought that "List" module is confusing in the context of fields -- I don't think of them as lists at all. If you need to change the name of the module, I would vote for Options if that is an ... option. :)

Well, options.module already exists. But I long wondered why list and option weren't one and the same. List provides no widgets of its own, and option provides no field types. Why not merge them?

Actually that's a very good question, and would not only solve the problem but would eliminate a weird UX confusion of having two modules that are very similar.

Maybe ping yched/KarenS, or git blame?

Well, they were added in #361683: Field API initial patch as separate modules, so no help there.

AFAIK, the D6 equivalents of list_integer and list_float were in the number.module and list_text was in text.module (I think the functionality of list_boolean was just the default for optionwidgets).

So it seems that all of the allowed_values multi-select-ish widgets from various modules were abstracted into one list.module. But the leap to simply combining it with options module never happened.

Merging with options sounds good in and of itself, I'd completely forgotten they were different modules..

Ok, let's change the proposal to this then.

The only reason I can think that these are two different modules is because option widgets can be used on other fields than just list fields.

Is there anything stopping us from changing options module without changing the field type name? Ie. provide list_boolean field type. This is not a user exposed thing.

To be technically proper, yes, we should namespace the fields by the module name.

However, in order to unblock this major I personally would be comfortable moving them over as-is and handling renaming + upgrade path as a "normal" follow-up task.

Here's an initial untested attempt to merge the two.
I didn't attempt to rename the field types, as webchick suggested, nor did I PSR-0-ify the tests. Just a straight move.

Whoops, I forgot about the entry in standard.info.
This installed locally for me.

Okay, got the tests running. Still a couple failure/exceptions, they didn't really make sense to me right away.

@aspilicious I don't think that's what's causing the failures.
I haven't tried to change the names of any of the field types, since that would mean a full upgrade path, and ideally this will require only a minor switch of the field type's module.

One more attempt.

Oh, missed one instance of 'list' in a setUp. That's what caused the error aspilicious mentioned.

The ingenuity of our community to solve the insurmountable looking problems with ease again amazes me. Hats off to jhodgdon and tim.plunkett for coming up with the options merge.

The exceptions are all due to $field['settings']['allowed_values'] being a string, haven't really looked at the rest of the failures (down to 7!).

Taking a break for now, but I'll be back if no one else finishes it first.

This should pass, two small changes:

- value callback on the on/off form fields wasn't renamed, that caused weird allowed values thing.
- The dynamic values test assigned the values to a non-existing field.

I've seen other field_list and similar names in there, we might want to change them as well.

We'll need upgrade tests when switching over the field types. Nothing we've done so far needs an upgrade test.

Here's the PSR-0 versions. I think this should go in as is, and there can be a non-major follow-up to migrate the field type names.

Also, somehow this magically worked out to be the exact same number of lines added and removed, even with the PSR-0 stuff. Kinda cool!

Is the list.module entry in the system table removed automatically if it doesn't exist anymore? If not then I guess that's something that we need to do in an update function, I guess in options.install.. ?

We can add that in if need be, I'm not sure that we need it.

+++ b/core/modules/field/modules/options/lib/Drupal/options/Tests/OptionsDynamicValuesValidationTest.phpundefined
@@ -0,0 +1,56 @@
+use Drupal\options\Tests\OptionsDynamicValuesTest;
+use Drupal\field\FieldException;
+use Drupal\field\FieldValidationException;
+use Drupal\field\Tests\FieldTestBase;

No need for use statements for FieldTestBase and FieldException (if I didn't miss something).

+++ b/core/modules/field/modules/options/lib/Drupal/options/Tests/OptionsSelectDynamicValuesTest.phpundefined
@@ -2,17 +2,17 @@
+ * Definition of Drupal\options\Tests\OptionsWidgetsTest.

Should be (...) OptionsSelectDynamicValuesTest

+++ b/core/modules/field/modules/options/lib/Drupal/options/Tests/OptionsSelectDynamicValuesTest.phpundefined
@@ -2,17 +2,17 @@
+use Drupal\options\Tests\OptionsDynamicValuesTest;

Not needed because we're in same namespace.

Well #1627350: Patch for: Case of acronyms in class names (SomethingXSSClassName versus SomethingXssClassName) hasn't been decided yet, so I'm not changing this to "Ui" until it is. ;)

Fixed the rest, thanks!

Thanks :) Found one outstanding issue.

+++ b/core/modules/field/modules/options/lib/Drupal/options/Tests/OptionsDynamicValuesTest.phpundefined
--- /dev/null
+++ b/core/modules/field/modules/options/lib/Drupal/options/Tests/OptionsDynamicValuesValidationTest.phpundefined

+++ b/core/modules/field/modules/options/lib/Drupal/options/Tests/OptionsDynamicValuesValidationTest.phpundefined
+++ b/core/modules/field/modules/options/lib/Drupal/options/Tests/OptionsDynamicValuesValidationTest.phpundefined
@@ -0,0 +1,53 @@

@@ -0,0 +1,53 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\options\Tests\OptionsDynamicValuesTest.
+ */

Definition of... OptionsDynamicValuesTest -> OptionsDynamicValuesValidationTest.

Sorry for coming late to the party, I'm on the road and semi-afk for the time being.

The list/options modules split comes from the CCK era.
The 'list' field types were just options within the number and text field types ("restrict to a given list of alowed values"). This "allowed values" feature was more of an afterthought added on top of the original text/number field types back in the early CCK days (D4.7 ? D5 ?), but didn't integrate very well conceptually. At some point during the D7 Field API overhaul, it was decided that this behavior was specific enough (+ required specific logic, and lead to inconsistent combinations - a "formatted" text field with a "list of available values" ??) to be moved to dedicated field types - "list" field types.
Options.module only provides generic (field-type agnostic) widgets, free for any field type that see fit to use them : in D6, text & number (only relevant if they had "allowed values"), node/user reference... In D7, taxonomy fields use options widgets too (and taxonomy.module requires options.module).

That split still makes sense IMO. There is no real reason that, in order to get select widgets for your taxonomy fields you need to enable the "text/number list" field types - especially if this move is motivated by "mundane" namespaces / reserved keywords issues...

This being said, I don't really have a better suggestion for the namespace / keyword issue right now. And I agree that "options.module" is currently a little hard to identify for end users, since its only visible effect is to enable a couple widgets for a couple field types - which is not a common pattern in core. Pure widget modules are more frequent in contrib, but they usually target a specific field type (file, taxonomy...).
OTOH, in the proposed approach, the main visible effect of options.module would be to enable list field types. It's not going to be much clearer that it also enables widgets for, say, taxonomy or reference fields.

Just a note, they're both currently part of the standard install, so to the majority of users it will not matter that one goes away.

If we do want them to be two separate modules... The thing is, "List" still makes zero sense to me as the human-readable module name. Really it's a module that allows you to define the static allowed values for an options widget, and "List" just doesn't get the point across.

So if we do want them to be separate (i.e., if the baggage of allowing for sets of static values if you enable the options widgets is too much), then I would vote for a name more like:
- Static Allowed Field Values [loooonnnnggg name, but gets the point across of what it does]
- Allowed Value Sets (or Allowed Value Lists? I guess technically they are lists in the sense that they have an order, rather than sets)
- Allowed Values for Options
- Option Values [this one would be my favorite -- then it kind of gets across the point that it's for use with the Options module?]

Thoughts?

#60: drupal-1592632-60.patch queued for re-testing.

OptionsDynamicValuesTest -> OptionsDynamicValuesValidationTest

The diffstats and patch sizes are drastically different from here to #60, but I confirmed that Rob and I just have different versions of git, and they handle renames differently.

Here's the interdiff as proof. (note the size!)

I'd much rather just merge them (patch above!) but if that's not an OPTION I vote for "Option Values".

How do we gather consensus and move forward here?

FWIW: The reason why @Rob Loach's patch is larger is because the diff was generated with a too low minimum similarity for the rename detection. git's default value is around 50%. His patch shows renames based on similarities of ~10%. A more sensible minimum value to use is 25-30%. (The value can be specified along with option; e.g.: git diff -M30%)

I was unhappy with the location of the moved code from List module. Typically, I expect field modules to define their Field API integration functions in a sensible order; i.e., field (type) info, field settings, field CRUD hooks, field widget, field formatter. As a coder who is not using any IDE, I really appreciate it when code is structured and grouped cleanly. I therefore adjusted the patch.

While doing so, I realized that some of the field settings form #element_validate and #value_callback helper functions are using names that do not really make clear that they belong to the field settings form. I adjusted their names.

Without the latter, I wouldn't have provided an interdiff, because the functions are just moved around. So to clarify the latter, I'm attaching the interdiff for those renames, before I moved the code.

Aside from that, this looks RTBC to me.

Ah yes, I'm definitely on board with more sane organizations of functions. I didn't put any thought/time into that.

Those function renames look good.

Looks RTBC to me too! Is anything missing in this patch?

The only remaining blocker for this patch is the Field API maintainer's objection ;-)

@yched: Thanks for providing insight and feedback! :)

1. It's definitely a good point that Options is a specialized implementation of list type fields already.
2. I also agree that a reserved keyword syntax error alone and on its own would be a very poor argument for destroying a previously clean separation of field type and field widget modules.
3. However, there is no module in Drupal core that only depends on List, but not on Options. If I understand you correctly, then a field module like Taxonomy would be one of the candidates that could. But Taxonomy also provides an alternative SELECT (options_select) element based widget, so it depends on Options, not on List.
4. So even though I can perfectly understand the architectural desire to keep things cleanly separated, I assume that the situation with other field type/widget modules looks fairly similar. While they could depend on List only, alternative use-cases of the field type/widget module can very quickly/easily demand for a very simple widget, which is provided by Options. I.e., in the same way as Taxonomy supports a select widget - which doesn't seem to make sense at first sight, but of course it does, when your particular use-case foresees a short and fixed list of options.
5. One chunk of sub-functionality, the allowed values stuff, actually does not feel really clean to me either way. While it is true that it's rather related to the field type (List) instead of the widget (Options), the functionality would make much more sense as a generic "plugin" that is tacked onto any kind of field on demand; i.e., not limited to lists and options in any way. As such, the code would rather move into Field module at some point. I'm not sure whether you hinted at that in #62, so I'm not sure whether I'm merely repeating you in different words ;) Overall, however, I don't think this merger would make that situation any worse or better.
6. The UX perspective on field type/widget/whatever modules is anything but usable, so I don't think it really makes sense to discuss this aspect here. Either way, removing a module, which does not do anything, from the list of available modules is definitely a +1 (again, relative to the current mess of UI/UX). At the same time, "Options" is total nonsense in terms of module name, because what does it do? "Do I get more module options if I enable Options?" It's similarly bad DX-wise, because seriously, "Options API"? Functions to handle options? hook_options_list()? huh? #type "options_select"? Is that an optional select element? _options_get_options()? ugh. ;) However, bashing on and renaming Options module to something that makes more sense seems to be out of scope for this issue. I'd love to see such a follow-up issue though. :)

In retrospective, nothing in #72 seems to be a total show-blocker.

However, there's a real bug in this patch: Options module cannot perform the update from List to Options module, because Options module might not be enabled at all.

Fixed in attached patch; by moving the update from options.install into field.install.

Better title. Can we move forward here? This is the last blocker for removing the registry, and I think this is RTBC.

@yched raised some general concerns in #62, which I think have been addressed in #72:

tl;dr: The allowed values functionality should be abstracted/plugin-ified when that is possible, but otherwise, this merger does not introduce architectural problems. "Options" was and is a poor name, but improving that is out of scope for this issue.

Echoing what yched said, the reason for the list module was basically to provide a vehicle for allowed values lists. It was separated out because the original architecture munged together bare-bones text and number fields with the whole concept of lists of values. I think the reason for pulling that out makes sense. I'm not sure that we have any reason for an allowed values API outside of the list field. What else would use it besides the list field? The list module *is* the allowed values API.

Options was originally called Option Widgets, which made a lot more sense. But it got truncated somewhere along the line into the much less understandable name of 'options'. The idea there was that it might become a whole collection of ways that you could view multiple values. And those widgets could work on things besides 'list' fields, so it made sense for the module to stay separate.

However I guess the original concept has fallen apart somewhat. I suppose there is a valid case to combine them into a module that provides a field that can contain lists of values and a widget that can display lists and an API for creating allowed values lists. The original name of 'list' makes the most sense for this functionality. Dumping this all into an 'Options' namespace seems somewhat more confusing. So I don't object to the general idea, but I think the name choice is just going to create further confusion in the future. I'd go back to some variation of the word 'list' because that is far more clear than 'options', IMO. Something like 'option_list', or 'listing' or 'value_list' or maybe even 'allowed_values', since this basically is the API for allowed values lists.

However I do agree that allowed values should be pluggable. I may have misunderstood that argument, I read this to say there was a proposal to create a separate allowed values API somewhere else, but reading back I may have misunderstood.

Between #72 and #75, I think there is consensus on combining the two modules.

I opened #1677052: Rename the Options module (which contains the former List module as well) as a follow-up to rename it.

Tentatively marking RTBC.

I do agree with @sun's assessment, but this could still use a sign-off from yched, IMO. I guess we can leave it at RTBC, but maybe one of the issue queue overlords can assign it to him.

No overlords necessary any core maintainer can.

Assigning to yched based on the above and @tim.plunkett's request.

@chx: That's what I meant. If I sounded negative or condescending that wasn't my intention, sorry. Not a native speaker.

AFAIK, @yched pretty much went for AWOL this year/summer, so I'm not sure whether it's the best idea to defer/delegate to him, as it could easily take some weeks until he comes back from rocking the stages of this world. :)

I'm also not sure whether @KarenS' feedback was really 100% in line with what we're proposing to do here. In an attempt to read between the lines, I've the slight impression that the comment might also meant to say:

duh, folks, you have no idea at all, List module is the allowed values API, and what you're proposing here will make Field API's architecture considerably worse.

:-D In other words, I do not really have the impression that Field API maintainers are fully on board with this change proposal.

What are allowed values?

An allowed value is a value of a selected list of valid values; a selection.

Alternatives for: "selection"

Main Entry:     assortment
Part of Speech: noun
Definition:     variety
Synonyms:       array, choice, collection, combination, combo, diversity, garbage, group, hodgepodge, jumble, kind, medley, miscellany, mishmash, mixed bag, mixture, mélange, potpourri, selection , sort 

So let me present you this alternative. :)

6073f06 Renamed List module files to Choice module.
0326104 Renamed "list" to "choice" (where appropriate; i.e., not for list of allowed values).
e0dceb1 Changed dependency in Standard profile.
8d7ee18 Fixed OptionsSelectDynamicValuesTest.

The idea of merging 'List' and 'Options' into a module that is designed to handle everything around the process of creating a list of allowable values and displaying it is acceptable to me. I was somewhat confused about the discussion about an Allowed Values API. I agree allowed values really ought to be pluggable, but if the proposal was to move a 'allowed values' API somewhere else I disagree because that is the fundamental purpose of the List module in a nutshell.

The current patch is merging them into the Options module, and I don't like that name for the new combined functionality, but there is now another issue about fixing the name.

The thing lost with this direction is the ability to use an option list widget without enabling the whole list field/allowed values functionality. But it's probably likely that most of the core field modules will be enabled most or all of the time anyway. So maybe that's an acceptable loss. The main place that comes up now is with reference fields (node or entity reference). If one of them is added to core the reason for the current architecture might seem more reasonable.

So with that all said, I think the patch in #73 is an acceptable direction -- combine the modules and then fix the name in another issue. I can't speak for yched of course :)

Alright. I (still) agree, and I think that is logical and makes sense.

In case "Choice" is a much better name, then we can still rename Options to Choice in #1677052: Rename the Options module (which contains the former List module as well) -- the order of renames makes no difference at all.

OK committed/pushed this to 8.x. I think we might want to rename options module but let's discuss that in the other issue. I don't have a particular problem with merging the functionality - if and when we have an entity reference module in core that's not remotely using the 'list' stuff we could always revisit that too.

Adding change record

@eddie_c: It looks like the change record for this issue also includes the changes from #1677052: Rename the Options module (which contains the former List module as well) which haven't been committed yet? Also, it would help if you could provide a link to the change record you've created.

@eddie_c Thanks for your help on this, but unless I misunderstand, the patch in #82 was not the one Catch committed, but the one in #72 which merges the list module into the options module.

Should we also mention that going forward, folks shouldn't name a module with the same name as a reserved word?

Alright, I've updated http://drupal.org/node/1691614

We need an update hook here -- any field that was created prior to this change in D7 is going to have 'list' as the module name in the field_config table. This won't be an issue until people start upgrading, but it needs to be addressed.

I'm creating a follow-up issue for this, #1697408: Update module name in field/instance config for List fields

I think I stumbled on some orphaned code from this issue, and opened a followup to remove it: #2012130: Regression: Views integration for "list" field types is broken. If anyone here has insight on whether it's safe to remove that code, or if we need to replace it with something, please comment on that issue. Thanks.

Followup: #2020691: Misleading module description for Options