FIX for #4417: Ensuring ->removeValidation() is defined on instances of Validator. Setup new API for enabling/disabling validation. Documentation and better type handling.

[patricknelson]

Replying to original PR here: #4418

@dhensby @tractorcow

Firstly: I was in the process of setting this up in master, however I realized that maybe it'd make more sense to go ahead and rethink this and take a different approach (since we're going against master and we can be a little more flexible with the API). That is -- since there must not be very many implementations of this which are already broken by the lack of ->removeValidation() method, maybe it will make more sense to update the API take the "enable" vs "disable" approach. I think if you need the ability remove validation, you should also then have the ability to enable it again. So, I'd suggest having a new method like ->setEnabled($enabled) which ->validate() will check first before actually doing anything useful.

Secondly: Since needed to have ->removeValidation() available by default (due to it's calling/usage elsewhere), it makes sense for it to do something. In light of the above API (->setEnabled()) we can have it just be an alias for ->setEnabled(false) and, in order to make it functional, it can maintain state for the ->validate() method, which (if I'm correct) is the goal. That is, to be able to allow regular execution of the ->validate() method without triggering errors (to effectively disable/remove validation, but temporarily if desired).

Thirdly: This PR contains that along with a cleanup of ->errors to be consistent with ->getErrors() documentation (that is, it's expected to always be an array). An empty array loosely validates to false just as well as null does, plus we're being more explicit here and, in my opinion, it's simpler to just return the same type all of the time.

[patricknelson]

I forgot to mention two things:

1. More important: I typecast ->requireField() since it appears it would error anyway if the $data variable weren't an array (at least with a warning). I think it makes sense to do this since you should never be passing anything by an array here.
2. I added a bunch of PHPDoc too for clarity and type hinting.

[tractorcow]

Could you please add a test case? It could be a simple validator that fails if a field value isn't a set value, but passes if you call removeValidation() on it. :)

Code looks good to me!

[dhensby]

I don't see value in type hinting here

[patricknelson]

You're right, the is_array check happening below makes it somewhat pointless.

How can we ensure that developers utilizing this API are aware of their mistake ahead of time without causing crashes/issues when in production? Seems like an easy mistake to make since this method expects an array of fields despite the name being singular. I like doing this semantically at the language level (i.e. the array type hint) since this method will not work if this isn't an array, but I don't like crashing code should it make it into production and I also don't like the inelegant excess of calling trigger_error() in a minor circumstance such as this.

[dhensby]

I'd like to ask you not to use short array syntax too... but our coding conventions doc is mute on this point ;)

[patricknelson]

@dhensby Alright, no problem. I've updated the array syntax to be consistent with the rest of the entire system (d'oh on my part, good habit I suppose). I realize it's in master and the PHP version for 4.x will be well above 5.4 however, that Massive Refactor(TM) hasn't happened yet, so good luck to whoever is going to be responsible for that one :) Don't want to trip them up.

On the array type hinting, I was going to be bullheaded about this. I did that initially as a proactive measure since if you're calling this, you're doing so from code you wrote and apparently not CMS/framework code (I searched for references, couldn't find any). So if the user passes anything except an array there, it will give them an error anyway, except it'll be a little more cryptic. Apparently a string will still work (dammit SilverStripe) but at least if a user looks at the method definition, the syntactical sugar is already there to tell them "This should be an array!".

Why on earth is this code producing the error PHP Catchable fatal error: Argument 2 passed to requireField() must be of the type array, string given when executed in a plain PHP file, yet not producing it when incorporated into SilverStripe?!?

$fieldName = "foobar";
$data = "this is a string";
requireField($fieldName, $data);

function requireField($fieldName, array $data) {
    if(is_array($data[$fieldName]) && count($data[$fieldName])) {
        echo "I will never execute :(";
    }
}

That seems like a more important issue, since at this point SilverStripe (or something in it somewhere) is causing unexpected functionality at a reasonably low level. If I were to guess, I'd probably say it's somewhere in that ErrorControlChain object tampering with standard behavior.

Let me know your opinion on that please. In the meantime I'll try to stamp out that unit test @tractorcow

[kinglozzer]

@patricknelson Hmm, that should’ve already been solved in master: #2928

[patricknelson]

Ok, got my test case up @tractorcow

@kinglozzer Really? How long ago?

Also, now that I'm involved, I suppose everything spirals out of control, hah. I wanted to give an opinionated rant that apparently (now that I'm testing the Validator) it has an important dependency upon a Form instance and some contortions are requires to set that up first before it can even be tested. From my shortsighted standpoint, that's a little bit of a cross contamination of concerns but that's just, like, my opinion, man. 😖

