Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
as title says
Comment | File | Size | Author |
---|---|---|---|
#28 | move_cardinality-2097691-28.patch | 36.49 KB | plopesc |
#25 | move_cardinality-2097691-25.patch | 36.5 KB | plopesc |
#25 | interdiff.txt | 10.85 KB | plopesc |
#23 | move_cardinality-2097691-23.patch | 36.64 KB | plopesc |
#22 | move_cardinality-2097691-22.patch | 37.17 KB | plopesc |
Comments
Comment #1
yched CreditAttribution: yched commentedIt might also be worth adding an isMultiple() helper method on FieldDefinitionInterface to encapsulate the
(cardinality == FIELD_CARDINALITY_UNLIMITED || cardinality > 1) logic that is reproduced at several places (this should actually cover most uses of the constant in external code)
Comment #2
plopescFirst round...
Comment #3
yched CreditAttribution: yched commented@plopesc: Thanks ! "First round" as in "working on more things", or is this ready for review ?
Comment #4
plopescYay! , it was as ready for review!
Sorry for the misunderstanding.
Regards.
Comment #5
yched CreditAttribution: yched commentedNeeds reroll :-)
missing space after return
Should be return $this->field->isFieldMultiple(), like other methods in the class that proxy to logic that is held in the Field
+ great for isFieldRequired(), but the initial goal was to move FIELD_CARDINALITY_UNLIMITED to a class constant in FieldDefinitionInterface ;-)
Comment #6
plopescHello
Sorry, I forgot to include the second part of the issue... waste of time.
Now including your comments and variable moved to
FieldDefinitionInterface
.In classes that previously included any of the interfaces that extends
FieldDefinitionInterface
(FieldInterface
andFieldInstanceInterface
), instead of including this class, I used the const from these classes, in order to avoid the load of another interface.Let's see what testbot says...
Comment #7
yched CreditAttribution: yched commentedSorry, should have mentioned earlier : since the constant is now namespaced by the interface, we can remove the FIELD_ prefix. Also, no need to mention "field API" in the docblock, that's kind of redundant :-)
Comment #8
plopescPatch grepped ; -)
Comment #9
amateescu CreditAttribution: amateescu commentedHow about "Returns whether the field can contain multiple items." and "TRUE if the field can contain multiple items, FALSE otherwise."
Comment #10
plopescDocblock improved
Comment #11
amateescu CreditAttribution: amateescu commentedCool, I think we're done here.
Comment #12
catchShouldn't this use the new isMultiple() method?
Comment #13
amateescu CreditAttribution: amateescu commentedWe don't have a field object there :)
Comment #14
catch#10: move_cardinality-2097691-10.patch queued for re-testing.
Comment #16
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #17
yched CreditAttribution: yched commentedAgreed with @catch that this last remaining use looks weird now. The code that constructs the $element could populate a #cardinality_multiple bool property with the result of isMultiple() ?
Comment #18
plopescPatch including '#cardinality_multiple' property in $element generation.
I think this option is not obvious, but given that it only appear once in core, probably, nobody will deal with it...
Regards
Comment #19
catch#18: move_cardinality-2097691-18.patch queued for re-testing.
Comment #21
plopescRerolled.
Comment #22
plopescRerolled after #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system is in.
Comment #23
plopescRerolled after #2115145: Move field type plugins to Plugin/Field/FieldType.
Comment #25
plopescSorry, I forgot to modify test files... :S
Comment #26
yched CreditAttribution: yched commentedLooks good. Thanks!
Comment #27
alexpottPatch no longer applies.
Comment #28
plopescRerolled
Comment #29
swentel CreditAttribution: swentel commentedWas RTBC before
Comment #30
alexpottCommitted 840e20a and pushed to 8.x. Thanks!
Comment #32
RunePhilosof CreditAttribution: RunePhilosof commentedDoesn't this need a change record?
Comment #33
yched CreditAttribution: yched commentedIndeed. @plopesc, can you look into that ?
Comment #34
plopescHi
Here is the change notice: https://drupal.org/node/2134999
I hope everything is well, is my first change notice :)
Regards
Comment #35
yched CreditAttribution: yched commentedLooks good. Thanks !