Problem/Motivation
Unable to add list field types in views
In #1592632: Merge List field types into Options module, we moved List module to Options module. However, it looks like some traces of the former List module code remained in Field module. I don't know if we need to replace it with anything, but I think the current code is both broken and not being called from anywhere.
Proposed resolution
Patch the Views Module by allowing it to include list field types by:
-Creating new field plugins for rendering options list fields
-Creating a new FieldSettingsTrait to allow views plugins to easily fetch settings for a given field
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
None. | Instructions |
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#145 | interdiff.txt | 2.89 KB | dawehner |
#145 | 2012130-145.patch | 39.9 KB | dawehner |
#141 | interdiff.txt | 789 bytes | dawehner |
#141 | 2012130-141.patch | 39.78 KB | dawehner |
#82 | interdiff-2012130-82.txt | 8.55 KB | damiankloip |
Comments
Comment #1
smiletrl CreditAttribution: smiletrl commentedIn field.module
function field_help($path, $arg)
It mentions List module is required. Probably it needs deleted too:)This patch removes two views arguments and one filter. It doesn't smell right to me, because I can't find the replacement at other places, including options module.
On the other hand, I tried to create 2 list fields (List--integet and List--text) and use the to fields as contextual filters. I thought that two arguments should have some impact on this. But it didn't. Everything looks the same after I applied this patch. I didn't find the settings at the two arguments, e.g.,
I didn't find this checkbox setting for both two fields I just created.
So my question is in what context which argument will be used? Ididn't find the answer, even for this Date argument It is obviously used for time-field, but I can't see the context in code that this argument should be used.
So is this patch. I can't tell how/when/where the three plugins should be used. So I can't tell should they be removed.
Comment #2
smiletrl CreditAttribution: smiletrl commentedAdding tag
Comment #3
yched CreditAttribution: yched commentedThe blame annotations on
function list_field_views_data($field) {
(merlinofchaos 2009-05-17 "Add the 7.x-3.x Views branch") seem to indicate that this comes straight from the direct merge of the D7 views code that was done at the beginning of the "VDC / Views in core" effort.list.module has been removed/merge into options.module in july, so probably before the VDC work started.
So this most probably never worked in D8 - but it's still something that's *meant* to be there...
Filters on List fields need to be able to provide a list of available options for the admin UI (or exposed filters).
Less sure about what the custom argument handlers do - at least they provide the text replacement in the view title when the argument is provided ('option_1' --> "The human label for option_1"
Comment #4
yched CreditAttribution: yched commentedAlso, I guess the list_field_views_data() function currently lies directly in field.views.inc rather than in options.module (and the associated handler classes live under field.module's lib folder) - as opposed to, say, image fields views integration being provided by image.module, is precisely because list.module had disappeared when the views folks imported the code.
@effulgentsia: so this currently never called.
The right fix would be to move that function & the classes over to options.module, rename to options_field_views_data().
But then it would be active again.
What do you think is easier on #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets ? "unbreak" this now, or leave it broken and care about it after #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets ?
Comment #5
yched CreditAttribution: yched commentedMore accurate title.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedI don't plan on working on this until after #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, but if someone else gets this fixed before then, then I'll update that issue to work with it.
If this is a regression from something that works in D7 Views, this probably needs to be a critical bug. Unless it's considered a minor enough thing that it can be deprioritized, but I'll leave that call to others.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedOh, and setting status to "active", since the original patch isn't remotely the right solution :)
Comment #8
smiletrl CreditAttribution: smiletrl commented@yched has got the point -- Perhaps list_field_views_data should be renamed and moved to options module, which explains why the three views plugins not working. But in D7, they work well.
@effulgentsia I will try to fix this according to instructions at #3 and #4, if this makes your life easier:)
Comment #9
smiletrl CreditAttribution: smiletrl commentedOk, this patch works well for me locally. Let's see if it pleases bot.
Comment #10
yched CreditAttribution: yched commentedThanks @smiletrl !
Code looks reasonable. I'll let @effulgentsia chime in whether it makes things easier to fix this now or later.
FieldList / ListString - not too consistent. That was already the case, but maybe we should fix that while we're here ?
What about:
NumberListField extends Numeric
TextListField extends String
ListField extends ManyToOne ?
Shouldn't there be a matching @endgroup then ?
Does this really belong here ?
If the previous code is incorrect, then it looks like other existing filters are currently broken ?
Comment #11
smiletrl CreditAttribution: smiletrl commentedThanks for review!
1) Yeah, we could do better with name consistency. But should we add module name prefix, like what taxonomy module does? like
OptionsNumberListField extends Numeric
OptionsTextListField extends String
OptionsListField extends ManyToOne ?
I hope the length wouldn't be that large:)
2) Although '@endgroup' sounds reasonable, it's not in the list https://drupal.org/node/1354:( I did a " grep -r '@endgroup' ." It returned nothing. @Jthorson in irc isn't sure about this either, but @amateescu is pretty sure the answer is NO:)
3). The previous code will report a bug for this kind of list fileds, it will say 'query' is not a defined property for this object, and 'add_where_expression' is trying to work on non-object. Of course, the views is broken too. I tried taxonomy field's filter to test this code, because its field filter also extends "ManyToOne". It reports the same error.
Take a whole picture of this class, ' $this->query->add_where_expression' looks more like a typo, because '$this->handler->query->add_where_expression' has been used everywhere:) I see $handler is used as a dynamic property of this class, it's not optimized? So I declare it as a property in the first place.
But when I test taxonomy field with views, I find another bug? See #2017829: "Show hierarchy in dropdown" doesn't work for taxonomy field views filter.
Comment #12
yched CreditAttribution: yched commentedNot sure prefixing the class name with the module name is required. What do the other "custom" views handlers to ?
@endgroup - ok, sorry about that, I learned something :-)
$this->query->add_where_expression:
Yup, it's a bit tedious because you can't really test the (re)introduced 'list fields" filter handler without fixing this bug too, but it really looks like a fix that would deserve its own issue...
Comment #13
smiletrl CreditAttribution: smiletrl commentedOk, I'm trying to split that views helper issue to #2017939: Views' helper ManyToOneHelper has a typo?, since taxonomy term field reports the issue too:)
This issue will focus on the list field thing. Rename these classes according to #2012130-10: Regression: Views integration for "list" field types is broken.
Comment #15
smiletrl CreditAttribution: smiletrl commented#13: views_integration_list_field-2012130-10.patch queued for re-testing.
Comment #16
smiletrl CreditAttribution: smiletrl commented#13: views_integration_list_field-2012130-10.patch queued for re-testing.
Comment #17
dawehnerThis is the proper way to go. In general please make a patch with using git diff 8.x -M, because this really allow the patch to decrease,
and people get not distracted by missing proper docs in all the views files.
Should be Contains "\" ...
Let's remove "and handlers" as it is not longer true (sorry that is d6 comment code.
Comment #18
smiletrl CreditAttribution: smiletrl commentedok now thing is a bit tricky for me. Ignore views test for now,
See
This is views filter plugin for list field. As for options_allowed_values, it requires $field_definition and $entity to get value. So we need to adjust this function here.
For $field_definition, I think we could use field instance(this could depend on bundle too!) for this field. For $entity, we need to get every quried entity object for this views. Is that even possible in views, dynamic options? Now I understand why @effulgentsia wants to push field_definition into core firstly:)
Any idea to proceed?
Comment #19
smiletrl CreditAttribution: smiletrl commentedFor $field_definition from #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, shall field_definition be independent from entity bundle? I think it should be, since it's called field-definition, instead of field_instance_definition:)
But, how to get a field_definition for a specific field name in this case, e.g., 'field_test_me'? The only way I could think of, is get a field instance of this field, then, I could use $field_definition = $instance to do what I want, which will depends on bundle again.
As for every queried entity object, e.g, every node this views is querying, here's a compromise way. If this field uses dynamic allowed value options, i.e.,
$function = $field_definition->getFieldSetting('allowed_values_function');
exists, we get back to the regular filter. otherwise, we still use the options here.I'd like to add reference of dynamic allowed options example here
Comment #20
smiletrl CreditAttribution: smiletrl commentedAlso, while options field supports dynamic allowed values, there's no way for users to add dynamic allowed_values_function from field UI. I'm thinking can we add some settings, so user could add such a function from field edit ui, without have to create such field from code?
Comment #21
smiletrl CreditAttribution: smiletrl commentedOpen this #2037217: Should we need dynamic options in options_allowed_values()?.
Also, some discussion in IRC
[23:04] dawehner_: smiletrl: we could just pass in the entity type and bundle as string to be honest to the API
[23:04] swentel: smiletrl: I don't see the flaw tbh - again, not following completely
[23:12] smiletrl: dawehner_: as you said, we could pass a fake entity type and bundle, this could work for most cases. One thing is, a field could show in different entiy type and bundle, we could see this from view context now. The other thing is pontentially, the options depends on each entity. when that happens, error will crashes views cruely...
[23:12] dawehner_: smiletrl: one major problem is: we don't have the bundle available
[23:12] dawehner_: smiletrl: all we have is the name of the field + the entity type
[23:12] dawehner_: smiletrl: but i always thought that options are defined by field not per instance
[23:13] dawehner_: smiletrl: the problem is that a filter potentially filters on different bundles
[23:13] tizzo-afk is now known as tizzo
[23:13] dawehner_: smiletrl: and the field has an instance on different bundles
Technically speaking, yeah, no way to accurately tell which bundle this context is in, since there could be bundle filter in views too. At this point, as @dawehner said, we could dismiss this dynamic options feature?
Want to see what @amateescu, @swentel @yched thinks:)
Comment #22
smiletrl CreditAttribution: smiletrl commentedOk, here's the idea of how to proceed.
This approach will ignore special options fields, which makes use of dynamic allowed values, for views.
If an option field makes use dynamic option fields, there could be no way to pre-define options for views, because we can't predict what will happen inside dynamic allowed function, imho.
Comment #23
smiletrl CreditAttribution: smiletrl commentedIgnore views test for now.
This patch is trying to fix options_allowed_values issue. It's designed according to comment #22.
Comment #24
dawehnerThis is a great start so far! Here are some nitpicks and comments to the actual important code.
nitpick: Files should be started with "Contains \..."
nitpick: this should have a @inheritdoc on there.
I am wondering whether similar to the other function this should already receive a Field entity.
THis exception is not "use"d in the file, so it will fail really if it fails.
Let's skip cacheable, as in this case the values are not dynamic.
If it uses dynamic values we could at least provide a pure string based filter/field
Comment #25
smiletrl CreditAttribution: smiletrl commentedThanks for your review!
Ok, new patch according to #24.
As for
This will fallback to default views filter for string. Right now in Drupal core HEAD, this is the case. Our patch will provide the ManyToOne options for views filter and arguments, provided the options list field is regular field.
Comment #27
smiletrl CreditAttribution: smiletrl commented#25: views_integration_list_field-2012130-24.patch queued for re-testing.
Comment #28
dawehnerThanks!
This change seems to be optional and kind of out of scope, but for sure it make sense to replace list with option here as well.
You could also inject the field_entity storage controller via the create() method to be able to load the field entity.
Comment #29
smiletrl CreditAttribution: smiletrl commentedHmm, if use entity_create, or storage,
We need to provide other information of this field(Id, and type are required), not just a field name. I guess simply loading the existing field is okay:)
Comment #30
dawehnerI meant something like this:
Comment #31
smiletrl CreditAttribution: smiletrl commentedThanks for the head-sup!
Hmmm, as I understand, this 'create' method is usually used inside a class to inject objects. In our case here, I'm not sure where to use this feature. Where can I inject the storage controller? Or you want me to create a new class?
Comment #32
smiletrl CreditAttribution: smiletrl commentedAdd test for options views integration. This is just a start, should fail:)
It only counts the regular options field for now. Perhaps we need to add tests for special options list field.
Comment #34
smiletrl CreditAttribution: smiletrl commentedComment #35
smiletrl CreditAttribution: smiletrl commentedComment #37
dawehnerI really like the amount of test coverage which is added by that patch.
Oh cool, there is a base test class!
This comment is not right anymore.
We could just skip this if else and setup the two entities before.
Test class names should always end with "...Test".
Oh cool, there is a base test class!
This comment is not right anymore.
We could just skip this if else and setup the two entities before.
Comment #38
smiletrl CreditAttribution: smiletrl commentedThanks for your review!
Ok new patch. This patch may pass, because it passes locally.
However, there're problems with this test OptionsListArgumentTest, but I can't figure out the problem.
This problem is this test should execute views
$view = views_get_view('test_options_list_argument');
, but it's verbose message of the execute view says different thing:This is exactly what it executes at test OptionsListFilterTest. It should be something like this:
This query is copied from test_options_list_argument views's displayed sql query.
Any ideas?
Comment #40
smiletrl CreditAttribution: smiletrl commented#38: views_integration_list_field-2012130-37.patch queued for re-testing.
Comment #42
smiletrl CreditAttribution: smiletrl commented#38: views_integration_list_field-2012130-37.patch queued for re-testing.
Comment #44
catchIsn't this missing a use? Assume that's why the test fails.
Nothing to do with taxonomy.
Comment #45
dawehner@smiletrl
You have to pass in the arguments in the $this->executeView call.
Comment #46
smiletrl CreditAttribution: smiletrl commentedThanks for your review!
@catch, I just found I used capital characters in namespace declaration for options base test, which probably leads to the problem:)
@dawehner, hmm, the first view -- views.view.test_options_list_filter.yml is a regular view, no contextual filters provided, do we need arguments for this view? As for the other view views.view.test_options_list_argument.yml, this view uses argument. But options argument class -- TextListField and NumberListField only add a summary options in views summary settings:'Display list value as human readable', which requires no arguments provided to take effect. So I think this view doesn't need arguments too?
Comment #48
smiletrl CreditAttribution: smiletrl commentedok, fix contain declaration. This is quite wizard, because the three tests share same namespace and it works locally.
If this fails gain, I shall add "use" as @catch said.
Comment #50
dawehnerSo you are using a mac, i guess this should solve the problem.
+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsTestBase.php
+namespace Drupal\options\Tests\Views;
See views vs. Views
Comment #51
smiletrl CreditAttribution: smiletrl commented@dawehner Great point!
I didn't expect namespace needs to match the real file directory:(
Comment #52
dawehnerJust in general, this is an impressive piece of work. Just have a look at the amount of
You should better inject the field_entity storage controller using the create method.
These kind of code should check whether ->value_options is already set.
I think you should add "Use Drupal\field\FieldException" at the top.
Comment #53
smiletrl CreditAttribution: smiletrl commentedThanks for review!
New patch according to #52. Some questions though.
1).
Which storage interface should be used to declare var, since FieldStorageController implements both EntityControllerInterface, EntityStorageControllerInterface? Is there any preference?
2). Compared to use
Drupal::entityManager()->getStorageController($entity_type);
directly, current code in patch inject entityManager. What's different here?Does 'Drupal::' means a Drupal current active container, but code could provide another/alternative container to provide a different entityManger? Kind of like drupal database connection, default connection is provided in settings.php, but code could provide alternative connection to connect to other databases.
3). The argument views test doesn't test what I want here, but I don't know how to do this in views.
The main function these two options field arguments have provided is the 'Summary' -- t('Display list value as human readable'). So I want to check whether executed result from views should contain options allowed values, not keys, e.g., should contain 'man' and 'woman', instead of key '1' and '2' for NumberListField.
But what I could get only contains node-id, not the pure text from views result. The test will pass, but passes in strange way. This is the verbose message for both tests:
I should separate test from fix to show whether the test really checks fix here.
Comment #54
dawehnerYou could argue that the deepest interface which has the methods you need, for example EntityStorageControllerInterface, if we just the load method.
The main point here is once you inject all the dependencies into an object, it is somehow self-documentation how to reuse the code. At the same time this allows you to write unit tests, as the object itself works together with its dependencies but independent from the other rest of the outside world.
So what you would do is to create a new view using the summary functionality in views and then ensure that the result of that special configuration is as expected. If you configure it, the result will differ.
Let's use String::checkPlain
As most of the views tests we are testing basically the functionality of a single class, so I really like to have a @see \Drupal\...\PathToClass, so it is really easy to figure out where to look at.
Comment #55
smiletrl CreditAttribution: smiletrl commentedThanks for your help!
This patch has applied some changes at #54.
However, this patch doesn't work for views. Options filter and argument is broken. I guess the reason is because of change mentioned at #52 to apply create method to inject container and storagecontroller. The way to instantiate these views plugins doesn't invoke create/inject way?
Comment #57
dawehnerThis would fix it.
Comment #58
catch#57: vdc-2012130-57.patch queued for re-testing.
Comment #60
smiletrl CreditAttribution: smiletrl commentedThere're still bugs there for the latest patch, which may involve the existing code of options field and taxonomy field. I'd love to wait untill options field and taxonomy field has been converted into the new FieldType plugin and then start to work at this.
I'll try to fix the two issue firstly, see:
#2015689: Convert field type to typed data plugin for options module and #2015687: Convert field type to FieldType plugin for taxonomy module
Of course, others are free to reassign this issue, if anyone would like to pick this issue too:)
Comment #61
jibranNeeds reroll
Comment #62
dawehnerRerolled ...
Comment #63
dawehner.
Comment #65
smiletrl CreditAttribution: smiletrl commentedThis patch intrduces
and especially this funtion
options_regular_allowed_values()
.It's interesting that taxonomy field doesn't need to consider this situation(I don't know actually how this is achieved^). It only searches the db to get all the tids inside the choosen vocabulary for $value_option. So I think maybe we could use the same trick here?
We could simply check whether list field uses custom callback. If it has, we return the NULL for value options. Otherwise, we use the common function to return the available options. In this way, we don't need this function
options_regular_allowed_values()
. This may relate with the converted new option fields issue.Comment #66
star-szrI can't speak to #65 but this could use another reroll it looks like.
Comment #67
webflo CreditAttribution: webflo commentedRerolled ...
Comment #68
webflo CreditAttribution: webflo commentedDoh, the last patch was broken. (core/modules/options/options.views.inc was missing)
Comment #70
webflo CreditAttribution: webflo commentedThis should work. Updated the tests to field_config and field_instance_config.
Comment #72
YesCT CreditAttribution: YesCT commented@smiletrl it's been a while. :) if you want to work on those fails, just comment back here. otherwise I think this is open for anyone.
patch applies, removing needs reroll tag
I viewed the test results. Fails (not random as they are in classes this patch adds) are:
Comment #73
pcambraThis needed re-roll after some changes in core, should be green but not sure if something else might be needed
Comment #75
pcambraJust a side note, tests pass locally both UI and command line
Comment #76
martin107 CreditAttribution: martin107 commentedHmm ... I can confirm #75 ... at least tests pass for me in local browser testing... so retesting.
Comment #77
martin107 CreditAttribution: martin107 commented73: vdc-2012130-73.patch queued for re-testing.
Comment #79
martin107 CreditAttribution: martin107 commentedrestest results ... unchanged... ( I am on a MAMP stack - just for information )
php 5.5.3
looks like testbot was PHP 5.4
Comment #80
damiankloip CreditAttribution: damiankloip commented73: vdc-2012130-73.patch queued for re-testing.
Comment #82
damiankloip CreditAttribution: damiankloip commentedHere are a few changes, quite a few things were broken with changes to core :) Alot of work still needs to be done.
The new tests were yielding broken handlers due to an exception thrown when trying to load the 'field_entity' entity type, which does not exist anymore. This should fix that at least. The tests should still fail (or actually pass). The arguments are not affecting the queries. The tests should also be checking that maybe just one of those entities is in the results, not both? or a combination with more node created for the tests.
We have a problem here:
options.module now just has options_allowed_values() which wants a field entity, and an entity to work out allowed values for. This is not really possibly for do for arguments and filters...
Comment #84
dawehnerOpened an issue for that, this is a bad thing: #2238085: [regression] options_allowed_values() signature doesn't allow for Views filter configuration
Comment #85
martin107 CreditAttribution: martin107 commentedComment #86
damiankloip CreditAttribution: damiankloip commentedThis is really postponed on the issue opened in #84.
Comment #87
mgiffordComment #88
xjmComment #89
YesCT CreditAttribution: YesCT commentednoted why this was postponed in the issue summary, but other details there need to be filled in.
Comment #90
mgiffordthis patch was going to need to get re-rolled anyways...
Comment #91
catchComment #92
xjm#2238085: [regression] options_allowed_values() signature doesn't allow for Views filter configuration is in!
Comment #93
dawehnerNo matter what we do here, but we need test coverage here, so we don't break that again.
Comment #94
mgiffordChecking #90 to see if it busts anything. Still needs new tests though.
Comment #95
damiankloip CreditAttribution: damiankloip commentedmgifford, did you see patches preview to #90, that 'reroll' is just a hunk of that patch. I'm confused. 31kb down to 1kb :)
Comment #96
damiankloip CreditAttribution: damiankloip commentedOk, rerolled #82, which has all the work and tests in. That is an 8 month reroll, so expect something to go wrong :p
Comment #97
dawehnerHA
FormState thing
That thing doesn't exist anymore
This is why I hate our 80chars limit.
Let's use protected $allowedValues here, it is a little bit more sane.
No bool needed anymore.
Comment #98
yched CreditAttribution: yched commented- This should work on a FieldStorageDefinition rather than a FieldConfig
- It should call options_allowed_values() rather than reading the 'allowed_values' setting.
Same here, this should be a FieldStorageDefinition
And same here - but not sure why this one uses options_regular_allowed_values() (which doesn't seem to exist ?)
Also - maybe that's not for this issue, but the pattern of reading the Field / FieldStorage definitions from config is a little weird, and very much hardcodes the various handlers to only work with configurable fields.
Handlers for Field API fields are about a FieldStorageDefinition, we should try to inject it, or at least read it from EntityManager::getFieldStorageDefinitions(), rather than pulling them from *config*.
It's true that currently field_views_data() only ever uses those handlers for configurable fields, but the less the handlers themselves know that, and the more they work with FieldStorageDefinitionInterface wherever it comes from, the better for the "use Field API handler for base fields" issues ?
Comment #101
damiankloip CreditAttribution: damiankloip commentedyched, dawehner, how about this? We make a trait with some field helper methods on (that the field Field plugin is using already) as reuse that in our args and filters?
Made some of those other changes too from points in #97. Haven't looked at the tests yet.
Comment #103
yched CreditAttribution: yched commentedYep, I think a trait makes tons of sense :-)
A couple adjustments then :
Not fully sure, but wouldn't that be more helpful in the phpdoc of getFieldDefinition() ?
"The field storage definition"
$fieldStorageDefinition
"Gets the field storage definition"
getFieldStorageDefinition()
s/$field/$field_storage/
Looks like this is not needed anymore ?
Comment #104
dawehnerAgreed a trait is helpful here.
Let's always use $this->t()
afaik we don't need hidden anymore, because the extension listing will not find it anyway
Can you try to recast all those entries?
We have to add entity_type and entity_field to all those new views configs.
In an ideal world we would use the entity_test instead of node integration.
Comment #105
dawehner.
Comment #106
damiankloip CreditAttribution: damiankloip commentedRerolling this with all that great feedback.
Comment #107
damiankloip CreditAttribution: damiankloip commentedComment #108
damiankloip CreditAttribution: damiankloip commentedUpdated some config typing and a couple of other things, should be less than 123 fails.
Comment #109
dawehnerLet's see whether @damiankloip lies
Comment #111
jibranspace missing.
Comment #112
dawehnerJust fixed stuff here and there.
Comment #114
dawehnerAlright, let's fix the failures and get some serious review done.
Comment #116
vladan.me CreditAttribution: vladan.me commentedHm, I have tested locally, didn't get a failure, let's try again?
Comment #119
star-szrI was going to run the tests myself but this no longer applies, non-trivial (to me) conflict with #2407801: Views generic field handler does not work with base fields.
Comment #120
pcambraHere's a reroll
Comment #122
dawehnerLet's fix it.
Comment #123
larowlanShould we put this behind a getEntityManager() method and add a setEntityManager() for unit-testing sake and consistency with other similar traits - or is that overkill?
nitpick : missing a space
I thought this was renamed to list_string - I can't find any reference to list_text in core anymore.
Yes it seems so
Comment #124
dawehnerThank you for your review!
Let's do that as well as use VIewUnitTestBase instead.
Comment #125
jibranI think this is ready.
more then 80 chars.
Comment #126
larowlanThanks, will try and get someone to look at the issue summary and manual tests during today's critical office hours
Comment #127
larowlanSome nits and one bug
Fixed #125 whilst at it
after manual testing and issue summary update, I think this is ready too
Comment #128
larowlanThis was the bug, the rest were nits
Comment #130
larowlanthought that might happen :)
Comment #131
dawehnerYeah this is stupidity of PHP 5.4
Comment #132
dashaforbes CreditAttribution: dashaforbes commentedManually tested as follows:
- Added a list(string) field and a list(text) field to the Article content type
- Created two sample Articles
- Created a view showing the Articles
View:
View Fields:
Comment #133
larowlanJust an issue summary update and I think we're there - any takers?
Comment #134
dashaforbes CreditAttribution: dashaforbes commentedComment #135
dashaforbes CreditAttribution: dashaforbes commentedMoving to RTBC as per comments 125, 127 and 133
Comment #136
alexpottMissing schema and, therefore, I think test coverage of StringListField.
Comment #137
dawehnerMh fair, let's work on that
Comment #138
yched CreditAttribution: yched commentedNitpick, not a blocker : the name FieldStorageViewsTrait is a bit vague, and the trait is not "just" about "field storage".
This is generic support methods for field API based views handlers, and for use by several kinds of handlers : fields, arguments, filters... So maybe FieldAPIHandlerTrait ? I know @dawehner and I have different, er, views :-p on class naming, so feel free to ignore...
It looks like this is the only difference between StringListField and NumberListField.
- couldn't those two inherit from a common one for all the rest of the code ?
- not sure why there is a difference, actually, since both those lines attempt to display a string label, independently of the underlying (string or number) value ?
Comment #139
dawehnerThat name is alright for me, ... there simply has been bad examples out there which copied basically the entire namespace into the classname.
Well, you missed one tiny little difference:
vs.
... all this functionality here is inherited from String vs. Numeric, the only common thing is that both are argument plugins, this is all.
Comment #140
yched CreditAttribution: yched commentedSelf-slap. Right, of course :-)
Yet, this remark from #138 is still valid IMO :
$this->allowedValues[$value] is a "human label" string. So I don't get why one would do a CaseTransform on the *label*, and not the other ? The labels, unlike the values, are the "same kind of stuff" (just strings for display) on both StringList and [Numeric]List field types.
Comment #141
dawehnerFair point. Here though, we just want to caseTransform the human label.
Comment #142
yched CreditAttribution: yched commented@dawehner: thanks :-)
Then, "two classes duplicating the exact same methods because they each have their own specific parent class to inherit from" is the textbook case for "should we make that a trait ?"
Comment #143
dawehnerWell, the implementation is still not the same ... As for the key case
Comment #144
yched CreditAttribution: yched commentedRight, I should have looked closer, sorry for that.
Down to just nitpicks then.
No preference between "to use" or not "to use", but consistency++ ?
That's a good thing to explain, IMO for clarity we could turn this into an actual sentence with a verb, and maybe expand a bit, since this is the crux of the Views / Field API issue :-)
Proposal :
A View works on an entity type across bundles, and thus only has access to field storage definitions. In order to be able to use widgets and formatters, we create a generic field definition out of that storage definition.
"Gets the field storage definition" ?
Minor, but for accuracy and consistency with the same code in NumberListField :
s/$field/$field_storage
Likewise, s/,/./
Comment #145
dawehnerThank you for your continuous reviews!
Yeah, let's just drop it.
I c&pasted your text.
fixed
Removed the caseTransform method again, because there is no way to configure it at the moment and so kinda pointless
Fixed.
Alright, next one :)
Comment #146
yched CreditAttribution: yched commentedThanks @dawehner !
From latest interdiff:
Unintentionally reverts #141 ?
Other than that, this looks RTBC to me ?
Comment #147
dawehnerUnintentionally reverts #141 ?
... nope, caseTransform just makes sense when you can configure the case ... but we don't expose that option for numeric handlers, and based upon that, calling it, doesn't makes sense, IMHO.
Comment #148
yched CreditAttribution: yched commentedHm. That gets us back to #141 - NumberListFild is a numeric handler, but it displays a string (the human label). Exposing the regular display options for that string could make sense IMO, but I can't really say I care that much :-)
Doesn't have to block the critical from getting fixed either. Let's get this in !
Comment #149
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 6ab60f7 and pushed to 8.0.x. Thanks!
Comment #151
damiankloip CreditAttribution: damiankloip commentedNice, thanks for picking this up guys!! I slacked off.
Comment #152
dawehner@damiankloip
So glad that you succeed in slacking :P