[kinglozzer]

@patricknelson March 2014 is when the fix was merged. I just checked out master and it does error as expected for me:

[Recoverable Error] Argument 1 passed to TestClassFoo::bar() must be of the type array, string given, called in...

[patricknelson]

I got you @kinglozzer I see why it didn't happen. I have my own test script (call it test.php) which includes another script that I created that initializes the SilverStripe framework similar to this, used by another module (a portion of which I authored indirectly: https://github.com/titledk/ttools-silverstripe/blob/master/lib/workbench-include.php)

So it bootstraps differently than the main.php include, which also bootstraps differently from the cli-script.php file which bootstraps differently from . . . oh nevermind.

[dhensby]

Great, love the tests - I'm not a fan of type hinting, because really any iterable object should work here (the is_array check is silly and redundant and (looks to be) broken if an array isn't passed in).

[patricknelson]

That's right, I suppose my goal would be to have it fail earlier rather than later when that array style access is attempted but doesn't work. Also note that is_array() is being done on $data[$fieldName], not $data which is the error I was referencing originally (i.e. accessing it as an array first, via is_array(), when $data itself is not an array).

Would be a framework level thing, but maybe you could setup a standard method that could be called to detect if something allows that sort of array access (albeit an array or ArrayIterator), similar to the function proposed here: http://stackoverflow.com/questions/3584700/iterable-objects-and-array-type-hinting

But instead, it'd be like this so you can still use array style syntax $data[$key]:

function ss_is_array($var) {
    return (is_array($var) || $var instanceof ArrayIterator);
}

Or something.

[patricknelson]

@tractorcow @dhensby @kinglozzer

This has sat around a little while, so I've rebased on master again to keep things clean. Also just to ping and bug you guys 😎

p.s. Sorry @tractorcow I posted this comment on the wrong issue/PR a few seconds ago, in case you're confused (I'm following up on some of these since I've got some free time this week and next).

[tractorcow]

Haha, I replied to your other comment already. It's all good. :)

[dhensby]

@sminnee shall I just merge this in by hand to deal with the conflicts?

[tractorcow]

Validator has changed a lot in the last 2 years. I suggest to raise it as a new PR.

[dhensby]

Hmm - there have been too many changes for this to be merged by hand without having it reviewed again. I've rebased and worked it into the new file but I can't push. @patricknelson can you enable maintainers to push to this PR please? 👼

[patricknelson]

Ok @dhensby I've added you, @tractorcow @sminnee and @kinglozzer as collaborators to my fork of silverstripe-framework. I've never had much time to follow up on my issues/PR's since usually my only time is within the day or two of initially submitting them 😔

[dhensby]

@patricknelson you don't have to add us to the whole repo, there's a check box on the right hand side of pull request pages saying "allow contributors [write access]"

But access to the whole fork works too ;)

[patricknelson]

Ah there it is, got it @dhensby . I'll revoke ;) Let's do it the right way, sorry @tractorcow @kinglozzer @sminnee

[tractorcow]

Probably need a getter if we have a setter.

[tractorcow]

Linting issues:

FILE: ...rstripe/silverstripe-framework/tests/php/Forms/ValidatorTest.php
----------------------------------------------------------------------
FOUND 8 ERRORS AFFECTING 7 LINES
----------------------------------------------------------------------
 16 | ERROR | [x] Opening brace of a class must be on the line after
    |       |     the definition
 21 | ERROR | [x] Spaces must be used for alignment; tabs are not
    |       |     allowed
 21 | ERROR | [x] Spaces must be used for alignment; tabs are not
    |       |     allowed
 22 | ERROR | [x] Spaces must be used for alignment; tabs are not
    |       |     allowed
 24 | ERROR | [x] Opening brace should be on a new line
 27 | ERROR | [x] Expected 1 space after FOREACH keyword; 0 found
 35 | ERROR | [x] Opening brace should be on a new line
 56 | ERROR | [x] The closing brace for the class must go on the next
    |       |     line after the body
----------------------------------------------------------------------
PHPCBF CAN FIX THE 8 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
Time: 42.37 secs; Memory: 59.75Mb

[patricknelson]

Was there a poll/vote anywhere about these coding conventions per chance? Couldn't find easily after a quick googling (in case you have on-hand).

[sminnee]

@patricknelson we adopted the PSRs; there were previous discussions about whether to step into line with the PSRs but beyond "yes, let's do that" we didn't get into the details of specific rules. I don't think it was a poll, the consensus was pretty clear that PSRs are the way to go.

[tractorcow]

Well, the discussion goes back years and years... it took us quite a while to actually get from the initial conception to actually moving over to psr2.

https://groups.google.com/forum/#!topic/silverstripe-dev/_ebPef7HgKQ

In fact there are still several parts of psr2 we don't yet adhere to (listed exemptions in framework/phpcs.xml.dist), but we are looking to remove those as we improve our adherence to the standard. :)

[patricknelson]

Oh, how ever will I learn to foreach ( instead of foreach(, place my curly braces on a new line and crack my egg on the small end? Shall I abstain in order to annoy the pedants?

Sarcasm aside, I get the need for the formatting standards since you'll have about as many variations in coding style as you have coders, so ¯\_(ツ)_/¯ I just think halting on the foreach should be soft, non-fatal (notice/warning, not error). Space indentation, even the curly braces on a new line (which I certainly don't prefer): sure, since it more severely affects consistency and readability.

@tractorcow I can't find phpcs.xml.dist. @dhensby Where's the linter?

[patricknelson]

I think I answered both questions, sorry. https://github.com/silverstripe/silverstripe-framework/blob/master/composer.json#L87 @tractorcow did you mean tests/phpcs/ruleset.xml?

[tractorcow]

See #6908. Once this is merged you can just do composer run-script lint-clean and it'll clean up your code before pull request stage.

You can use an IDE plugin to automatically show linting issues as you go.

[sminnee]

@patricknelson most of us run with linters in our editors to pick up on these things as you're editing. What editor do you used?

Also: "composer run-script lint" runs the checks locally if that helps

[sminnee]

@tractorcow can we correct the location of the ruleset so that IDEs can be configured more easily?

[sminnee]

Patrick, my view on syntax rules is that they're either hard rules or no-rules-at-all — soft rules merely rot.

As a community I think getting the IDE linters into our contributors workflows is probably the best outcome and so happy to help with that/.

[tractorcow]

@tractorcow can we correct the location of the ruleset so that IDEs can be configured more easily?

My PR moves it to the standard "default" location #6908, so it should (in more cases) not need specifying if runinng the linter on the repository root.

fixed

[dhensby]

We are greed and ready to go :)

[patricknelson]

@sminnee

Patrick, my view on syntax rules is that they're either hard rules or no-rules-at-all — soft rules merely rot.

I agree 😄 Not my project and I've had less and less time to contribute to PR's, so despite the lint rules making it more difficult to swallow (so to speak), I can only ... deal with it 🕶️

[sminnee]

Did you manage to get your IDE set up to respect the linter rules? Did merging 6908 make it easier?

[dhensby]

@tractorcow are you happy now?