as title says

CommentFileSizeAuthor
#28 move_cardinality-2097691-28.patch36.49 KBplopesc
#25 move_cardinality-2097691-25.patch36.5 KBplopesc
#25 interdiff.txt10.85 KBplopesc
#23 move_cardinality-2097691-23.patch36.64 KBplopesc
#22 move_cardinality-2097691-22.patch37.17 KBplopesc
#21 move_cardinality-2097691-21.patch37.15 KBplopesc
#18 move_cardinality-2097691-18.patch37.12 KBplopesc
#18 interdiff.txt2.17 KBplopesc
#16 move_cardinality-2097691-16.patch36.05 KBamateescu
#10 move_cardinality-2097691-10.patch35.26 KBplopesc
#10 interdiff.txt704 bytesplopesc
#8 move_cardinality-2097691-8.patch35.21 KBplopesc
#8 interdiff.txt26.96 KBplopesc
#6 move_cardinality-2097691-6.patch35.48 KBplopesc
#6 interdiff.txt32.34 KBplopesc
#2 move_cardinality-2097691-2.patch5.33 KBplopesc

Comments

yched’s picture

yched commented

It 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)

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Active » Needs review
StatusFileSize
new move_cardinality-2097691-2.patch5.33 KB

First round...

yched’s picture

yched commented

@plopesc: Thanks ! "First round" as in "working on more things", or is this ready for review ?

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

Yay! , it was as ready for review!

Sorry for the misunderstanding.

Regards.

yched’s picture

yched commented

Needs reroll :-)

  1. +++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
@@ -599,6 +599,14 @@ public function isFieldRequired() {
+    return($cardinality == FIELD_CARDINALITY_UNLIMITED) || ($cardinality > 1);

    missing space after return 

  2. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstance.php
@@ -590,6 +590,14 @@ public function isFieldRequired() {
+    $cardinality = $this->getFieldCardinality();
+    return($cardinality == FIELD_CARDINALITY_UNLIMITED) || ($cardinality > 1);

    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 ;-)

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
StatusFileSize
new interdiff.txt32.34 KB
new move_cardinality-2097691-6.patch35.48 KB

Hello

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 and FieldInstanceInterface), 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...

yched’s picture

yched commented

Sorry, 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 :-)

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
StatusFileSize
new interdiff.txt26.96 KB
new move_cardinality-2097691-8.patch35.21 KB

Patch grepped ; -)

amateescu’s picture

amateescu commented 
+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinitionInterface.php
@@ -170,6 +176,14 @@ public function getFieldCardinality();
+   * Returns whether the field is multiple.
+   *
+   * @return bool
+   *   TRUE if the field is multiple.

How about "Returns whether the field can contain multiple items." and "TRUE if the field can contain multiple items, FALSE otherwise."

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
StatusFileSize
new interdiff.txt704 bytes
new move_cardinality-2097691-10.patch35.26 KB

Docblock improved

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

Cool, I think we're done here.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/field/field.form.inc
@@ -24,7 +25,7 @@ function theme_field_multiple_value_form($variables) {
+  if ($element['#cardinality'] > 1 || $element['#cardinality'] == FieldDefinitionInterface::CARDINALITY_UNLIMITED) {

Shouldn't this use the new isMultiple() method?

amateescu’s picture

amateescu commented
Status: Needs work » Reviewed & tested by the community

We don't have a field object there :)

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: -Field API, -Entity Field API

#10: move_cardinality-2097691-10.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Field API, +Entity Field API

The last submitted patch, move_cardinality-2097691-10.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new move_cardinality-2097691-16.patch36.05 KB

Rerolled.

yched’s picture

yched commented

Agreed 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() ?

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
StatusFileSize
new interdiff.txt2.17 KB
new move_cardinality-2097691-18.patch37.12 KB

Patch 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

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: -Field API, -Entity Field API

#18: move_cardinality-2097691-18.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Field API, +Entity Field API

The last submitted patch, move_cardinality-2097691-18.patch, failed testing.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new move_cardinality-2097691-21.patch37.15 KB

Rerolled.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
StatusFileSize
new move_cardinality-2097691-22.patch37.17 KB

Rerolled after #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system is in.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
StatusFileSize
new move_cardinality-2097691-23.patch36.64 KB

Rerolled after #2115145: Move field type plugins to Plugin/Field/FieldType.

Status: Needs review » Needs work

The last submitted patch, move_cardinality-2097691-23.patch, failed testing.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt10.85 KB
new move_cardinality-2097691-25.patch36.5 KB

Sorry, I forgot to modify test files... :S

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Looks good. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new move_cardinality-2097691-28.patch36.49 KB

Rerolled

swentel’s picture

swentel commented
Status: Needs review » Reviewed & tested by the community

Was RTBC before

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 840e20a and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

RunePhilosof’s picture

RunePhilosof commented

Doesn't this need a change record?

yched’s picture

yched commented
Title: Move FIELD_CARDINALITY_UNLIMITED constant to FieldDefinitionInterface » [Change notice] Move FIELD_CARDINALITY_UNLIMITED constant to FieldDefinitionInterface
Priority: Normal » Major
Issue summary: View changes
Status: Closed (fixed) » Active
Issue tags: +Needs change record

Indeed. @plopesc, can you look into that ?

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Active » Needs review

Hi

Here is the change notice: https://drupal.org/node/2134999

I hope everything is well, is my first change notice :)

Regards

yched’s picture

yched commented
Title: [Change notice] Move FIELD_CARDINALITY_UNLIMITED constant to FieldDefinitionInterface » Move FIELD_CARDINALITY_UNLIMITED constant to FieldDefinitionInterface
Priority: Major » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Looks good. Thanks !

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.