Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow add or remove field on form.refreshFields event #5006

Closed
wants to merge 1 commit into from
Closed

Allow add or remove field on form.refreshFields event #5006

wants to merge 1 commit into from

Conversation

RomainMazB
Copy link
Contributor

This allow the developper to add or remove fields naturally using addFields or removeField on the backend.form.refreshFields event.

A use case is to display some additionnal fields pre-filled(using default field properties) with another model data (which is my actual case).

Here is an example of use:

// plugins/romainmazb/realestate/controllers/Lands.php
Event::listen('backend.form.refreshFields', function ($formWidget, $allFields) {
    $formWidget->removeField('tile_front');
    $formWidget->removeField('tile_back');
    $formWidget->addFields([
        'my_field1' => [
            'label'   => 'My Field 1',
            'comment' => 'This is a custom field I have added.',
            'default' => 'Some custom value'
        ],
    ]);
    $formWidget->addFields([
        'my_field2' => [
            'label'   => 'My Field 2',
            'comment' => 'This is a custom field I have added.',
            'default' => 'Some other value'
        ],
    ]);
});

And here is the result which can't be done without this changes since the actual source code only trigger makePartial on the field who are linked with dependsOn and don't even look if the $this->allFields changed:
ezgif com-optimize

Hope it will be helpful to the community.

This allow the developper to add or remove fields naturally using addFields or removeField on the backend.form.refreshFields event.

A use case is to display some additionnal fields pre-filled(using `default` field properties) with another model data (which is my actual case).
@RomainMazB RomainMazB changed the title Allow add or remove field on formRefresh event Allow add or remove field on form.refreshFields event Mar 26, 2020
@RomainMazB
Copy link
Contributor Author

RomainMazB commented Mar 26, 2020

And here is my finished use case, I added a switch box to "disable" the whole form refresh which can be annoying in some case just to show what is possible:

// plugins/romainmazb/realestate/controllers/Lands.php
public function __construct()
{
    parent::__construct();
    BackendMenu::setContext('RomainMazB.RealEstate', 'main-menu', 'lands-menu');

    Event::listen('backend.form.refreshFields', function ($formWidget, $allFields) {
        // Disable refreshing with a simple switch box
        if( ! input('Land._activate_refresh_description'))
            return;

        $selected_descriptions = $this->getDescriptionForSelected();
        if(count($selected_descriptions)) {
            $fields = [];
            foreach($selected_descriptions as $house_model) {
                $fields["house_model///" . $house_model['id']] = [
                    'label'   => $house_model['name'],
                    'type' => 'Backend\FormWidgets\RichEditor',
                    'size' => 'huge',
                    'default' => $house_model['description'],
                    'cssClass' => 'tw-font-black'
                ];
            }
        } else {
            $fields["no_description"] = ['type' => 'section', 'comment' => 'Aucun modèle sélectionné'];
        }

        $formWidget->addFields($fields);
    });
}

public function getDescriptionForSelected() {

    $selected = input('Land._selected_house_models', []);

    if(empty($selected))
        return [];

    return HouseModel::select(['id', 'name', 'description'])
                                    ->whereIn('id', $selected)
                                    ->get()
                                    ->toArray();
}
// fields.yaml
_selected_mixable_house_models:
    label: 'Choisissez les modèles de maisons à mixer'
    type: checkboxlist
    cssClass: 'inline-options'

_offers_descriptions:
    label: 'Description des projets de maisons neuves'
    type: section
    dependsOn:
        - _activate_refresh_description
        - _selected_mixable_house_models

_activate_refresh_description:
    label: Activer l'affichage des textes automatiques
    type: switch

And here is the result:
ezgif com-video-to-gif

@mjauvin
Copy link
Contributor

mjauvin commented Mar 26, 2020

@RomainMazB is there a cleaner way to detect changes in fields after the event is fired?

@mjauvin
Copy link
Contributor

mjauvin commented Mar 26, 2020

Like:

$updated = $fieldsBeforeRefresh != $this->allFields;

@RomainMazB
Copy link
Contributor Author

RomainMazB commented Mar 26, 2020

@mjauvin I didn't find anyone, I know the array_merge(array_diff_key(),array_diff_key()) is really ugly and performs a loop where in 99% case you will not need it but afaik it's the best solution to be sure to detect any key changes in two array:

array_diff_key only detects the keys who's not present in the first array which are in the second, so we are forced to perform it twice to intersect the results with array_merge

I could probably have done that with array_intersect_key which catches the keys who are matching in both array, but still, we are forced then to "inverse" the selection with another loop, not so better.

I spent two hours to search another way to do just that line. Open to any idea who can solved that, I will be the first who'll enjoy it :)

I can create a function to be more eyefriendly but that doesn't change the fact it's ugly :]

The problem with the == comparator is that any change in the array such as a value, a required parameter or any other changes would be triggered as a change.

