yiisoft / form

The package helps with implementing data entry forms
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
45 stars 20 forks source link

Form fields are not customizable #17

Closed armpogart closed 3 years ago

armpogart commented 4 years ago

I like in which direction this library goes, but I still don't like some aspects of it, will post each problem in a separate issue. This one concerning \Yiisoft\Yii\Form\Widget\Field class extendibility.

In yii2 all widgets were just not final classes and our own classes could easily extend them to customize the behavior wanted. I'm aware of the issues that not final classes pose, such as it becomes more difficult to maintain/support/change them later as each change can cause BC break, so here comes final classes. And I always encourage to use final classes where possible. But Meyer's open-closed principle (from SOLID) still states that the class must be open for extension (while still being closed for modification). DI in yii3 partly solves the problem, I say partly as the classes must be still designed carefully to allow that extendibility. And now comes an example.

I want to customize the fields for usage with bootstrap 4, here is an example of a typical bootstrap 4 form:

<form>
  <div class="form-group">
    <label for="inputLogin">Login</label>
    <div class="input-group">
      <div class="input-group-prepend">
        <span class="input-group-text" id="inputGroupPrepend">@</span>
      </div>
      <input type="text" class="form-control" id="inputLogin" placeholder="Example input placeholder">
    </div>
  </div>
  <div class="form-group">
    <label for="inputPassword">Another label</label>
    <input type="text" class="form-control" id="inputPassword" placeholder="Another input placeholder">
    <small class="form-text text-muted">
      Your password must be 8-20 characters long, contain letters and numbers, and must not contain spaces, special characters, or emoji.
    </small>
  </div>
  <div class="form-group">
    <label for="inputSelect">Select Label</label>
    <select id="inputSelect" class="form-control">
        <option selected>Choose...</option>
        <option>...</option>
    </select>
  </div>
</form>

Now the issues with current implementation:

terabytesoftw commented 4 years ago

You can configure the widget to your liking in various ways, regards.

<?php

declare(strict_types=1);

use App\Asset\LoginAsset;
use Yiisoft\Yii\Form\Widget\Form;
use Yiisoft\Yii\Form\Widget\Field;
use Yiisoft\Html\Html;

$this->params['breadcrumbs'] = 'Login';
$this->setTitle($this->params['breadcrumbs']);

$assetManager->register([
    LoginAsset::class
]);

$fieldConfig = [
    'labelOptions()' => [['class' => 'block text-gray-800 text-sm font-bold mb-2', 'label' => '']],
    'inputOptions()' => [['class' => 'hadow appearance-none border rounded w-full py-2 px-3 text-gray-700 ' .
        'leading-tight focus:outline-none focus:shadow-outline', 'placeholder' => false]],
    'errorOptions()' => [['class' => 'text-red-500 text-xs font-bold italic']],
    'hintOptions()' => [['class' => 'text-blue-500 text-xs font-bold']]
];
?>

<?= Html::beginTag('div', ['class' => 'form-security-login']) ?>

    <?= Html::beginTag('p', ['class' => 'form-security-login-subtitle font-bold']) ?>
        Please fill out the following fields <br/> Sign In
    <?= Html::endTag('p') ?>

    <?= Html::tag('hr', '', ['class' => 'mb-2']) ?>

    <?= Form::begin()
        ->action($urlGenerator->generate('auth/login'))
        ->options(
            [
                'id' => 'form-security-login',
                'class' => 'forms-security-login bg-white shadow-md rounded px-8 pb-8',
                'csrf' => $csrf
            ]
        )
        ->start() ?>

        <?= Field::widget($fieldConfig)
            ->config($data, 'login', ['class' => 'mb-4'])
        ?>

        <?= Field::widget($fieldConfig)
            ->config($data, 'password', ['class' => 'mb-6'])
            ->passwordInput()
        ?>

        <?= Field::widget($fieldConfig)
            ->config($data, 'rememberMe')
            ->checkbox()
        ?>

        <?= Html::beginTag('div', ['class' => 'flex items-center justify-between']) ?>
            <?= Html::submitButton(
                'Login',
                [
                    'class' => 'bg-blue-500 hover:bg-blue-700 text-white font-bold py-2 px-4 rounded ' .
                        'focus:outline-none focus:shadow-outline',
                    'id' => 'login-button',
                    'tabindex' => '4',
                ]
            ) ?>
            <?= Html::a(
                'Forgot Password',
                $urlGenerator->generate('recovery/request'),
                ['class' => 'inline-block align-baseline font-bold text-sm text-blue-500 hover:text-blue-800']
            ) ?>

        <?= Html::endTag('div') ?>

        <?= Html::beginTag('div', ['class' => 'text-center pt-3']) ?>
            <?= Html::a(
                'Didn\'t receive confirmation message',
                $urlGenerator->generate('registration/resend'),
                ['class' => 'inline-block align-baseline font-bold text-sm text-red-500 hover:text-red-800']
            ) ?>
        <?= Html::endTag('div') ?>

    <?= Form::end() ?>

