Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

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

Status: Active » Needs review
FileSize
5.33 KB

First round...

yched’s picture

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

plopesc’s picture

Yay! , it was as ready for review!

Sorry for the misunderstanding.

Regards.

yched’s picture

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

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

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

Patch grepped ; -)

amateescu’s picture

+++ 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

Docblock improved

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Cool, I think we're done here.

catch’s picture

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

Status: Needs work » Reviewed & tested by the community

We don't have a field object there :)

catch’s picture

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

Status: Needs work » Reviewed & tested by the community
FileSize
36.05 KB

Rerolled.

yched’s picture

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

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

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

Status: Needs work » Needs review
FileSize
37.15 KB

Rerolled.

plopesc’s picture

plopesc’s picture

Status: Needs review » Needs work

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

plopesc’s picture

Status: Needs work » Needs review
FileSize
10.85 KB
36.5 KB

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

yched’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

plopesc’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
36.49 KB

Rerolled

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Was RTBC before

alexpott’s picture

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

Doesn't this need a change record?

yched’s picture

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

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

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.