Skip to content

Conversation

@enumag
Copy link
Contributor

@enumag enumag commented Apr 20, 2015

This closes #71.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tady ten by tu být neměl aby se ověřilo že se to zavolá ikdyž je tam jenom jeden onSuccess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ono se to nevolalo pokud error nastal v posledním onSuccess handleru - jejich celkový počet nemá vliv, tedy je jedno zda tenhle onSuccess odstraním či nikoli

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pravda :)

@enumag
Copy link
Contributor Author

enumag commented Apr 20, 2015

Should I refactor it to this? It has better performance because isValid is not called multiple times.

(each isValid call means going through the whole component subtree)

        if (!$this->isValid()) {
            $this->onError($this);
        } elseif ($this->onSuccess) {
            foreach ($this->onSuccess as $handler) {
                $params = Nette\Utils\Callback::toReflection($handler)->getParameters();
                $values = isset($params[1]) ? $this->getValues($params[1]->isArray()) : NULL;
                Nette\Utils\Callback::invoke($handler, $this, $values);
                if (!$this->isValid()) {
                    $this->onError($this);
                    break;
                }
            }
        }

@dg
Copy link
Member

dg commented Apr 22, 2015

Can you write better commit message?

@enumag
Copy link
Contributor Author

enumag commented Apr 22, 2015

What's wrong with this one? I can't come up with anything better.

@vojtech-dobes
Copy link
Contributor

Form::fireEvents() calls onError even with only one success handler (deducing from original issue)

@enumag
Copy link
Contributor Author

enumag commented Apr 22, 2015

@vojtech-dobes That's actually wrong. The onError event previously wasn't called if the error was added by last onSuccess handler (regardless how many there are in total).

@enumag
Copy link
Contributor Author

enumag commented Apr 22, 2015

Maybe Form::fireEvents() can call onError even after last onSuccess handler ?

@fprochazka
Copy link
Contributor

@enumag

Form::fireEvents() calls onError even after last onSuccess handler

@enumag
Copy link
Contributor Author

enumag commented Apr 22, 2015

Commit message changed (thx @fprochazka). I've also changed the implementation to the one with better performance as mentioned above.

@vojtech-dobes
Copy link
Contributor

👍

@dg
Copy link
Member

dg commented Apr 23, 2015

Thank you

dg added a commit that referenced this pull request Apr 23, 2015
Fixed onError event dispatching
@dg dg merged commit 48fd7a4 into nette:master Apr 23, 2015
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.

Form::fireEvents() calls onError only when it has multiple success handlers

4 participants