Plugins with configuration should implement defaultConfiguration().

This sort of faffing in the form should be removed too, as once defaultConfiguration() is defined, it's not necessary as this->configuration will always contain at least the defaults:

      '#default_value' => isset($this->configuration['license_role']) ? $this->configuration['license_role'] : '',
CommentFileSizeAuthor
#7 role_plugin_should_implement_defaultConfiguration_method-2905892-7.patch1009 byteskarthikkumarbodu
#2 2905892_role-plugin-should-implement-defaultConfiguration.2.patch1.1 KBchrisrockwell

Comments

joachim created an issue. See original summary.

chrisrockwell’s picture

chrisrockwell commented
StatusFileSize
new 2905892_role-plugin-should-implement-defaultConfiguration.2.patch1.1 KB
chrisrockwell’s picture

chrisrockwell commented
Status: Active » Needs review
joachim’s picture

joachim commented
Status: Needs review » Needs work

Thanks for the patch!

+++ b/src/Plugin/Commerce/LicenseType/Role.php
@@ -30,6 +30,17 @@ public function buildLabel(LicenseInterface $license) {
+      'role' => [
+        'license_role' => ''
+      ]

@@ -82,7 +93,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
-      '#default_value' => isset($this->configuration['license_role']) ? $this->configuration['license_role'] : '',
+      '#default_value' => $this->configuration['role']['license_role'],

This is the right approach, but you've added a level of nesting we don't need.

This plugin has only one property, 'license_role', and that should be at the top level of the array.

chrisrockwell’s picture

chrisrockwell commented

I'm going to assume this is related to #2905959: Accommodate changes to defaultConfiguration that break form default values and calculateDate configs which was closed as a duplicate for #2879301: Time handling and periodic field/form element type (?) - this issue might suffer the same fate. Something broke the configuration, just not sure what yet.

[EDIT] I take that back, we still need the changes, but it's best that we fix the configuration issue first so that the nesting isn't required.

joachim’s picture

joachim commented

#2905959: Accommodate changes to defaultConfiguration that break form default values and calculateDate configs was about the expiry plugin, whereas this is the license type plugin. At any rate, that one is working fine. The nesting should be removed.

karthikkumarbodu’s picture

karthikkumarbodu commented
Status: Needs work » Needs review
StatusFileSize
new role_plugin_should_implement_defaultConfiguration_method-2905892-7.patch1009 bytes

joachim’s picture

joachim commented
Status: Needs review » Fixed

Committed. Thanks!

Status: Fixed » Closed (fixed)

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