Re: [Catalyst] Handling of "keywords" for controller methods
On Wed, May 13, 2009 at 08:28:22PM +0200, Roland Lammel wrote: > For the records, just did my first commit for Catalyst to resolve this. > > Thnx for directions. Welcome to the team. I think you forgot to add yourself to http://search.cpan.org/~flora/Catalyst-Runtime-5.80003/lib/Catalyst.pm#CONTRIBUTORS though ... (alphabetical by IRC nick please) -- Matt S Trout Catalyst and DBIx::Class consultancy with a clue Technical Director and a commit bit: http://shadowcat.co.uk/catalyst/ Shadowcat Systems Limited mst (@) shadowcat.co.ukhttp://shadowcat.co.uk/blog/matt-s-trout/ ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Handling of "keywords" for controller methods
For the records, just did my first commit for Catalyst to resolve this. Thnx for directions. +rl On Wed, May 13, 2009 at 14:15, Tomas Doran wrote: > Roland Lammel wrote: > >> I actually did that in intial patch, that just showed the erronous >> behaviour, I'll add that together and repost to the list, with a note of why >> that got changed in the test itself. >> > > Great stuff. > > Feel free to drop by irc and demand a commit bit so you can just commit it, > rather than having to repost a diff if you want :) > > Cheers > > t0m > ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Handling of "keywords" for controller methods
Roland Lammel wrote: I actually did that in intial patch, that just showed the erronous behaviour, I'll add that together and repost to the list, with a note of why that got changed in the test itself. Great stuff. Feel free to drop by irc and demand a commit bit so you can just commit it, rather than having to repost a diff if you want :) Cheers t0m ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Handling of "keywords" for controller methods
On Tue, May 12, 2009 at 14:07, Tomas Doran wrote: > Roland Lammel wrote: > >> Here is the very simplistic patch, which only renames the "actions" >> attribute to "_controller_actions" in Catalyst::Controller. Test suite still >> passes with the patch and my app that originally showed that error, is now >> also working like a charm. >> > > Great stuff, change looks perfect. > > Could you also add an action named 'actions' to one of the TestApp > controllers in the test suite, so that there is a regression test and > evidence in the log/tests of _why_ this got changed? > > This should be fairly trivial, as adding just an empty method should cause > everything to explode, right? I actually did that in intial patch, that just showed the erronous behaviour, I'll add that together and repost to the list, with a note of why that got changed in the test itself. > > > TIA > t0m > > > ___ > List: Catalyst@lists.scsys.co.uk > Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst > Searchable archive: > http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ > Dev site: http://dev.catalyst.perl.org/ > -- Roland Lammel QuikIT - IT Lösungen - flexibel und schnell Web: http://www.quikit.at Email: i...@quikit.at "Enjoy your job, make lots of money, work within the law. Choose any two." ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Handling of "keywords" for controller methods
Roland Lammel wrote: Here is the very simplistic patch, which only renames the "actions" attribute to "_controller_actions" in Catalyst::Controller. Test suite still passes with the patch and my app that originally showed that error, is now also working like a charm. Great stuff, change looks perfect. Could you also add an action named 'actions' to one of the TestApp controllers in the test suite, so that there is a regression test and evidence in the log/tests of _why_ this got changed? This should be fairly trivial, as adding just an empty method should cause everything to explode, right? TIA t0m ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Handling of "keywords" for controller methods
Here is the very simplistic patch, which only renames the "actions"
attribute to "_controller_actions" in Catalyst::Controller. Test suite still
passes with the patch and my app that originally showed that error, is now
also working like a charm.
Cheers
+rl
On Tue, May 12, 2009 at 00:07, Roland Lammel wrote:
> Ok, I'll do a patch renaming it to "_controller_actions" that *should* be
> sensible enough to not clash in the future.
>
> +rl
>
>
> On Mon, May 11, 2009 at 19:22, Hans Dieter Pearcey <
> hdp.perl.catalyst.us...@weftsoar.net> wrote:
>
>> On Mon, May 11, 2009 at 06:13:19PM +0100, Matt S Trout wrote:
>> > That's a bug, the attribute should -not- be called 'actions'.
>>
>> Or, at least, its accessor shouldn't. (Naming the attribute itself
>> 'actions'
>> vs. '_actions' is a matter of taste.)
>>
>> hdp.
>>
>> ___
>> List: Catalyst@lists.scsys.co.uk
>> Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
>> Searchable archive:
>> http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
>> Dev site: http://dev.catalyst.perl.org/
>>
>
>
>
> --
> Roland Lammel
> QuikIT - IT Lösungen - flexibel und schnell
> Web: http://www.quikit.at
> Email: i...@quikit.at
>
> "Enjoy your job, make lots of money, work within the law. Choose any two."
>
--
Roland Lammel
QuikIT - IT Lösungen - flexibel und schnell
Web: http://www.quikit.at
Email: i...@quikit.at
"Enjoy your job, make lots of money, work within the law. Choose any two."
diff -u -r Catalyst-Runtime-5.80003/lib/Catalyst/Controller.pm Catalyst-Runtime-5.80003-fix-actions-keyword/lib/Catalyst/Controller.pm
--- Catalyst-Runtime-5.80003/lib/Catalyst/Controller.pm 2009-04-27 14:22:54.0 +0200
+++ Catalyst-Runtime-5.80003-fix-actions-keyword/lib/Catalyst/Controller.pm 2009-05-12 00:18:37.0 +0200
@@ -29,7 +29,7 @@
predicate => 'has_action_namespace',
);
-has actions =>
+has _controller_actions =>
(
is => 'rw',
isa => 'HashRef',
@@ -41,7 +41,7 @@
my $action = delete $args->{action} || {};
my $actions = delete $args->{actions} || {};
my $attr_value = $self->merge_config_hashes($actions, $action);
-$self->actions($attr_value);
+$self->_controller_actions($attr_value);
}
=head1 NAME
@@ -260,7 +260,7 @@
# superior while mantaining really high degree of compat
my $actions;
if( ref($self) ) {
-$actions = $self->actions;
+$actions = $self->_controller_actions;
} else {
my $cfg = $self->config;
$actions = $self->merge_config_hashes($cfg->{actions}, $cfg->{action});
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Handling of "keywords" for controller methods
Ok, I'll do a patch renaming it to "_controller_actions" that *should* be sensible enough to not clash in the future. +rl On Mon, May 11, 2009 at 19:22, Hans Dieter Pearcey < hdp.perl.catalyst.us...@weftsoar.net> wrote: > On Mon, May 11, 2009 at 06:13:19PM +0100, Matt S Trout wrote: > > That's a bug, the attribute should -not- be called 'actions'. > > Or, at least, its accessor shouldn't. (Naming the attribute itself > 'actions' > vs. '_actions' is a matter of taste.) > > hdp. > > ___ > List: Catalyst@lists.scsys.co.uk > Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst > Searchable archive: > http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ > Dev site: http://dev.catalyst.perl.org/ > -- Roland Lammel QuikIT - IT Lösungen - flexibel und schnell Web: http://www.quikit.at Email: i...@quikit.at "Enjoy your job, make lots of money, work within the law. Choose any two." ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Handling of "keywords" for controller methods
On Mon, May 11, 2009 at 06:13:19PM +0100, Matt S Trout wrote: > That's a bug, the attribute should -not- be called 'actions'. Or, at least, its accessor shouldn't. (Naming the attribute itself 'actions' vs. '_actions' is a matter of taste.) hdp. ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Handling of "keywords" for controller methods
On Mon, May 11, 2009 at 02:08:40AM +0200, Roland Lammel wrote: > Hi all, > > I was bitten by the naming of one of my controller methods. After digging a > little into that I found that the method name "actions" was not the best > idea to use. It is in catalyst terms a keyword (actually an attribute) of > Catalyst::Controller where also my controller inherits from. That's a bug, the attribute should -not- be called 'actions'. Can somebody do up a patch that renames it to something sensible? (_actions seems reasonably safe to me ...) -- Matt S Trout Catalyst and DBIx::Class consultancy with a clue Technical Director and a commit bit: http://shadowcat.co.uk/catalyst/ Shadowcat Systems Limited mst (@) shadowcat.co.ukhttp://shadowcat.co.uk/blog/matt-s-trout/ ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
[Catalyst] Handling of "keywords" for controller methods
Hi all,
I was bitten by the naming of one of my controller methods. After digging a
little into that I found that the method name "actions" was not the best
idea to use. It is in catalyst terms a keyword (actually an attribute) of
Catalyst::Controller where also my controller inherits from.
Although this is definitly a thing you should not do, all that was reported
to me during the failing startup was, that one of my controller was trying
to call a method on an undefined value (which was $c, the context object
actually). This was very misleading and quite hard to find. Others might
fall into the same pit.
With the powers of moose it should be possible to know that an attribute
"actions" (wich an accessor of the same name") should not be allowed to be
overridden by a method of a child class. This could be reported to the user
(while still failing to start up).
A test for showing the behaviour is attached as simple patch against
5.80003. It just shows the failure. If I know what the intended behaviour
should be, I can produce a more appropriate test.
Cheers and comments welcome.
+rl
--
Roland Lammel
QuikIT - IT Lösungen - flexibel und schnell
Web: http://www.quikit.at
Email: i...@quikit.at
"Enjoy your job, make lots of money, work within the law. Choose any two."
diff -r -N Catalyst-Runtime-5.80003/t/lib/TestApp/Controller/Keyword.pm Catalyst-Runtime-5.80003-IwCJtd/t/lib/TestApp/Controller/Keyword.pm
0a1,12
> package TestApp::Controller::Keyword;
>
> use strict;
> use base 'Catalyst::Controller';
>
> sub actions : Local {
> my ( $self, $c ) = @_;
> die("Call to controller method without context not allowed!") unless $c;
> $c->res->output("Test case for using keywords with catalyst\n");
> }
>
> 1;
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