I can also add a parameter to the formWidget to perform this loop just when needed and mentioned by the developer.

Something like:

/**
* @var bool Watch for fields changes on refresh event.
*/
public $structureMetamorph = false;

// ...

$fieldsBeforeRefresh = $this->allFields;
$this->fireSystemEvent('backend.form.refreshFields', [$this->allFields]);
if($this->structureMetamorph) {
    $fieldsStructureModifications = array_merge(array_diff_key($fieldsBeforeRefresh, $this->allFields), array_diff_key($this->allFields, $fieldsBeforeRefresh));
}
        
/**
 * Does any fields has been removed or added?
 */
$formStructureModified = (bool)$fieldsStructureModifications ?? false;

then from the controller we could activate the metamorph behavior:

Event::listen('backend.form.refreshFields', function ($formWidget, $allFields) {
    $formWidget->structureMetamorph = true;
    $formWidget->addFields();
    $formWidget->removeField('someField');
}

@mjauvin
Copy link
Contributor

mjauvin commented Mar 26, 2020

Try the test I provided above, seems to work for me...

@RomainMazB
Copy link
Contributor Author

RomainMazB commented Mar 26, 2020

I have done a test, look at it

This would happen when just a field parameter have changed(which is why the event "backend.form.refreshFields" is for).

I'm about to perform a benchmark for curiosity to see which would be the best between my ugly line and array_intersect_key and an "selection inversion".

@mjauvin
Copy link
Contributor

mjauvin commented Mar 26, 2020

@RomainMazB don't you want the form to update if you change fields values in the backend.form.refreshFields event handler?

@RomainMazB
Copy link
Contributor Author

RomainMazB commented Mar 26, 2020

I do, but that behavior is already handled with the beforeRefresh event:

$dataHolder = (object) ['data' => $saveData];
$this->fireSystemEvent('backend.form.beforeRefresh', [$dataHolder]);
$saveData = $dataHolder->data;

We can't use the beforeRefresh event to change our fields, only the data are passed in this event.

And if we changes the data with the refreshFields event handler, with the == comparator, it will trigger the form partial refresh even if no field were added/removed. Which is not we want.

I think the best compromise between performance and usability would be the structureMetamorph or whatever named property to trigger the fields comparison only when needed.

Just for the curious about the benchmark, with 30'000'000 iterations (the difference was too small under that):

--------------------------------------
|        PHP BENCHMARK SCRIPT        |
--------------------------------------
Start : 2020-03-26 16:55:15
Server : ?@?
PHP version : 7.4.4
Platform : Linux
--------------------------------------
test_ArrayIntersectKey    : 36.294 sec.
test_ArrayDiffKey         : 31.517 sec.
--------------------------------------
Total time:               : 67.811 sec.

@RomainMazB
Copy link
Contributor Author

RomainMazB commented Apr 23, 2020

Just a little bump with more informations.

I tried to do without modifying the file directly and tried to override/extend it properly.

The first step obviously were to override the beforeRefresh method to implement the behavior I need. So I've created a custom FormWidget:

<?php

namespace RomainMazB\RealEstate\Classes;

use Backend\Widgets\Form;

class FormWidget extends Form
{
    public function onRefresh()
    {

        // ... Useless lines for the demonstration

        $fieldsBeforeRefresh = $this->allFields;
        $this->fireSystemEvent('backend.form.refreshFields', [$this->allFields]);

        $fieldsStructureModifications = array_merge(
            array_diff_key($fieldsBeforeRefresh, $this->allFields),
            array_diff_key($this->allFields, $fieldsBeforeRefresh)
        );

        // ... Useless lines for the demonstration

        if (empty($result) || $formStructureModified) {
            $result = ['#'.$this->getId() => $this->makePartial('form')];
        }

        // ... Useless lines for the demonstration
    }
}

But since the original FormWidget is a "top-level one", I faced up an issue because the instantation of the $formWidget is hard-coded in \Backend\Behaviors\FormController. So I had to override this method too:

<?php


namespace RomainMazB\RealEstate\Classes;

use Backend\Behaviors\FormController as BaseFormController;

class FormController extends BaseFormController
{
    public function initForm($model, $context = null)
    {

        // ... Useless lines for the demonstration
        
        $this->formWidget = $this->makeWidget('RomainMazB\RealEstate\Classes\FormWidget', $config);

        // ... Useless lines for the demonstration
    }
}

Then for sure, I faced another issue because the form widget partials were not found. So I had to copy the whole october/modules/backend/widgets/form/partials/ into /plugins/romainmazb/realestate/classes/formwidget/partials

Now I get it to work, but I lose the updates for some really important class/partials from october.

Hope this pull request will be merged, let me know if you want me to make more modification, maybe the addition of the structureMetamorph or whatever named propertie discussed at the end of this comment to not trigger this behavior when not needed (the default would be false).

If not merged, this comment will stand as a How-To :)

@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants