Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#107 | interdiff-103-106.txt | 1.32 KB | smiletrl |
#107 | drupal_options_field_type-2015689-106.patch | 40.19 KB | smiletrl |
#104 | interdiff-100-103.txt | 1.22 KB | smiletrl |
#104 | drupal_options_field_type-2015689-103.patch | 40.11 KB | smiletrl |
#102 | interdiff-97-100.txt | 2.75 KB | smiletrl |
Comments
Comment #1
swentel CreditAttribution: swentel commentedComment #2
swentel CreditAttribution: swentel commentedComment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedok Taking this one
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedGo testbot go ! to test my first patch.
Comment #6
swentel CreditAttribution: swentel commentedThis can't be right, should be ConfigFieldItemBase.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedAdded the suggested changes of #6.Please needs review
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedNeed Help into this.How to make my patch green ?
Comment #11
jsacksick CreditAttribution: jsacksick commentedHere's a new patch, we still have to convert the
hook_field_validate()
but I wanted to get inputs on the attached patch before I continue to work more on that.The patch coming from #1848570: Upgrade to Doctrine\Common 2.4 is needed, otherwise you'll get a Doctrine\Common\Annotations\AnnotationException.
Comment #13
amateescu CreditAttribution: amateescu commentedI just want to note that the allowed values stuff will change very soon in #1758622: Provide the options list of an entity field.
Comment #14
smiletrl CreditAttribution: smiletrl commented#11: drupal-convert-field-type-to-typed-data-plugin-options-2015689-11.patch queued for re-testing.
Comment #16
jsacksick CreditAttribution: jsacksick commentedThis new patch now contains the
hook_field_validate()
conversion, in order to do that I had to create a new ChoiceConstraint validation class. Hope this one will pass the tests.Comment #18
smiletrl CreditAttribution: smiletrl commentedGood start:) Some suggestions
1). Extra whitesapce
drupal-convert-field-type-to-typed-data-plugin-options-2015689-16.patch:551: trailing whitespace.
drupal-convert-field-type-to-typed-data-plugin-options-2015689-16.patch:653: trailing whitespace.
2).
I think
if
is unnecessary here, since this class is for field type list_boolean.3).
'#element_validate' => array(array($this, 'validateAllowedValues')),
should use'property_constraints' => array()
too?Comment #19
jsacksick CreditAttribution: jsacksick commentedI had to copy the
flattenOptions()
method fromOptionsWidgetBase
because the options_array_flatten function has been removed, we should probably put it back, otherwise we'll have to copy it everytime we need it...Comment #20
jsacksick CreditAttribution: jsacksick commentedThe attached patch removes the whitespaces + the field type condition.
Comment #21
amateescu CreditAttribution: amateescu commentedThis means we still need to do it in this patch, right?
{@inheritdoc}
This wouldn't be needed anymore if we can get #2041423: Rely on 'provider' instead of 'module' for Field plugin types done before :(
There seems to be some weird indentation going on there and in all the other schema() implementations. This and the item below (accessing settings) needs to be fixed in all places :)
You should use $field->getFieldSetting() instead of relying on the ArrayAccess implementation that should be removed soon.
by the... ?
I know the code where this were copied from did not have them, but we should add type hints everywhere now.
Out of scope for this issue.
Thanks for working on this! :)
Comment #22
jsacksick CreditAttribution: jsacksick commentedThanks for your review amateescu, here's a new patch with some of the feedbacks addressed.
Comment #23
amateescu CreditAttribution: amateescu commentedOhh, interdiff, nice :) You forgot the part that
$settings = $field->getFieldSettings();
and type hints needs to be done for all new classes..Comment #24
jsacksick CreditAttribution: jsacksick commentedIs there any typehints left? Because I can't find them.
Comment #25
smiletrl CreditAttribution: smiletrl commentedGood job!
'multiple' will force validated value to be multiple values, i.e. an array. In this case, validator is validating FieldItem's each primitive 'value', an string or number. This line should be deleted to use the default 'multiple' value, i.e., false.
Also how about 'illegal value selected.':)
Comment #26
jsacksick CreditAttribution: jsacksick commentedYes but I'm only setting multiple to TRUE if the cardinality of the field is unlimited which is wrong, I should do that if the cardinality is != 1.
Comment #27
jsacksick CreditAttribution: jsacksick commentedComment #28
smiletrl CreditAttribution: smiletrl commentedHmm, cardinality != 1 will probably fail too:)
You may try an options field with cardinality != 1 and save the field.
Comment #29
jsacksick CreditAttribution: jsacksick commentedYes you're right, it seems we don't need the multiple flag there, it works fine without.
Comment #30
smiletrl CreditAttribution: smiletrl commentedMaybe apply this change to demonstrate why 'multiple' line should be deleted:)
Comment #32
jsacksick CreditAttribution: jsacksick commentedComment #33
amateescu CreditAttribution: amateescu commentedYou updated the wrong class/method :) And it's
{@inheritdoc}
, notice the {}.I think we should put the parent:: call at the top, as our class usually needs to add (or replace) something to the element provided by the parent.
Wrong indentation here.
And here.
And here :P
...
No other complains otherwise :)
Comment #34
jsacksick CreditAttribution: jsacksick commentedThanks again for your review, I decided to remove the settingsForm implementation from the ListBooleanItem class and I regrouped everything in ListItemBase.
Comment #35
smiletrl CreditAttribution: smiletrl commentedA quick review, this looks pretty good -- looks RTC to me:)
Comment #36
stevectorAfter applying this patch I got errors when trying to add list fields to a node bundle:
and
Here's a reroll that should fix the errors.
Comment #37
yched CreditAttribution: yched commented#1758622: Provide the options list of an entity field is going to reformulate a couple things there, let's wait until it's in.
Comment #38
aspilicious CreditAttribution: aspilicious commentedBack to needs work as the linked issue is in :)
Comment #39
smiletrl CreditAttribution: smiletrl commentedMost stuff is associated with the validation. Some possible conversions:
1).
Options fieldItems should imlements interface AllowedValuesInterface
Personally speaking, I don't think it's really necessary for options module to implement this interface. Current options modules doesn't make use of hook_options_list. It provides allowed values via options_allowed_values
But the new OptionsWidgetBase has made use of this new interface, so it forces us to use this interface, if we are going to use this field widget. This also means we should probably move
Options_allowed_values
into that four methods of the new interface.2).
Get rid of
We are going to make use this class AllowedValuesConstraint
3).
This part could be updated. Change 'Choice' to 'AllowedValues'.
Add allowed values at getSettableValues(), see class AllowedValuesConstraintValidator
So, we also need to remove this line
+ 'choices' => array_keys($allowed_values),
, becuase it will be overridden at the validator to make use ofgetSettableValues()
.Comment #40
smiletrl CreditAttribution: smiletrl commentedMost changes come from #39.
One Notable thing regarding TypedDataManager::getConstraints()
I think this part of code is wrong. This constraint added here will validate the whole object/typedData as value against an array of 'AllowedValues'. But in fact, we need to validate a property of the object, e.g., value of ListTextItem object here, to against an array of allowed values, not to validate the ListTextItem object itself against the allowed value array.
This part of code is deleted in this patch. Maybe a cleanup for #1758622: Provide the options list of an entity field, and it needs to add , or check existing test to show why this part of code should be deleted.
Comment #42
smiletrl CreditAttribution: smiletrl commentedThere're still bugs, but post it here, in case someone else wants to play with it too.
Comment #44
smiletrl CreditAttribution: smiletrl commentedThis time it should be fine.
interdiff-36-44.txt indicates the change since #36.
It includes a bug fix for #1758622: Provide the options list of an entity field.
The default constraints for object/class which implements AllowedValuesInterface isn't always as primitive value to validate against the allowed value array. If this object is an ComplexData, e.g., ListTextItem, then it will result in problem that tries to validate the whole object againt an array.
So, we need to make sure the default contraints at TypedDataManager::getConstraints only works for Prmitive value. The ComplexData's property constraints shoud be added at its own
::getConstraints
function, like this:Comment #45
smiletrl CreditAttribution: smiletrl commentedoops:)
Comment #47
smiletrl CreditAttribution: smiletrl commented#36: drupal-convert-field-type-to-typed-data-plugin-options-2015689-36.patch queued for re-testing.
Comment #48
smiletrl CreditAttribution: smiletrl commented#44: options_field_type-2015689-44.patch queued for re-testing.
Comment #49
smiletrl CreditAttribution: smiletrl commentedNeed more work since #2074547: Remove ConfigFieldItemInterface::getInstance() is in.
Comment #50
smiletrl CreditAttribution: smiletrl commentedRemove $this->getInstance() for fieldItem class.
Comment #51
smiletrl CreditAttribution: smiletrl commentedRemove module entry.
Comment #52
swentel CreditAttribution: swentel commented#51: options_field_type-2015689-51.patch queued for re-testing.
Comment #53
swentel CreditAttribution: swentel commentedWill probably break heavily after #1953408: Remove ArrayAccess BC layer from field config entities got in
Comment #55
smiletrl CreditAttribution: smiletrl commentedWe als need to figure out the validation stuff in #2015687: Convert field type to FieldType plugin for taxonomy module .
Comment #56
catchMarked #2014671: [META] Convert all field types to plugins as duplicate now we're down to the last two.
Comment #57
mradcliffeAttempt at a re-roll of the patch. Setting to needs review until test bot has a go at it (probably going to fail).
Comment #58
mradcliffeNamespace should be Drupal\options\Plugin\Field\FieldType
for all field types defined in this patch.
Class changed to
Drupal\Core\Field\ConfigFieldItemBase;
$field is an object, not an array.
Comment #59
mradcliffeAdditionally there is a section in ListItemBase that requires the widget to change the allowed values description based on the widget. This is not possible in D8 because of form display modes.
Comment #60
mradcliffeHere's a patch for review.
- I did not include the rename commit in the interdiff as it would make it as large as the patch itself.
- With regard to my previous comment, I made the description text contain all of the instructions. I have attached an image to show the output at a mobile width. More help text will make users' eyes glaze over and not read it. I think this is better than removing it altogether though.
Comment #61
mradcliffeForgot interdiff that I uploaded and then deleted twice in the previous comment due to slow ajax response.
Comment #64
mradcliffeOne line fix to fix all those test fails on my local install.
One line cosmetic fix.
Comment #65
star-szrTagging for reroll.
Comment #66
mradcliffeRe-rolled.
- fetched origin
- rebase origin/8.x
- diff origin/8.x
Edit: I hid some older patches.
Comment #68
mradcliffeChanges form_error() usage to \Drupal::formBuilder()->setError() with additional $form_state parameter.
Comment #69
catchComment #70
mradcliffe68: 2015689-drupal-options-field-type-68.patch queued for re-testing.
Comment #72
mradcliffeSimple re-roll with git rebase origin/8.x
Comment #74
mradcliffeFixed patch to work with changes in TypedDataManager and ComplexDataInterface
Comment #76
jibranPatch failed.
Comment #77
mradcliffe- Fixed API change in #2143263: Remove "Field" prefix from FieldDefinitionInterface methods.
Comment #79
fagoInstead of removing it, this should embrace allowed values validation now - analogously to term references.
Comment #80
smiletrl CreditAttribution: smiletrl commented@mradcliffe thanks for your work!
Here's a patch to clean the code a bit. Also, the main property of list field is 'value', so the default main property of FieldItemBase should be ok to do with AllowedValuesInterface.
Comment #81
mradcliffeJust to clarify, will the flattenOptions method on formBuilder make a tighter dependency on field validation/constraints and the Form API? I'm not familiar with the new \Drupal static methods and how loose or tight they are.
Comment #82
smiletrl CreditAttribution: smiletrl commented\Drupal::formBuilder() is a core required method, i.e. form api is a required component. So, from my eyes, the dependency could be ignored and it's ok to use these methods in other parts, not just inside form api.
There're many other methods created by \Drupal container, and we have used them everywhere, e.g., \Drupal::typedDataManager().
Comment #83
mradcliffeYes, I know we like to depend on the container a lot already. I think it's probably the easiest way to get the patch in. Being able to solve this issue would be best in Drupal 9. It's always good to start thinking about it now. For instance, field validation is not necessarily form validation.
Comment #84
BerdirI think this needs to be reviewed by @fago, I'm not up to date enough with the allowed values discussion that happened in the taxonomy issue to RTBC this.
Comment #85
Berdir80: drupal_options_field_type-2015689-80.patch queued for re-testing.
Comment #87
smiletrl CreditAttribution: smiletrl commentedThis should fix that problem.
Comment #88
effulgentsia CreditAttribution: effulgentsia commented87: drupal_options_field_type-2015689-87.patch queued for re-testing.
Comment #90
effulgentsia CreditAttribution: effulgentsia commentedComment #91
effulgentsia CreditAttribution: effulgentsia commentedThis is just a code cleanup with no intended functional changes.
Comment #92
effulgentsia CreditAttribution: effulgentsia commentedOpened #2169983: Move type-specific logic from ListItemBase to the appropriate subclass as a normal priority, non-beta-blocking followup, and added @todos linking to it.
Comment #94
effulgentsia CreditAttribution: effulgentsia commentedThis UI change seems out of scope for this issue, so this patch reverts it, but attempts to improve the code comment that I think caused the confusion that led to the initial change.
I reviewed #87 quite thoroughly prior to my round of minor tweaks, and from my perspective, this patch is now good to go, including the way that AllowedValuesInterface is used (per #84). But since this is assigned to fago, and I've now authored some of it, I'll defer to him to RTBC. I agree with #83 that some of the dependencies on services in Drupal:: can be improved but that none of that is a regression from HEAD or release blocking. Anyone who wants to can still work on improving that in a follow up.
Comment #97
smiletrl CreditAttribution: smiletrl commentedThis should fix the problem.
Comment #99
effulgentsia CreditAttribution: effulgentsia commented97: drupal_options_field_type-2015689-97.patch queued for re-testing.
Comment #100
fagoDefinitely!
Patch looks pretty good to me. I just found some small issues (see below), else this looks RTBC to me.
Comment exceeds 80chars.
This needs to be updated to point to the method.
This should be in the data definition of the 'value' property as there is no reason to define it dynamically here.
Comment #101
yched CreditAttribution: yched commentedNo other remarks than @fago's #100.
Not strictly related, but while reviewing, created the following issues:
#2171393: Remove implementation of removed hook_options_list()
#2171395: OptionsWidgetBase should use WidgetInterface::massageFormValues()
Comment #102
smiletrl CreditAttribution: smiletrl commentedIndeed, constraints should be moved.
Comment #103
BerdirThis needs to be ListItemBase::allowedValuesString(), otherwise it's a function.
This seems short enough to put it on a single line, but fine if we do it like this elsewhere too.
Comment #104
smiletrl CreditAttribution: smiletrl commentedoops, missed that~
Comment #105
BerdirChanges look good, @fago, @effulentsia and @yched confirmed everything else, that should be enough. RTBC :)
Comment #106
yched CreditAttribution: yched commentedSorry, I thought I posted that, but apparently closed the tab before doing so :-/
I think @see needs to have the full namespaced class so that it gets correclty parsed by api.module
(just reacting to the last interdiff, didn't check the rest of the patch for other ocurrences)
Comment #107
smiletrl CreditAttribution: smiletrl commentedThanks, fixed. No other occurences.
Comment #108
yched CreditAttribution: yched commentedThanks @smilerl. RTBC+1.
Comment #109
webchickGreat work folks!
Committed and pushed to 8.x.
I believe this now un-postpones #2087393: Remove LegacyField once all field types converted.
We may need to add to/extend an existing change notice about this, so tagging.
Comment #110
xjmTagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release
Comment #111
BerdirI think we're good, this is part of the meta that is already referenced in https://drupal.org/node/2064123 and this was simply the last one to be converted.