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

Comments

bojanz created an issue. See original summary.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 3012104-2.patch2.93 KB

How about this?

bojanz’s picture

bojanz 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 commented
Status: Needs work » Needs review
StatusFileSize
new 3012104-4.patch3.81 KB
new 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
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
commented
jeffschuler’s picture

jeffschuler
Location Boulder, Colorado
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 🇺🇦
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 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
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
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 commented
StatusFileSize
new 3012104-10.patch3.73 KB

Created a patch from the interdiff at #10

jsacksick’s picture

jsacksick 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.