Update form.php

[bauer-git]

Fix possible access error and efficiency issues. (Code calling functions when it doesn't need to.)

[pollen8]

yes i think it should be done to ensure that the errors array is always consistent regardless of whether errors were found or not

[bauer-git]

OK.
But checking for if(!empty($this->errors)) is still not what should be checked here, no? It should be to checking for If(!$ok). Like I said, there's a reason that $ok flag was created to begin with. If left as is, this will continue to allow the FabrikWorker::getPluginManager()->runPlugins('onError', $this) function to be run even when it does not need to be run.

[pollen8]

i agree with the change to if ($ok)... but $this->setErrors($this->errors) should be outside that if statement

[Sophist-UK]

Review comment still outstanding - this PR cannot be merged until it has been addressed.

[Sophist-UK]

Please remove the comments and annotate the commit instead.

[bauer-git]

It's good to know that you see things as I do, Sophist. (Even the 'plain English' explanation comment above the code says "If the user can't view or edit the element, then don't validate it".)

I'm trying to learn and 'play by the rules' - but I don't understand what you mean by "annotate the commit instead.". I am willing to do whatever is needed - but if I don't understand, I tend to just 'give up'. Which is probably why I never responded to Rob's last instructions - which IMO, pretty much wipe out the whole purpose of my changes.

[cheesegrits]

Nobody has actually addressed my use case. The reason that was put in was for a specific case. Elements can still get assigned values during submission, even if the user doesn't have view access. The only entirely safe condition not to validate is if they have neither edit nor view, when the element (and hence validation message) is not visible on the form. Period. I know, because I remember writing that code, and the two use cases I was addressing at the time.

We've run into this situation a lot, where you see some behavior that doesn't fit your use, and you dig your heels in, ignoring me when I raise other use cases. But people use Fabrik in a lot of different ways, and I have decide where I have to compromise to accommodate corner cases.

If it's a serious problem for your use case, then run a fork. This is GNU licensed, open source software, on github.

[Sophist-UK]

Hugh - Your use case is when element is not visible i.e. you cannot use and cannot view and is still being addressed.

The use case addressed with this PR is to skip validation if either:

a. No view - it is not visible to the user so you cannot show the error message and the user cannot correct the input value to be able to submit the form; or

b. No edit - the element may or may not be visible, but either way the user cannot fix it.

So the use case in this PR still includes your use case along with a broader use case.

That said, this may not be how Fabrik is supposed to work - perhaps Fabrik should validate situations where the field is read-only but visible - the error will be visible, but the user cannot change it to make the submit work. An example of this might be using a calc field to do some complex validation that cannot be done any other way and for the calc field to have an element validation on it to display an error.

[bauer-git]

All I can say is... Wow.
I've known some hot-headed people in 65+years on the planet - but to just throw that all out without any further debate, or even understanding the extent of the problems it causes, is just mind-blowing.

The initial 'if' condition was just one part of the problem.
I'm not even sure if Hugh understands that I'm not questioning his logic - I'm questioning the syntax of the 'if (!$elementModel->canUse() && !$elementModel->canView())' expression. He seems to explain it in the exact same 'plain English' that makes me agree with his logic - yet the syntax doesn't seem to match that.

The OTHER issue is the 'if (!empty($this->errors))' that is being used - when $this->errors is NEVER empty. '$this->errors' might be an array containing a lot of empty arrays - but it's never empty (as php sees it) because a new key is added to it each time through the 'foreach ($elementModels as $elementModel)' loop! (Even Rob agreed with me on that - yet no one fixed this almost 2 years later?)

This later issue is the reason an element that has a validation 'Condition' is being validated regardless. And THAT in turn causes validation errors sometimes in situations where the element isn't even visible (and the 'Condition' php code was supposed to take care of that.)

That is my case - but it surely cannot be as rare as to even label it a "corner case".
And as I mentioned, this also plays havoc with the 'Toggle Submit' form feature used with 'Must Validate' elements.

And this is exactly the same issue that Hugh claims he was trying to prevent when he wrote that 'if condition' with the bad/questionable syntax.

So... could it be that the cause of the problem he was trying to fix at the time "as he remembers" was actually the use of 'if (!empty($this->errors))' when it should be 'if (!$ok)'?

The reason I give up and usually don't even argue with people anymore is because I learned long ago not to argue with programmers - especially Joomla 3rd party developers. They have the most delicate egos on the planet. And if you bruise an ego you're at risk of getting banned from the website.

I don't know what to make of this experience, really. I have no idea the politics around here - I assumed it a democracy, but for all I know it could be run by a bunch of Nazis. At any rate, I'm getting too old to even care any more. I do enjoy helping others but it's not much encouragement when my few 'spirited debates' I've had around here always turn into some heavy-handed fist pounding by the Fuehrer.

If you want to have an extension that has known bugs and doesn't own up to its promises and supposed features - that's what you'll have. It's not my reputation on the line. Though I would like to proudly boast about my contributions and encourage others to buy into the product. But that's kind of hard to do right now.