<?php echo Html::endTag('div');
armpogart commented 4 years ago

@terabytesoftw Sorry, but your answer doesn't help with any of the issues mentioned. I'm not sure why you have closed the issue (I don't always have time to answer to all github replies immediately).

Now, for the issues:

Now, the Yii2 approach of not having final class allowed to simply extend the class and make the necessary modifications, but the new approach extremely limits the possibilities. And now to have a form for any custom css framework we need to again build a custom library, but instead of just applying the changes we need, we must simply duplicate all the code that we need from here and customize it.

I'm not sure what's the right approach to solve that problem. One way I guess would be to provide also traits with common functionality for anybody who needs customization. Another way (the better one) would be to make the widgets (especially the Yiisoft\Form\Widget\Field) make more extendable by keeping it final (closed for modification), but I'm not sure how.

@samdark Do you have any input here?

terabytesoftw commented 4 years ago

If you see the code example you will see that you can customize everything, through the constructor options.

In the example above I changed all the default settings to tailwindcss.

Options correct setting factory:

$fieldConfig = [
    'labelOptions()' => [['class' => 'block text-gray-800 text-sm font-bold mb-2', 'label' => '']],
    'inputOptions()' => [['class' => 'hadow appearance-none border rounded w-full py-2 px-3 text-gray-700 ' .
        'leading-tight focus:outline-none focus:shadow-outline', 'placeholder' => false]],
    'errorOptions()' => [['class' => 'text-red-500 text-xs font-bold italic']],
    'hintOptions()' => [['class' => 'text-blue-500 text-xs font-bold']]
];

<?= Field::widget($fieldConfig)
    ->config($data, 'login', ['class' => 'mb-4'])
?>

Global:

Field::class => static function () {
    $fieldConfig = [
        'labelOptions()' => [['class' => 'block text-gray-800 text-sm font-bold mb-2', 'label' => '']],
        'inputOptions()' => [['class' => 'hadow appearance-none border rounded w-full py-2 px-3 text-gray-700 ' .
        'leading-tight focus:outline-none focus:shadow-outline', 'placeholder' => false]],
        'errorOptions()' => [['class' => 'text-red-500 text-xs font-bold italic']],
        'hintOptions()' => [['class' => 'text-blue-500 text-xs font-bold']]
    ];

    return Field::widget($fieldConfig);
},

WebViewFactory:

$webView->setDefaultParameters(
       [
           'field' => $container->get(Field::class)
       ]
);

we just have to know how to use the container-di and the factory, in his example its use is wrong.

samdark commented 4 years ago

I prefer keeping final and making everything customizable.

I still don't see a way to do that customization globally. You have showed how to customize some aspects in the view, but when I try to that customization in DI Container nothing happens

That should be customizable via factory config. Have you used it?

samdark commented 4 years ago

Seems we need to add more docs on how to customize it, at least.

Enrica-r commented 4 years ago

In Yii2 exists a lot of extensions based on form, activeform and their fields (e.g Kartik-v). With final class you will lock out future migrations from Yii2 to Yii 3.

I fear that either Yii2 will survive next 10 years or they will fork form package. In general the more breaking staff is built in Yii 3 more developers will stay on Yii2 instead of migrate. We have learnt this during switch from Yii 1 to Yii2. It shouldn't be a target to maintain three different Yii's for years. But the version steps would be another discussion.

Back to the current discourses. Inheritance isn't just css class configuration. It could have further methods. Once i wanted to extend activefield with automatic type recognition delivered from model. How can you do this withou inheritance or traits? Think about such scenarios please.

samdark commented 4 years ago

@Enrica-r most likely, you can do so with composition instead of inheritance.

Enrica-r commented 4 years ago

@samdark Thanks for your answer. I think composition isn't an ideal instrument for this purpose. I found a good blog named The Decision Between Inheritance And Composition which describes when using what.

An inheritance is a strong dependency saying "is a" while composition communicates between classes via interface (and DI) especially when reusing code.

In this case here I have a strong dependency like a "specialField is a Field". The "specialField" isn't eg. a frontend which communicates with a backend. They are never independant.

I have a theoretic example: You want to add something in method "run" before running. What would i do:

Inheritance: new class SpecialField extends Field override method "run", add further functionality and call "parent::run" --> that's it

Composition: new class SpecialField inject interface "FieldInterface" -> store it in private variable "$field" redefine all methods and call interface like eg private function renderBegin() { return $field->renderBegin(); } even if my class never changes this method. And more, such a class "SpecialField" will never be independent of changes in class "Field". It has to adapt to new content. Also, you inherit Field from Widget because Field is a Widget.

Do I have something missunderstand?

samdark commented 4 years ago

There are multiple ways to solve your example:

  1. Introduce beforeRun event. Then you don't have to deal with either inheritance or composition.
  2. Introduce an interface. Then you can implement decorator pattern.

But in your case you're using SpecialField explicitly and are willing to replace it in views. So it will be:

class SpecialField
{
   public function run()
   {
       // before run
       return Field::widget()->run();
   }
}
YiiRocks commented 4 years ago

What is the big advantage of not allowing extending classes/inheritance?

samdark commented 4 years ago

In many cases it results in better design.

armpogart commented 4 years ago

After working a little bit with Field and FormModel in real project, I have following concerns:

  1. requiredCssClass, successCssClass, errorSummaryCssClass properties are not used anywhere
  2. default options (all set by constants, e.g. classes) can not be removed at all. Why make them hardcoded at all?
  3. isAttributeRequired property in FormModel is ignored when building field. Why not set required input attribute by default in that case?
  4. No way to determine if the form (FormModel) is already validated. Maybe add some flag?
  5. Nested forms (FormModels) to avoid duplicate code when building complex model from simple ones, that are already used somewhere else?
  6. No way add dynamic parts. So for adding bootstrap frameworks input group to input field, I had to do for each field:
    $field->config($form, 'email')
    ->template("{label}\n<div class='input-group'>\n{input}\n<div class='input-group-append'><div class='input-group-text'><span class='fas fa-envelope fa-fw'></span></div></div>{hint}\n{error}</div>")

    What about a way to add some sort of dynamic parts, that can be precofigured, and we will just need to specify content of that part in view?

These are all main issues while building complex forms in real application.

terabytesoftw commented 4 years ago

If you have a proposal you can do it, nested forms seem like a good idea to me, and in general the use in real projects does not give a better way to use them.

samdark commented 4 years ago

@terabytesoftw isn't it possible to configure factory to solve point 6?

armpogart commented 4 years ago

@terabytesoftw I can configure the template itself in factory, but how am I supposed to change the dynamic part for each field individually? In my example <span class='fas fa-envelope fa-fw'></span> is dynamic part, which is not actually part of predefined template and it needs to be set for each field. So the template is:

"{label}\n<div class='input-group'>\n{input}\n<div class='input-group-append'><div class='input-group-text'>{dynamic}</div></div>{hint}\n{error}</div>"

and I want something like:

$field->config($form, 'email')
    ->label(false)
    ->textInput(['required' => true])
    ->setDynamicPart('dynamic', 'Here goes dynamic contents')
armpogart commented 4 years ago

I'm updating here again with another issue encountered while working on project (not to forget to address them all) and easy non-bc fix.

7. Field class dropDownList method and DropDownList widget don't support setting custom attributes on items (options) which is valid html and is widely used for storing additional data needed for js as an example.

Update: The fix is not as trivial as I thought :( Update 2: This is already supported. PHPDoc comments were a little messy on this one, so I've found it later, while reading the code.

armpogart commented 4 years ago
  1. An ability to disable container for the field (Field class). Use case: when stacking inputs in one row we don't need container (https://getbootstrap.com/docs/4.5/components/forms/#form-row) ✅ #33
armpogart commented 4 years ago
  1. Array properties in the form model.
terabytesoftw commented 3 years ago

After working a little bit with Field and FormModel in real project, I have following concerns:

  1. requiredCssClass, successCssClass, errorSummaryCssClass properties are not used anywhere
  2. default options (all set by constants, e.g. classes) can not be removed at all. Why make them hardcoded at all?
  3. isAttributeRequired property in FormModel is ignored when building field. Why not set required input attribute by default in that case?
  4. No way to determine if the form (FormModel) is already validated. Maybe add some flag?
  5. Nested forms (FormModels) to avoid duplicate code when building complex model from simple ones, that are already used somewhere else?
  6. No way add dynamic parts. So for adding bootstrap frameworks input group to input field, I had to do for each field:
$field->config($form, 'email')
    ->template("{label}\n<div class='input-group'>\n{input}\n<div class='input-group-append'><div class='input-group-text'><span class='fas fa-envelope fa-fw'></span></div></div>{hint}\n{error}</div>")

What about a way to add some sort of dynamic parts, that can be precofigured, and we will just need to specify content of that part in view?

These are all main issues while building complex forms in real application.

Solve in new PR.

samdark commented 3 years ago

https://github.com/yiisoft/form/pull/86