We already have #allowed_units on the form elements, but there's no setting on the widget, or an alter hook. We need at least one of those.

CommentFileSizeAuthor
#13 3012104-10.patch3.73 KBbalagan
#10 interdiff_4-10.txt1.55 KBOleksiy
#4 interdiff.txt3.74 KBDinesh18
#4 3012104-4.patch3.81 KBDinesh18
#2 3012104-2.patch2.93 KBpcambra

Issue fork physical-3012104

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz created an issue. See original summary.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra at Cambrico for Zensations commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3012104-2.patch2.93 KB

How about this?

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Needs work

It's a good start.

We're missing updated schema (in physical.schema.yml, the field.widget.settings.* entries)

Also:

+    if (array_filter($this->getSetting('allowed_units'))) {
+      $element['#available_units'] = $this->getSetting('allowed_units');
+    }

Why the mismatch, allowed_units VS available_units? I'd expect the widget setting to also be available_units.

@@ -87,8 +100,8 @@ abstract class PhysicalWidgetBase extends WidgetBase {
   /**
    * Gets the unit class for the current field.
    *
-   * @return string
-   *   The fully qualified class name.
+   * @return \Drupal\physical\UnitInterface
+   *   The unit class.
    */
   abstract protected function getUnitClass();

Why are we changing this? Looks like we're still returning just the class name.

Dinesh18’s picture

Dinesh18 CreditAttribution: Dinesh18 as a volunteer commented
Status: Needs work » Needs review
FileSize
3012104-4.patch3.81 KB
interdiff.txt3.74 KB

Fixed the below points :
1) We're missing updated schema (in physical.schema.yml, the field.widget.settings.* entries)
2) Why the mismatch, allowed_units VS available_units? I'd expect the widget setting to also be available_units.

as per comment #3

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra at Cambrico commented

@bojanz, regarding the change on getUnitClass, on my tests I've found that it was a unitinterface what I was receiving, please correct me if I'm wrong:

+    /** @var \Drupal\physical\UnitInterface $unit_class */
+    $unit_class = $this->getUnitClass();
+    $unit_class::getLabels();
pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra at Cambrico commented
jeffschuler’s picture

jeffschuler
Location Boulder, Colorado
CreditAttribution: jeffschuler as a volunteer commented
Status: Needs review » Reviewed & tested by the community

This is working well and very necessary.
Much appreciated!

Oleksiy made their first commit to this issue’s fork.

Oleksiy opened merge request !3

Oleksiy’s picture

Oleksiy
Primary language Ukrainian
Location Lutsk 🇺🇦
CreditAttribution: Oleksiy at Skilld for Drupal Ukraine Community commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff_4-10.txt1.55 KB

I got the error during the configs import:

In ConfigImportCommands.php line 357:

The import failed due to the following reasons:
Unexpected error during import with operation update for core.entity_form_d
isplay.node.product_detail_page.default: The configuration property content
.field_thickness.settings.available_units.mm doesn't exist.

The config schema attached to the patch is not correct. The Config inspector module tells the same.

Configuration validation => Array
(
    [core.entity_form_display.node.product_detail_page.default:content.field_thickness.settings.available_units] => missing schema
)

Added updated scheme to the new MR.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented
Status: Needs review » Reviewed & tested by the community

#9 looks good for me
the only thing which would be nice to address is to sanitize the options of units which is not selected as allowed. That is not something that blocks the feature, widget setting properly handles such values. I suggest addressing that in the follow-up if it really requires attention.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented

#10 something is unrelated to this patch as the #9 declares sequence and the message posted here showing undefined mapping. Looks like something messed up in the project configuration. And dont forget rebuild the container after applying the patch.

balagan’s picture

balagan CreditAttribution: balagan at Gizra commented
FileSize
3012104-10.patch3.73 KB

Created a patch from the interdiff at #10

jsacksick’s picture

jsacksick CreditAttribution: jsacksick at Centarro commented
Status: Reviewed & tested by the community » Fixed

Committed! Thanks everyone!

Status: Fixed » Closed (fixed)

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