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

Ability to provide all initialized forms #307

Closed
wants to merge 1 commit into from

Conversation

drzraf
Copy link
Contributor

@drzraf drzraf commented Nov 29, 2018

Provide Form->getForms() that return initialized forms.

If I want to post-process a form (disregarding caching), I may use getForm() but it's too limited.
If I'm only interested in forms having a xtra_field set to 1, independently of their name or route, I'm stuck. Moreover getForm() fails when it comes to modular pages.

So just give users and developers the power to proceed with their data. Don't lock them with protected properties when API do not provide any (right) way to access such a data.

See also getgrav/grav#2277

edit: flat_forms may be better after all.

@drzraf drzraf changed the title Provide all initialized forms Ability to provide all initialized forms Nov 29, 2018
@mahagr
Copy link
Member

mahagr commented Dec 1, 2018

Here are some examples of how you can manipulate the forms and their data from your plugin. There are likely more, but there I took from a single project where I'm customizing form per page.

Adding a form to the page from your plugin:

    public function onFormPageHeaderProcessed(Event $event)
    {
        /** @var Page $page */
        $page = $event['page'];

        if ($page->template() === 'flex-shop') {
            $form = CompiledYamlFile::instance('blueprints://products/form.yaml')->content();
            }
            if (isset($form['form'])) {
                $header = $event['header'];
                $header->form = $form['form'];
            }
        }
    }

Initialize the form:

    public function onFormInitialized(Event $event)
    {
        /** @var Uri $uri */
        $uri = $this->grav['uri'];

        /** @var \Grav\Plugin\Form\Form $form */
        $form = $event['form'];

        $id = $uri->param('id');
        if ($form->name() === 'flex-shop-order') {
            // Load products somehow...
            $product = Products::get(...);
            if (!$product) {
                return;
            }

            $product->updateForm($form);
        }
    }

Custom validation and data conversion:

    public function onFormValidationProcessed(Event $event)
    {
        /** @var Form $form */
        $form = $event['form'];

        if ($form->name() === 'flex-shop-product-form') {
            $data = $form->getData();

            $size = $data->get('size');
            $size['sf'] = (float) ($size['sf'] ?? 0);
            foreach ($size['area'] as &$area) {
                $area['length'] = (float) ($area['length'] ?? 0);
                $area['width'] = (float) ($area['width'] ?? 0);
            }
            unset($area);

            if (!$size) {
                throw new ValidationException('No size!');
            }
            $data->set('size', $size);
        }
    }

Post-process form, like save object, etc:

    public function onFormProcessed(Event $event)
    {
        /** @var Form $form */
        $form = $event['form'];
        $action = $event['action'];
        $params = $event['params'];

        if ($action === 'my_action' && $params == true) {
            // You can do whatever here
        }
    }

@drzraf
Copy link
Contributor Author

drzraf commented Dec 1, 2018

Thank you for the examples.
Anyway let's see an example where form events are limiting:

        $this->enable([
            // Triggered: by form plugin during onPageProcessed(), which itself is ONLY triggered on uncached pages
            // Mission: Adds custom defaults options to form
            'onFormPageHeaderProcessed' => ['onFormPageHeaderProcessed', 0],
            // Triggered: always
            'onTwigSiteVariables'       => [
                // Mission: register and enqueue css, js and translation (only apply if forms are being output
                ['onTwigSiteVariables', 10],
                // Mission: inline-js the mailing-list configuration ($form-specific)
                ['onTwigSiteVariablesMailingList', 10]
            ],
        ]);

Here the two hooks @onTwigSiteVariables are onTwigSiteVariables which intent is to output assets only if a form (a specific/modified/whatever form) is being generated.
Same for onTwigSiteVariablesMailingList which inlineJs(json_encode(config($var))); where config($vars) depends upon the form being display.

Cache is also an important topic and beat me during my previous attempts. onFormPageHeaderProcessed is not run for cached pages and in some situation this may be an additional problem.

I hope this clarify why I think a direct and unconditional access to currents forms is needed and beneficial.

Side note: my forms do not submit to Grav.

@mahagr
Copy link
Member

mahagr commented Dec 3, 2018

You missed onFormInitialized event where I customize the form in real time.

Also, you can register your own services to the $grav object, which allows you to get the current form. Just be careful not to use a too general name as you do not want to replace existing service.

@drzraf
Copy link
Contributor Author

drzraf commented Dec 4, 2018

onFormInitialized is nice (I'm currently trying to adapt my code to use it), but:

  1. in term of code organization it feels smarter to add several listeners onTwigSiteVariable (esp. if they have different purposes) with the ability to check the state of Grav/Form-plugin rather than having to bundle and run them all inside one onFormInitialized.
  2. onFormInitialized runs multiple time (once per existing form in the Grav instance actually) while onTwigSiteVariable is garanteed to run only once which is a must-have if listeners call addInlineJs()

NB: the fact that onFormInitialized runs for form which are not intended to be rendered seems like an issue by itself.

@drzraf
Copy link
Contributor Author

drzraf commented Dec 4, 2018

Latest, biggest (and actually blocking) problem is that onFormInitialized is not run on cached page.
As a consequence : any assets (inline or not) is not triggered what means cached pages are broken.

Note: You may want to think the opposite : that cache should take care of associated assets...

The underlying issue is that doctrine cache is only enabled for individuals pages but is not for a (modular) page as a whole.
That means cached version of every individual components is restored but any modification affecting the page as a whole done during initial rendering (= any deferred operation, like enqueing assets, deferred blocks, ...) is not recorded.

I already hit this issue when I tried to defer the HTML output of some bootstrap modals into the footer.

@drzraf
Copy link
Contributor Author

drzraf commented Dec 6, 2018

The problem highly relates to getgrav/grav#1934.
which IHMO should be reopened (@rhukster) because don't cache is not an adequate resolution when it applies to any modular page whose template depends upon on assets, variables, ...

@drzraf
Copy link
Contributor Author

drzraf commented Jan 23, 2019

Looking at https://github.com/getgrav/grav/blob/1.6/CHANGELOG.md I found two very interesting additions:

It's worth trying but I don't think these two will provide the needed workaround and I don't think they will resolve the underlying issue getgrav/grav#1934

@drzraf
Copy link
Contributor Author

drzraf commented Feb 20, 2019

I see that the constructor of Form in 3.x makes use of it and works well. I started relying on it.

@drzraf drzraf closed this Feb 20, 2019
@mahagr
Copy link
Member

mahagr commented Feb 21, 2019

Cool, thanks. :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants