in reply to [RFC: Catalyst::Plugin::AutoValidate] Easy request parameter validation with Catalyst

One off the bat. Same one expressed on the Catalyst list. If it's specific only to controllers it should be a controller base, not a plugin. Plugins should only be plugins if they cannot be generalized to work in any other fashion.

Use c3 if you can. NEXT is deprecated for new development.

Defaulting to "index" makes me a bit queasy. Index is peer-pressure deprecated and too much voodoo (automatic action) makes me uncomfortable with a package.

I hate seeing $_ in loops that call subroutines. Declared temp vars are cheaper than debugging.

If you do stick with a plugin, "Catalyst::Plugin::AutoValidate" in the config should just be "Plugin::AutoValidate."

You're using "/" for paths. Many devs are running Cat on Windows. You just broke their applications. :) Path::Class is guaranteed to be there for Cat stuff.

Setting hash keys against the request object strikes me as really bad... Just gut feeling.

So, how's that for encouragement? In fact, I hope you do continue with it. One thing I have never been satisfied with is validation stuff and the more competing entries the better as far as I'm concerned. Keep working on it!

  • Comment on Re: [RFC: Catalyst::Plugin::AutoValidate] Easy request parameter validation with Catalyst
  • Download Code

Replies are listed 'Best First'.
Re^2: [RFC: Catalyst::Plugin::AutoValidate] Easy request parameter validation with Catalyst
by mr_mischief (Monsignor) on Apr 10, 2008 at 09:19 UTC
    You're using "/" for paths. Many devs are running Cat on Windows. You just broke their applications. :) Path::Class is guaranteed to be there for Cat stuff.

    What about Catalyst causes it to break on Windows from using a solidus for paths? Most applications have no problem using it on Windows. IIRC the system internally even supports it, although the command interpreter does not. In any case, I've been using '/' as a path separator on Windows for years. 

    perl -e "print $_ . qq{\n} for glob q{'c:/documents and settings/*'}"

    [download]
    perl -e "opendir my $d, 'c:/documents and settings'; print $_ . qq{\n}
+ for readdir $d;"

    [download]
    perl -e "open my $f, '<', $ENV{systemdrive} . '/windows/system32/drive
+rs/etc/hosts'; print $_ while <$f>;"

    [download]
    perl -e "$ENV{PATH} = 'c:/windows/'; system( 1, 'notepad.exe', 'c:/win
+dows/system32/drivers/etc/hosts' );"

    [download]

    You might notice that last one is an example of the system using a path with '/' to find an application and launch it and that program using its argument with '/' as the separator to open the file. It's not subject to any special reworking by the Perl runtime.

    Having a portable path-handling library helps with platforms like MacOS Classic, VMS, and others. Any version of Windows that's still getting security updates from MS shouldn't need any special treatment for the path separator, though. That volume enumeration by letter in the paths is still oddball these days, but that's supposed to go away in the near future.

    If an application on Windows is broken by a path with '/' as the separator, there's a problem, IMO, in that application. The Windows CLI itself gets excused for mistakes of the past. Even it does the right thing with proper convincing: 

    dir "c:/windows/system"

    [download]
[reply]
[d/l]
[select]

      I actually knew that. What I don't know is what happens when you mix the two? "c:\windows\system32" . "/drivers/etc/hosts" for example. I can't believe that would work(?). Better to abstract them out and let the underlying stuff do what it really wants. Portable by design instead of chance.

[reply]
[d/l]
        That example actually does work, but your point in general is taken. Even code that's written with both Windows and *nix in mind could need to move to some exotic place. 
        dir "c:/windows\system"

        [download]
[reply]
[d/l]
Re^2: [RFC: Catalyst::Plugin::AutoValidate] Easy request parameter validation with Catalyst
by holli (Abbot) on Apr 10, 2008 at 09:14 UTC
    I gently disagree :) At least with your first points. AutoValidate isn't controller specific. All it does is looking at the request and leaving it's opinion in the request object by setting "validated" and "validation_error". Granted, validattion is a task which is traditionally done (if you can talk about traditions anyway here), but this module is an attemt to abstract that away from the controllers as much as possible. Also, being it a plugin i can enable it in a single place and have it active everywhere.

    I wasn't sure about C3, the Catalyst::Plugin::C3 says it's still experimental. But as it seems it's a simple replacement, when NEXT is ever abandoned.

    Defaulting to index is a sensible thing to do, I think. I need a file to put the spec in (here index.vcs). Without index that would be ".vcs" which is much worse.

    I agree with the rest. I'll make the paths compatible, turn the keys into accessors and replace $_.

    Thanks for your input mum :)

    Update: new version as per YourMothers suggestion:
    Read more... (7 kB)


    holli, /regexed monk/
[reply]
[d/l]
[select]

In Section Meditations