Refactor form for adding classes to Styles entities

Hi team,
I have updated the entity style classes field to have simpler form by updating the ajax enabled fieldset field to textfield.

Please review the PR

Thanks

Nice work, @akshay_d

This worked well in m y manual testing, but there are still a few changes I'd like to see.

First, I have a couple nitpicky things from the form build:

+++ b/src/Form/StylesForm.php
@@ -16,12 +16,7 @@ class StylesForm extends EntityForm {
+    $classes = !empty($styles->get('classes')) ? $styles->get('classes') : NULL;

I think I'd rather see this:

$classes = $styles->get('classes') ?? [];

And then later we can implode without any additional logic:

+++ b/src/Form/StylesForm.php
@@ -42,38 +37,14 @@ class StylesForm extends EntityForm {
+      '#default_value' => $classes ? implode(' ', $classes) : '',

That could be changed to:

'#default_value' => implode($classes)

And then I'd like to see the save method approached in a different way. We should override the parent's copyFormValuesToEntity method and deal with setting values in there. I think it would look like this:

  /**
   * {@inheritdoc}
   */
  protected function copyFormValuesToEntity(EntityInterface $entity, array $form, FormStateInterface $form_state) {
    $values = $form_state->getValues();
    foreach ($values as $key => $value) {
      if ($key === 'classes') {
        $entity->set('classes', explode(' ', $value));
      }
      else {
        $entity->set($key, $value);
      }
    }
  }

Then the save method would look like this:

  /**
   * {@inheritdoc}
   */
  public function save(array $form, FormStateInterface $form_state) {
    $styles = $this->entity;
    $status = $styles->save();

    switch ($status) {
      case SAVED_NEW:
        $this->messenger()->addStatus($this->t('Created the %label Styles.', [
          '%label' => $styles->label(),
        ]));
        break;

      default:
        $this->messenger()->addStatus($this->t('Saved the %label Styles.', [
          '%label' => $styles->label(),
        ]));
    }
    $form_state->setRedirectUrl($styles->toUrl('collection'));
    return $status;
  }

Note we're returning the status at the end which is something we are forgetting to do at the moment.

This is a much better logical division of labor. Now the save method is only handling saving, messaging, and redirecting, which is what the docbloc says it should do.

And now the copyFormValuesToEntity will do what it says it does, and do it correctly the first time.

Thanks @danflanagan8 for reviewing the patch.

Also, I made the requested change please re-review.

Looks good, @akshay_d. Thanks for the updates!

I just have one nit. It looks like the word "or" is missing from this sentence:

+++ b/src/Form/StylesForm.php
@@ -42,38 +38,14 @@ class StylesForm extends EntityForm {
+      '#description' => $this->t('Add one more classes by delimiting with space.'),

Can you change this to read, "Add one or more space-delimited classes."

@danflanagan8 Thanks for the detailed review.

Also, Updated the patch please review.

Thanks, @akshay_d!

Here's an updated patch with tests. Ease of test coverage, after all, was part of the motivation here.

The last submitted patch, 7: interdiff-6-7.patch, failed testing. View results →

Ha! The interdiff mostly passed. :)

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